Encode spans relative to the enclosing item by cjgillot · Pull Request #84373 · rust-lang/rust

cjgillot · 2021-04-20T18:32:44Z

The aim of this PR is to avoid recomputing queries when code is moved without modification.

This is achieved by :

storing the HIR owner LocalDefId information inside the span;
encoding and decoding spans relative to the enclosing item in the incremental on-disk cache;
marking a dependency to the source_span(LocalDefId) query when we translate a span from the short (Span) representation to its explicit (SpanData) representation.

Since all client code uses Span, step 3 ensures that all manipulations
of span byte positions actually create the dependency edge between
the caller and the source_span(LocalDefId).
This query return the actual absolute span of the parent item.
As a consequence, any source code motion that changes the absolute byte position of a node will either:

modify the distance to the parent's beginning, so change the relative span's hash;
dirty source_span, and trigger the incremental recomputation of all code that
depends on the span's absolute byte position.

With this scheme, I believe the dependency tracking to be accurate.

For the moment, the spans are marked during lowering.
I'd rather do this during def-collection,
but the AST MutVisitor is not practical enough just yet.
The only difference is that we attach macro-expanded spans
to their expansion point instead of the macro itself.

compiler/rustc_span/src/lib.rs

petrochenkov · 2021-04-20T18:54:02Z

I don't like the idea of keeping the ID in SpanData and would like to see a separate type for this, like LoweredSpan.
I see why it was faster to prototype this with a single Span type though.

cjgillot · 2021-04-24T18:32:27Z

@bors try @rust-timer queue

rust-timer · 2021-04-24T18:32:28Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-04-24T18:32:36Z

⌛ Trying commit d861775a3095b6bc7b23e4dc2acfb30a82f104c2 with merge 53c860c88de8437287861ee8c26eaedbe95dd7fb...

cjgillot · 2021-04-24T18:44:01Z

I don't like the idea of keeping the ID in SpanData and would like to see a separate type for this, like LoweredSpan.
I see why it was faster to prototype this with a single Span type though.

On the one hand, I agree with you, putting the ID in the SpanData is a bad hack, and invites silent invalidations.
On the other hand, I would like to assign the stable spans in the AST, to make macro-expanded spans relative to the enclosing macro.

I shall think a bit more about it. Creating a hir::Span and putting it everywhere is probably the most robust option.

bors · 2021-04-24T19:32:57Z

☀️ Try build successful - checks-actions
Build commit: 53c860c88de8437287861ee8c26eaedbe95dd7fb (53c860c88de8437287861ee8c26eaedbe95dd7fb)

rust-timer · 2021-04-24T19:32:58Z

Queued 53c860c88de8437287861ee8c26eaedbe95dd7fb with parent 2b68027, future comparison URL.

cjgillot · 2021-04-24T22:44:41Z

perf report : bad (I mean don't look, its really bad).
I think I've been too heavy handed in querifying source_map, which pessimizes MIR and CGU reuse.

src/test/incremental/spans_significant_w_debuginfo.rs

src/test/incremental/spans_significant_w_panic.rs

src/test/incremental/change_symbol_export_status.rs

cjgillot · 2021-04-25T12:11:28Z

@bors try @rust-timer queue

rust-timer · 2021-04-25T12:11:30Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-09-02T16:42:55Z

⌛ Trying commit f9e65cc5e495f6943d3c02cc4c4f2c30809379f2 with merge 4bbad094c33b7feb41e0f016c658bccffd0793bb...

bors · 2021-09-02T18:04:58Z

☀️ Try build successful - checks-actions
Build commit: 4bbad094c33b7feb41e0f016c658bccffd0793bb (4bbad094c33b7feb41e0f016c658bccffd0793bb)

rust-timer · 2021-09-02T18:05:00Z

Queued 4bbad094c33b7feb41e0f016c658bccffd0793bb with parent fcce644, future comparison URL.

rust-timer · 2021-09-02T22:17:57Z

Finished benchmarking try commit (4bbad094c33b7feb41e0f016c658bccffd0793bb): comparison url.

Summary: This change led to large relevant regressions 😿 in compiler performance.

Large regression in instruction counts (up to 2.2% on incr-unchanged builds of tuple-stress)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

petrochenkov · 2021-09-03T09:36:12Z

I don't have any more comments about the code, I'd just like to identify the regression sources.
If it's simply branches on parent and larger size of SpanData, then we cannot do anything and have to live with it until incremental_relative_spans compensates for the overhead from keeping span parents (or until the parents are removed if it doesn't compensate enough).
r? @michaelwoerister

michaelwoerister · 2021-09-07T11:54:37Z

Well, the regressions are not great but not catastrophic either. The compiler team decided to go ahead with this change via MCP rust-lang/compiler-team#443 in order to enable further experimentation in this area. So I would say we merge the PR now. The feature is behind a flag and we can still revert it if it turns out to be a dead end.

@cjgillot, you can r=michaelwoerister,petrochenkov after fixing the conflicts.

bors · 2021-09-08T23:58:20Z

☔ The latest upstream changes (presumably #80522) made this pull request unmergeable. Please resolve the merge conflicts.

We force the relative span's parent to be absolute. This avoids having to handle long dependency chains.

Now that we encode spans relative to the items, the item's own span is never actually hashed as part of the HIR. In consequence, we explicitly include it in the crate hash to avoid missing cross-crate invalidations.

We only do this operation when incremental compilation is enabled. This avoids pessimizing the span handling for non-incremental compilation.

cjgillot · 2021-09-11T17:11:43Z

@bors r=michaelwoerister,petrochenkov

About the perf regression: this PR implements an opt-in feature to reduce span-related recomputations for incr-comp. Measurements in #84762 show up to -25% instruction reduction when enabled. When disabled, the perf hit appears to be a consistent 1-2% instruction hit.

bors · 2021-09-11T17:11:45Z

📌 Commit 7842b80 has been approved by michaelwoerister,petrochenkov

bors · 2021-09-11T23:35:36Z

⌛ Testing commit 7842b80 with merge 547d937...

bors · 2021-09-12T02:35:54Z

☀️ Test successful - checks-actions
Approved by: michaelwoerister,petrochenkov
Pushing 547d937 to master...

rust-timer · 2021-09-12T04:15:04Z

Finished benchmarking commit (547d937): comparison url.

Summary: This change led to large relevant regressions 😿 in compiler performance.

Large regression in instruction counts (up to 2.1% on incr-unchanged builds of tuple-stress)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Mark-Simulacrum · 2021-10-03T01:03:01Z

About the perf regression: this PR implements an opt-in feature to reduce span-related recomputations for incr-comp. Measurements in #84762 show up to -25% instruction reduction when enabled. When disabled, the perf hit appears to be a consistent 1-2% instruction hit.

So seems justified and action on this PR not expected.

Aaron1011 reviewed Apr 20, 2021

View reviewed changes

compiler/rustc_span/src/lib.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

jyn514 added A-incr-comp Area: Incremental compilation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-compiletime Issue: Problems and improvements with respect to compile times. labels Apr 20, 2021

cjgillot force-pushed the resolve-span branch from 1992f11 to 58ee1ee Compare April 24, 2021 18:03

This comment has been minimized.

Sign in to view

cjgillot force-pushed the resolve-span branch from 58ee1ee to d861775 Compare April 24, 2021 18:28

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 24, 2021

This comment has been minimized.

Sign in to view

cjgillot force-pushed the resolve-span branch from d861775 to a39f526 Compare April 24, 2021 19:59

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 24, 2021

cjgillot force-pushed the resolve-span branch from a39f526 to 0dbf000 Compare April 25, 2021 12:07

cjgillot commented Apr 25, 2021

View reviewed changes

src/test/incremental/spans_significant_w_debuginfo.rs Outdated Show resolved Hide resolved

cjgillot commented Apr 25, 2021

View reviewed changes

src/test/incremental/spans_significant_w_panic.rs Outdated Show resolved Hide resolved

cjgillot commented Apr 25, 2021

View reviewed changes

src/test/incremental/change_symbol_export_status.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

cjgillot added 11 commits September 10, 2021 20:17

Keep def_spans collected by resolution.

06f7ca3

Keep a parent LocalDefId in SpanData.

00485e0

Encode spans relative to their parent.

e85ddeb

Track span dependency using a callback.

b19ae20

Rename decode to data_untracked.

940fa92

Add sanity check.

fb5ced0

We force the relative span's parent to be absolute. This avoids having to handle long dependency chains.

Add actual spans to the crate hash.

6f782c4

Now that we encode spans relative to the items, the item's own span is never actually hashed as part of the HIR. In consequence, we explicitly include it in the crate hash to avoid missing cross-crate invalidations.

Give spans their parent item during lowering.

f84856c

We only do this operation when incremental compilation is enabled. This avoids pessimizing the span handling for non-incremental compilation.

Remove some span tracking.

5e026ea

Record call_site parent for macros.

2e37ed8

Duplicate tests for incremental spans mode.

11a999e

This comment has been minimized.

Sign in to view

Rebase fallout.

7842b80

bors mentioned this pull request Sep 12, 2021

Simplify lazy DefPathHash decoding by using an on-disk hash table. #82183

Merged

cjgillot mentioned this pull request Sep 12, 2021

Encode span parent in the inlined representation. #88886

Closed

JonathanBrouwer mentioned this pull request Dec 26, 2025

Outer attribute spans on items are never lowered correctly #150400

Closed

Uh oh!

Conversation

cjgillot commented Apr 20, 2021 • edited by pnkfelix Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

petrochenkov commented Apr 20, 2021

Uh oh!

This comment has been minimized.

cjgillot commented Apr 24, 2021

Uh oh!

rust-timer commented Apr 24, 2021

Uh oh!

bors commented Apr 24, 2021

Uh oh!

This comment has been minimized.

cjgillot commented Apr 24, 2021

Uh oh!

bors commented Apr 24, 2021

Uh oh!

rust-timer commented Apr 24, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

cjgillot commented Apr 24, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cjgillot commented Apr 25, 2021

Uh oh!

rust-timer commented Apr 25, 2021

Uh oh!

bors commented Sep 2, 2021

Uh oh!

bors commented Sep 2, 2021

Uh oh!

rust-timer commented Sep 2, 2021

Uh oh!

rust-timer commented Sep 2, 2021

Uh oh!

petrochenkov commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelwoerister commented Sep 7, 2021

Uh oh!

This comment has been minimized.

bors commented Sep 8, 2021

Uh oh!

This comment has been minimized.

cjgillot commented Sep 11, 2021

Uh oh!

bors commented Sep 11, 2021

Uh oh!

bors commented Sep 11, 2021

Uh oh!

bors commented Sep 12, 2021

Uh oh!

rust-timer commented Sep 12, 2021

Uh oh!

Mark-Simulacrum commented Oct 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

cjgillot commented Apr 20, 2021 •

edited by pnkfelix

Loading

petrochenkov commented Sep 3, 2021 •

edited

Loading