Encode spans relative to the enclosing item -- enable on nightly by cjgillot · Pull Request #84762 · rust-lang/rust

cjgillot · 2021-04-30T18:52:38Z

Follow-up to #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.

cjgillot · 2021-04-30T19:19:25Z

@bors try @rust-timer queue

rust-timer · 2021-04-30T19:19:26Z

Awaiting bors try build completion.

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

bors · 2021-04-30T19:19:34Z

⌛ Trying commit 0cadd9de4986fb7044d22b242bf514ff674d9384 with merge 3c172eeb011b95ff8df485a4979d875ae919ce24...

bors · 2021-04-30T20:00:43Z

☀️ Try build successful - checks-actions
Build commit: 3c172eeb011b95ff8df485a4979d875ae919ce24 (3c172eeb011b95ff8df485a4979d875ae919ce24)

rust-timer · 2021-04-30T20:00:45Z

Queued 3c172eeb011b95ff8df485a4979d875ae919ce24 with parent 8fef006, future comparison URL.

rust-timer · 2021-04-30T22:18:18Z

Finished benchmarking try commit (3c172eeb011b95ff8df485a4979d875ae919ce24): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

cjgillot · 2021-05-02T21:26:12Z

@bors try @rust-timer queue

rust-timer · 2021-05-02T21:26:14Z

Awaiting bors try build completion.

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

bors · 2021-05-02T21:26:21Z

⌛ Trying commit 8d7131dfdeb2f5d52207e2acb8566d1e906d6af8 with merge 0355bb6a9f6aca12079750fb694c0353bf2533b2...

bors · 2021-05-02T22:12:20Z

☀️ Try build successful - checks-actions
Build commit: 0355bb6a9f6aca12079750fb694c0353bf2533b2 (0355bb6a9f6aca12079750fb694c0353bf2533b2)

rust-timer · 2021-05-02T22:12:22Z

Queued 0355bb6a9f6aca12079750fb694c0353bf2533b2 with parent e10cbc3, future comparison URL.

rust-timer · 2021-05-03T07:20:11Z

Finished benchmarking try commit (0355bb6a9f6aca12079750fb694c0353bf2533b2): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

rfcbot · 2022-11-25T14:08:53Z

michaelwoerister · 2022-11-28T16:21:02Z

Regarding performance, I think this should be enabled.

But I'm a bit worried about the potential for subtle bugs here. Spans + incremental just has a track record of introducing those. Is there a way to get more real-world testing for the feature before it's enabled by default? Maybe enable it by default just on nightly for a couple of cycles?

cjgillot · 2022-12-02T16:20:46Z

@michaelwoerister I agree. I can enable this on nightly only, and then by default in 1.70.0.

michaelwoerister · 2022-12-05T09:54:24Z

Sounds good, @cjgillot 🙂

rfcbot · 2022-12-08T10:14:57Z

🔔 This is now entering its final comment period, as per the review above. 🔔

apiraino · 2022-12-15T10:36:37Z

@petrochenkov I had removed S-waiting-on-team on purpose last week after T-compiler meeting. Makes sense?

petrochenkov · 2022-12-15T10:40:33Z

@apiraino

Makes sense?

Not at all.
There should always be some "waiting on" status label, and in this case waiting-on-team is the most appropriate label because the FCP week is still a time for potential comments from the team.

pnkfelix · 2022-12-15T15:37:03Z

@apiraino

Makes sense?

Not at all. There should always be some "waiting on" status label, and in this case waiting-on-team is the most appropriate label because the FCP week is still a time for potential comments from the team.

I agree there should always be some "waiting on" status label.

In this case, it would have been reasonable to apply S-waiting-on-fcp (rather than S-waiting-on-team), since that is specifically for PRs that have entered FCP and are going through the ten day final comment period.

(And also because all but two of the team had indeed signed off already, so S-waiting-on-fcp is more accurate reflection of what we are waiting on, in my opinion.)

petrochenkov · 2022-12-15T19:31:12Z

In this case, it would have been reasonable to apply S-waiting-on-fcp

Ah, okay, I haven't seen this label before.

rfcbot · 2022-12-18T10:33:09Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

Using that options basically changes all stable hashes we may compute. Adding/removing as UNTRACKED it makes everything ICE (unstable fingerprint everywhere). As TRACKED, it can still do its job without ICEing.

The diagnostics are replayed at the correct place anyway.

petrochenkov · 2023-01-02T11:13:25Z

@bors r+

bors · 2023-01-02T11:13:27Z

📌 Commit 7b6ead2 has been approved by petrochenkov

It is now in the queue for this repository.

bors · 2023-01-02T13:10:19Z

⌛ Testing commit 7b6ead2 with merge fb9dfa8...

bors · 2023-01-02T16:04:25Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing fb9dfa8 to master...

rust-timer · 2023-01-02T17:30:58Z

Finished benchmarking commit (fb9dfa8): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

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
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.7%	[0.2%, 5.0%]	108
Regressions ❌ (secondary)	1.8%	[0.2%, 7.6%]	90
Improvements ✅ (primary)	-20.0%	[-68.9%, -0.6%]	25
Improvements ✅ (secondary)	-2.7%	[-4.8%, -1.5%]	11
All ❌✅ (primary)	-2.4%	[-68.9%, 5.0%]	133

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.7%	[0.7%, 7.9%]	60
Regressions ❌ (secondary)	5.2%	[1.9%, 8.0%]	11
Improvements ✅ (primary)	-9.4%	[-32.6%, -1.5%]	17
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-32.6%, 7.9%]	77

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.0%	[1.1%, 6.4%]	38
Regressions ❌ (secondary)	3.2%	[1.3%, 4.4%]	22
Improvements ✅ (primary)	-19.0%	[-68.6%, -1.9%]	29
Improvements ✅ (secondary)	-5.7%	[-13.0%, -2.7%]	15
All ❌✅ (primary)	-6.5%	[-68.6%, 6.4%]	67

rylev · 2023-01-03T17:15:56Z

There was an FCP on whether the performance regression in some benchmarks were worth it which passed.

@rustbot label perf-regression-triaged

dtolnay · 2023-01-07T18:24:45Z

This had what seems to be an unintended effect on JSON diagnostics: #106571.

This comment has been minimized.

Sign in to view

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

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 30, 2021

cjgillot force-pushed the resolve-span-opt branch from 0cadd9d to 8d7131d Compare May 2, 2021 21:22

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

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 3, 2021

cjgillot force-pushed the resolve-span-opt branch 2 times, most recently from e6448db to 8fd8ff9 Compare May 8, 2021 07:46

This comment has been minimized.

Sign in to view

cjgillot mentioned this pull request May 12, 2021

Encode spans relative to the enclosing item #84373

Merged

petrochenkov self-assigned this May 12, 2021

petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 12, 2021

cjgillot force-pushed the resolve-span-opt branch 2 times, most recently from e5b631e to 8bb0a62 Compare May 31, 2021 20:03

cjgillot added 2 commits December 25, 2022 16:41

Encode span parent in the inlined representation.

6d42636

Mark incremental-ignore-spans as TRACKED.

44972b2

Using that options basically changes all stable hashes we may compute. Adding/removing as UNTRACKED it makes everything ICE (unstable fingerprint everywhere). As TRACKED, it can still do its job without ICEing.

This comment was marked as resolved.

Sign in to view

cjgillot added 6 commits December 25, 2022 18:48

Use absolute spans when trying to steal an AST diagnostic.

66e0316

Enable relative span hashing.

65f342d

Only enable relative span hashing on nightly.

40c8165

Ignore span references from diagnostics.

4719cb3

The diagnostics are replayed at the correct place anyway.

Remove references to removed option from tests.

5b9360b

Explain disabled span hashing.

7b6ead2

bors mentioned this pull request Jan 2, 2023

Refactoring report_method_error #105752

Merged

dtolnay mentioned this pull request Jan 7, 2023

Regression: duplicate messages appear in --error-format=json #106571

Open

cjgillot mentioned this pull request Sep 7, 2023

Enable incremental-relative-spans by default. #115656

Merged

nnethercote mentioned this pull request Jan 10, 2024

Cleanup things in and around Diagnostic #119763

Merged

Uh oh!

Conversation

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

Uh oh!

Uh oh!

This comment has been minimized.

cjgillot commented Apr 30, 2021

Uh oh!

rust-timer commented Apr 30, 2021

Uh oh!

bors commented Apr 30, 2021

Uh oh!

bors commented Apr 30, 2021

Uh oh!

rust-timer commented Apr 30, 2021

Uh oh!

rust-timer commented Apr 30, 2021

Uh oh!

cjgillot commented May 2, 2021

Uh oh!

rust-timer commented May 2, 2021

Uh oh!

bors commented May 2, 2021

Uh oh!

bors commented May 2, 2021

Uh oh!

rust-timer commented May 2, 2021

Uh oh!

rust-timer commented May 3, 2021

Uh oh!

This comment has been minimized.

rfcbot commented Nov 25, 2022 • edited by lcnr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelwoerister commented Nov 28, 2022

Uh oh!

cjgillot commented Dec 2, 2022

Uh oh!

michaelwoerister commented Dec 5, 2022

Uh oh!

rfcbot commented Dec 8, 2022

Uh oh!

apiraino commented Dec 15, 2022

Uh oh!

petrochenkov commented Dec 15, 2022

Uh oh!

pnkfelix commented Dec 15, 2022

Uh oh!

petrochenkov commented Dec 15, 2022

Uh oh!

rfcbot commented Dec 18, 2022

Uh oh!

This comment was marked as resolved.

petrochenkov commented Jan 2, 2023

Uh oh!

bors commented Jan 2, 2023

Uh oh!

bors commented Jan 2, 2023

Uh oh!

bors commented Jan 2, 2023

Uh oh!

rust-timer commented Jan 2, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Uh oh!

rylev commented Jan 3, 2023

Uh oh!

dtolnay commented Jan 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!

cjgillot commented Apr 30, 2021 •

edited

Loading

rfcbot commented Nov 25, 2022 •

edited by lcnr

Loading