Skip to content

Conversation

@jdonszelmann
Copy link
Contributor

r? @oli-obk

There were some cases where we didn't lower spans properly:

  1. 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.
  2. 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.
  3. the visibility span of use items was lowered, but to the wrong parent.

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 12, 2025
@rustbot
Copy link
Collaborator

rustbot commented Nov 12, 2025

oli-obk is not on the review rotation at the moment.
They may take a while to respond.

@jdonszelmann
Copy link
Contributor Author

@bors2 try @rust-timer queue

@rust-timer

This comment has been minimized.

@rust-bors

This comment has been minimized.

rust-bors bot added a commit that referenced this pull request Nov 12, 2025
Check span owners (span lowering) in debug builds and fix missing lowerings
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 12, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-bors
Copy link

rust-bors bot commented Nov 12, 2025

💔 Test for ad40871 failed: CI. Failed jobs:

@jdonszelmann
Copy link
Contributor Author

@bors2 try @rust-timer queue

@rust-timer

This comment has been minimized.

@rust-bors

This comment has been minimized.

rust-bors bot added a commit that referenced this pull request Nov 12, 2025
Check span owners (span lowering) in debug builds and fix missing lowerings
@cjgillot cjgillot self-assigned this Nov 12, 2025
@rust-log-analyzer

This comment has been minimized.

@jdonszelmann
Copy link
Contributor Author

@bors2 try cancel

@rust-bors
Copy link

rust-bors bot commented Nov 12, 2025

Try build cancelled. Cancelled workflows:

@jdonszelmann
Copy link
Contributor Author

@bors2 try @rust-timer queue

@rust-timer

This comment has been minimized.

@rust-bors

This comment has been minimized.

rust-bors bot added a commit that referenced this pull request Nov 12, 2025
Check span owners (span lowering) in debug builds and fix missing lowerings
@rust-bors
Copy link

rust-bors bot commented Nov 12, 2025

☀️ Try build successful (CI)
Build commit: 473cf1d (473cf1d90b60379fd4df276817ffa9b24e4735a9, parent: 95aeb4696545eb4c9cbb68516f2912770e3846ea)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

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%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 12, 2025
@jdonszelmann
Copy link
Contributor Author

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_asm or use items 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants