Encode spans relative to the enclosing item #84373

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

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

…ster,petrochenkov Encode spans relative to the enclosing item The aim of this PR is to avoid recomputing queries when code is moved without modification. MCP at rust-lang/compiler-team#443 This is achieved by : 1. storing the HIR owner LocalDefId information inside the span; 2. encoding and decoding spans relative to the enclosing item in the incremental on-disk cache; 3. 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.

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.

…ust#88677

…ster,petrochenkov Encode spans relative to the enclosing item The aim of this PR is to avoid recomputing queries when code is moved without modification. MCP at rust-lang/compiler-team#443 This is achieved by : 1. storing the HIR owner LocalDefId information inside the span; 2. encoding and decoding spans relative to the enclosing item in the incremental on-disk cache; 3. 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.

…nkov Encode spans relative to the enclosing item -- enable on nightly Follow-up to rust-lang#84373 with the flag `-Zincremental-relative-spans` set by default. This PR seeks to remove one of the main shortcomings of incremental: the handling of spans. Changing the contents of a function may require redoing part of the compilation process for another function in another file because of span information is changed. Within one file: all the spans in HIR change, so typechecking had to be re-done. Between files: spans of associated types/consts/functions change, so type-based resolution needs to be re-done (hygiene information is stored in the span). The flag `-Zincremental-relative-spans` encodes local spans relative to the span of an item, stored inside the `source_span` query. Trap: stashed diagnostics are referenced by the "raw" span, so stealing them requires to remove the span's parent. In order to avoid too much traffic in the span interner, span encoding uses the `ctxt_or_tag` field to encode: - the parent when the `SyntaxContext` is 0; - the `SyntaxContext` when the parent is `None`. Even with this, the PR creates a lot of traffic to the Span interner, when a Span has both a LocalDefId parent and a non-root SyntaxContext. They appear in lowering, when we add a parent to all spans, including those which come from macros, and during inlining when we mark inlined spans. The last commit changes how queries of `LocalDefId` manage their cache. I can put this in a separate PR if required. Possible future directions: - validate that all spans are marked in HIR validation; - mark macro-expanded spans relative to the def-site and not the use-site.

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 Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Rebase fallout.

7842b80

cjgillot force-pushed the resolve-span branch from 890e381 to 7842b80 Compare September 11, 2021 15:59

bors added S-waiting-on-bors 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 Sep 11, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 12, 2021

bors merged commit 547d937 into rust-lang:master Sep 12, 2021

rustbot added this to the 1.57.0 milestone Sep 12, 2021

bors mentioned this pull request Sep 12, 2021

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

Merged

cjgillot deleted the resolve-span branch September 12, 2021 11:39

cjgillot mentioned this pull request Sep 12, 2021

Encode span parent in the inlined representation. #88886

Closed

Mark-Simulacrum added the perf-regression-triaged The performance regression has been triaged. label Oct 3, 2021

JohnTitor added a commit to JohnTitor/rust-semverver that referenced this pull request Oct 15, 2021

Rustup to rust-lang/rust#84373, rust-lang/rust#87867, and rust-lang/r…

7dbee3c

…ust#88677

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encode spans relative to the enclosing item #84373

Encode spans relative to the enclosing item #84373

cjgillot commented Apr 20, 2021 •

edited by pnkfelix

Loading

This comment has been minimized.

petrochenkov commented Apr 20, 2021

This comment has been minimized.

cjgillot commented Apr 24, 2021

rust-timer commented Apr 24, 2021

bors commented Apr 24, 2021

This comment has been minimized.

cjgillot commented Apr 24, 2021

bors commented Apr 24, 2021

rust-timer commented Apr 24, 2021

This comment has been minimized.

This comment has been minimized.

cjgillot commented Apr 24, 2021

cjgillot commented Apr 25, 2021

rust-timer commented Apr 25, 2021

This comment has been minimized.

cjgillot commented Sep 11, 2021

bors commented Sep 11, 2021

bors commented Sep 11, 2021

bors commented Sep 12, 2021

rust-timer commented Sep 12, 2021

Mark-Simulacrum commented Oct 3, 2021

Encode spans relative to the enclosing item #84373

Encode spans relative to the enclosing item #84373

Conversation

cjgillot commented Apr 20, 2021 • edited by pnkfelix Loading

This comment has been minimized.

petrochenkov commented Apr 20, 2021

This comment has been minimized.

cjgillot commented Apr 24, 2021

rust-timer commented Apr 24, 2021

bors commented Apr 24, 2021

This comment has been minimized.

cjgillot commented Apr 24, 2021

bors commented Apr 24, 2021

rust-timer commented Apr 24, 2021

This comment has been minimized.

This comment has been minimized.

cjgillot commented Apr 24, 2021

cjgillot commented Apr 25, 2021

rust-timer commented Apr 25, 2021

This comment has been minimized.

cjgillot commented Sep 11, 2021

bors commented Sep 11, 2021

bors commented Sep 11, 2021

bors commented Sep 12, 2021

rust-timer commented Sep 12, 2021

Mark-Simulacrum commented Oct 3, 2021

cjgillot commented Apr 20, 2021 •

edited by pnkfelix

Loading