Check span owners (span lowering) in debug builds and fix missing lowerings by jdonszelmann · Pull Request #148863 · 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 Commits4 Checks12 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 }})
r? @oli-obk
There were some cases where we didn't lower spans properly:
- attributes; though these are hashed slightly differently, so I'm not convinced this is a problem right now. Though, if we could lower spans properly, it'd simplify some code significantly. Might be a perf win in the end.
- macro definitions that are encoded to hir when exported from a crate for example. This might turn out to be slower since the lowering takes time. Though it means that when you add a comment above a macro we don't have to recompile it which seems valuable.
- the visibility span of use items was lowered, but to the wrong parent.
Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html)
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
oli-obk is not on the review rotation at the moment.
They may take a while to respond.
This comment has been minimized.
This comment has been minimized.
rust-bors bot added a commit that referenced this pull request
Check span owners (span lowering) in debug builds and fix missing lowerings
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rust-bors bot added a commit that referenced this pull request
Check span owners (span lowering) in debug builds and fix missing lowerings
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rust-bors bot added a commit that referenced this pull request
Check span owners (span lowering) in debug builds and fix missing lowerings
☀️ Try build successful (CI)
Build commit: 473cf1d (473cf1d90b60379fd4df276817ffa9b24e4735a9, parent: 95aeb4696545eb4c9cbb68516f2912770e3846ea)
This comment has been minimized.
Finished benchmarking commit (473cf1d): comparison URL.
Overall result: ❌ regressions - 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 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | 0.2% | [0.1%, 0.3%] | 9 |
Max RSS (memory usage)
Results (primary 2.0%)
A less reliable metric. May be of interest, but not used to determine the overall result above.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | 2.0% | [2.0%, 2.0%] | 1 |
| Regressions ❌ (secondary) | - | - | 0 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | 2.0% | [2.0%, 2.0%] | 1 |
Cycles
Results (primary -3.2%, secondary -2.6%)
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) | - | - | 0 |
| Improvements ✅ (primary) | -3.2% | [-3.2%, -3.2%] | 1 |
| Improvements ✅ (secondary) | -2.6% | [-3.1%, -2.3%] | 4 |
| All ❌✅ (primary) | -3.2% | [-3.2%, -3.2%] | 1 |
Binary size
This benchmark run did not return any relevant results for this metric.
Bootstrap: 474.976s -> 473.989s (-0.21%)
Artifact size: 391.06 MiB -> 391.10 MiB (0.01%)
so, the question is: is this acceptable.
We lower more spans, and I checked the perf results locally, it's all in source_span and specifically the call made in span.with_parent: in other words, unsurprisingly, we spend more time lowering spans! Because indeed, we lower more spans. This takes time. However, the good part is that we theoretically, in a few cases, can be more incremental. I'd say these spans always had to be lowered and we just forgot, which is a bug. But the result is a slight regression.
It only shows up on the typenum and slightly the syn benchmarks, and even then only the incremental version. That's because:
- non-incremental benchmarks skip a lot of steps in span lowering, we don't need to track as much
- my hypothesis is that we mainly spend more time in lowering spans for macro defs. i.e. a crate with a lot of macros will compile slower. We probably don't have enough benchmarks with a lot of
global_asmoruseitems with enough spans in them to be significant.
I personally think not lowering spans is a bug and we should make this change. But there are measurable, smol, regressions on crates that have a lot of big macros, and we don't have a good way to measure the benefit of the lowering for actual incremental builds.
We could try crafting an incremental test that has less dirty queries after your changes. Then it becomes a fairly obvious bugfix. Just because our benchmarks don't show this doesn't mean it isn't an incremental problem for real users
Labels
Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html)
Performance regression.
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.