assert more in release in rustc_ast_lowering by workingjubilee · Pull Request #142267 · rust-lang/rust (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation21 Commits2 Checks9 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
My understanding of the compiler's architecture is that in the ast_lowering crate, we are constructing the HIR as a one-time thing per crate. This is after tokenizing, parsing, resolution, expansion, possible reparsing, reresolution, reexpansion, and so on. In other words, there are many reasons that perf-focused PRs spend a lot of time touching rustc_parse, rustc_expand, rustc_ast, and then rustc_hir and "onwards", but ast_lowering is a little bit of an odd duck.
In this crate, we have a number of debug assertions. Some are clearly expensive checks that seem like they are prohibitive to run in actual optimized compiler builds, but then there are a number that are simple asserts on integer equalities, is_empty, or the like. I believe we should do some of them even in release builds, because the correctness gain is worth the performance cost: almost zero.
Status: Ongoing experiment that does not require reviewing and won't be merged in its current state.
label
Relevant to the compiler team, which will review and decide on the PR/issue.
label
This comment has been minimized.
⌛ Trying commit c62171a with merge d4e2ead…
To cancel the try build, run the command @bors2 try cancel.
rust-bors bot added a commit that referenced this pull request
…ing, r=
[EXPERIMENTAL] assert in release builds in rustc_ast_lowering
My understanding of the compiler's architecture is that in the ast_lowering crate, we are constructing the HIR as a one-time thing per crate. This is after tokenizing, parsing, resolution, expansion, possible reparsing, reresolution, reexpansion, and so on. In other words, there are many reasons that perf-focused PRs spend a lot of time touching rustc_parse, rustc_expand, rustc_ast, and then rustc_hir and "onwards", but ast_lowering is a little bit of an odd duck.
In this crate, we have a number of debug assertions. Some are clearly expensive checks that seem like they are prohibitive to run in actual optimized compiler builds, but then there are simple integer equalities. I am curious if the latter introduce a serious problem if we simply do them.
r? @ghost
☀️ Try build successful (CI)
Build commit: d4e2ead (d4e2ead1d10ad3fc360e74bb08414c0476225305)
This comment has been minimized.
Finished benchmarking commit (d4e2ead): comparison URL.
Overall result: ❌✅ regressions and improvements - please read the text below
Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.
Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.
@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression
Instruction count
Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | 0.2% | [0.1%, 0.3%] | 9 |
| Regressions ❌ (secondary) | 0.3% | [0.2%, 0.4%] | 10 |
| Improvements ✅ (primary) | -1.2% | [-1.2%, -1.2%] | 1 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | 0.1% | [-1.2%, 0.3%] | 10 |
Max RSS (memory usage)
This benchmark run did not return any relevant results for this metric.
Cycles
This benchmark run did not return any relevant results for this metric.
Binary size
This benchmark run did not return any relevant results for this metric.
Bootstrap: 754.885s -> 754.261s (-0.08%)
Artifact size: 372.30 MiB -> 372.35 MiB (0.01%)
hm. I suspect the index functions might be hot enough I need to back their diff out.
This comment has been minimized.
bors added a commit that referenced this pull request
…ing, r=
[EXPERIMENTAL] assert in release builds in rustc_ast_lowering
My understanding of the compiler's architecture is that in the ast_lowering crate, we are constructing the HIR as a one-time thing per crate. This is after tokenizing, parsing, resolution, expansion, possible reparsing, reresolution, reexpansion, and so on. In other words, there are many reasons that perf-focused PRs spend a lot of time touching rustc_parse, rustc_expand, rustc_ast, and then rustc_hir and "onwards", but ast_lowering is a little bit of an odd duck.
In this crate, we have a number of debug assertions. Some are clearly expensive checks that seem like they are prohibitive to run in actual optimized compiler builds, but then there are simple integer equalities. I am curious if the latter introduce a serious problem if we simply do them.
r? @ghost
☀️ Try build successful - checks-actions
Build commit: a729c24 (a729c24463fb0fe8f3af7881cfd00143616f9bfd)
This comment has been minimized.
Finished benchmarking commit (a729c24): comparison URL.
Overall result: no relevant changes - no action needed
Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.
@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression
Instruction count
This benchmark run did not return any relevant results for this metric.
Max RSS (memory usage)
Results (primary -3.5%, secondary -1.9%)
A less reliable metric. May be of interest, but not used to determine the overall result above.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | 8.5% | [8.5%, 8.5%] | 1 |
| Improvements ✅ (primary) | -3.5% | [-4.0%, -3.0%] | 3 |
| Improvements ✅ (secondary) | -2.5% | [-3.7%, -1.1%] | 18 |
| All ❌✅ (primary) | -3.5% | [-4.0%, -3.0%] | 3 |
Cycles
This benchmark run did not return any relevant results for this metric.
Binary size
This benchmark run did not return any relevant results for this metric.
Bootstrap: 754.321s -> 755.264s (0.13%)
Artifact size: 372.15 MiB -> 372.18 MiB (0.01%)
[EXPERIMENTAL] assert in release builds in assert more in release in rustc_ast_loweringrustc_ast_lowering
rustbot has assigned @compiler-errors.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Status: Ongoing experiment that does not require reviewing and won't be merged in its current state.
label
📌 Commit dd78c95 has been approved by oli-obk
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request
…n-ast-lowering, r=oli-obk
assert more in release in rustc_ast_lowering
My understanding of the compiler's architecture is that in the ast_lowering crate, we are constructing the HIR as a one-time thing per crate. This is after tokenizing, parsing, resolution, expansion, possible reparsing, reresolution, reexpansion, and so on. In other words, there are many reasons that perf-focused PRs spend a lot of time touching rustc_parse, rustc_expand, rustc_ast, and then rustc_hir and "onwards", but ast_lowering is a little bit of an odd duck.
In this crate, we have a number of debug assertions. Some are clearly expensive checks that seem like they are prohibitive to run in actual optimized compiler builds, but then there are a number that are simple asserts on integer equalities, is_empty, or the like. I believe we should do some of them even in release builds, because the correctness gain is worth the performance cost: almost zero.
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request
…n-ast-lowering, r=oli-obk
assert more in release in rustc_ast_lowering
My understanding of the compiler's architecture is that in the ast_lowering crate, we are constructing the HIR as a one-time thing per crate. This is after tokenizing, parsing, resolution, expansion, possible reparsing, reresolution, reexpansion, and so on. In other words, there are many reasons that perf-focused PRs spend a lot of time touching rustc_parse, rustc_expand, rustc_ast, and then rustc_hir and "onwards", but ast_lowering is a little bit of an odd duck.
In this crate, we have a number of debug assertions. Some are clearly expensive checks that seem like they are prohibitive to run in actual optimized compiler builds, but then there are a number that are simple asserts on integer equalities, is_empty, or the like. I believe we should do some of them even in release builds, because the correctness gain is worth the performance cost: almost zero.
bors added a commit that referenced this pull request
Rollup of 9 pull requests
Successful merges:
- #128425 (Make
missing_fragment_specifieran unconditional error) - #135927 (retpoline and retpoline-external-thunk flags (target modifiers) to enable retpoline-related target features)
- #140770 (add
extern "custom"functions) - #142176 (tests: Split dont-shuffle-bswaps along opt-levels and arches)
- #142248 (Add supported asm types for LoongArch32)
- #142267 (assert more in release in
rustc_ast_lowering) - #142274 (Update the stdarch submodule)
- #142276 (Update dependencies in
library/Cargo.lock) - #142308 (Upgrade
object,addr2line, andunwindingin the standard library)
Failed merges:
- #140920 (Extract some shared code from codegen backend target feature handling)
r? @ghost
@rustbot modify labels: rollup
try-job: aarch64-apple try-job: x86_64-msvc-1 try-job: x86_64-gnu try-job: dist-i586-gnu-i586-i686-musl try-job: test-various
bors added a commit that referenced this pull request
Rollup of 9 pull requests
Successful merges:
- #128425 (Make
missing_fragment_specifieran unconditional error) - #135927 (retpoline and retpoline-external-thunk flags (target modifiers) to enable retpoline-related target features)
- #140770 (add
extern "custom"functions) - #142176 (tests: Split dont-shuffle-bswaps along opt-levels and arches)
- #142248 (Add supported asm types for LoongArch32)
- #142267 (assert more in release in
rustc_ast_lowering) - #142274 (Update the stdarch submodule)
- #142276 (Update dependencies in
library/Cargo.lock) - #142308 (Upgrade
object,addr2line, andunwindingin the standard library)
Failed merges:
- #140920 (Extract some shared code from codegen backend target feature handling)
r? @ghost
@rustbot modify labels: rollup
try-job: aarch64-apple try-job: x86_64-msvc-1 try-job: x86_64-gnu try-job: dist-i586-gnu-i586-i686-musl try-job: test-various
rust-timer added a commit that referenced this pull request
Rollup merge of #142267 - workingjubilee:debug-assert-less-in-ast-lowering, r=oli-obk
assert more in release in rustc_ast_lowering
My understanding of the compiler's architecture is that in the ast_lowering crate, we are constructing the HIR as a one-time thing per crate. This is after tokenizing, parsing, resolution, expansion, possible reparsing, reresolution, reexpansion, and so on. In other words, there are many reasons that perf-focused PRs spend a lot of time touching rustc_parse, rustc_expand, rustc_ast, and then rustc_hir and "onwards", but ast_lowering is a little bit of an odd duck.
In this crate, we have a number of debug assertions. Some are clearly expensive checks that seem like they are prohibitive to run in actual optimized compiler builds, but then there are a number that are simple asserts on integer equalities, is_empty, or the like. I believe we should do some of them even in release builds, because the correctness gain is worth the performance cost: almost zero.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this pull request
github-actions bot pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request
This comment has been minimized.
Finished benchmarking commit (bd9d8ad): comparison URL.
Overall result: no relevant changes - no action needed
Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.
@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression
Instruction count
This benchmark run did not return any relevant results for this metric.
Max RSS (memory usage)
Results (secondary 2.7%)
A less reliable metric. May be of interest, but not used to determine the overall result above.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | 2.7% | [2.7%, 2.7%] | 1 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | - | - | 0 |
Cycles
Results (secondary 5.8%)
A less reliable metric. May be of interest, but not used to determine the overall result above.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | 5.8% | [5.8%, 5.8%] | 1 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | - | - | 0 |
Binary size
This benchmark run did not return any relevant results for this metric.
Bootstrap: 755.964s -> 690.393s (-8.67%)
Artifact size: 372.26 MiB -> 372.24 MiB (-0.00%)
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request
…iaskrgr
Rollup of 9 pull requests
Successful merges:
- rust-lang#128425 (Make
missing_fragment_specifieran unconditional error) - rust-lang#135927 (retpoline and retpoline-external-thunk flags (target modifiers) to enable retpoline-related target features)
- rust-lang#140770 (add
extern "custom"functions) - rust-lang#142176 (tests: Split dont-shuffle-bswaps along opt-levels and arches)
- rust-lang#142248 (Add supported asm types for LoongArch32)
- rust-lang#142267 (assert more in release in
rustc_ast_lowering) - rust-lang#142274 (Update the stdarch submodule)
- rust-lang#142276 (Update dependencies in
library/Cargo.lock) - rust-lang#142308 (Upgrade
object,addr2line, andunwindingin the standard library)
Failed merges:
- rust-lang#140920 (Extract some shared code from codegen backend target feature handling)
r? @ghost
@rustbot modify labels: rollup
try-job: aarch64-apple try-job: x86_64-msvc-1 try-job: x86_64-gnu try-job: dist-i586-gnu-i586-i686-musl try-job: test-various
antoyo pushed a commit to rust-lang/rustc_codegen_gcc that referenced this pull request
Labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.