[WIP] Stop ignoring `Span` field when hashing some `Ident`s #92210

Aaron1011 · 2021-12-22T21:32:52Z

Split out from #92204

rust-highfive · 2021-12-22T21:32:56Z

(rust-highfive has picked a reviewer for you, use r? to override)

This causes us to miss legitimate evaluatiosn (e.g. an upstream `ExpnId` no longer exists), leading to ICEs when decoding stale values from the incremental cache.

Aaron1011 · 2021-12-24T22:19:44Z

@bors try @rust-timer queue

rust-timer · 2021-12-24T22:19:45Z

Awaiting bors try build completion.

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

bors · 2021-12-24T22:19:52Z

⌛ Trying commit 7d18371 with merge 360798a02d08783253f065b05e6d2b0952dc3361...

bors · 2021-12-24T22:21:50Z

💔 Test failed - checks-actions

rust-log-analyzer · 2021-12-24T22:21:51Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Aaron1011 · 2021-12-24T22:31:56Z

@bors try

bors · 2021-12-24T22:32:03Z

⌛ Trying commit 7d18371 with merge 1d91d882ec1873c87b78b0a1b8631fb4bf1f2d78...

bors · 2021-12-25T00:06:37Z

☀️ Try build successful - checks-actions
Build commit: 1d91d882ec1873c87b78b0a1b8631fb4bf1f2d78 (1d91d882ec1873c87b78b0a1b8631fb4bf1f2d78)

rust-timer · 2021-12-25T00:06:39Z

Queued 1d91d882ec1873c87b78b0a1b8631fb4bf1f2d78 with parent aad4f10, future comparison URL.

rust-timer · 2021-12-25T02:08:50Z

Finished benchmarking commit (1d91d882ec1873c87b78b0a1b8631fb4bf1f2d78): comparison url.

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

Very large regression in instruction counts (up to 452.6% on incr-patched: println builds of webrender)

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 try 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

Aaron1011 · 2021-12-25T18:21:58Z

This is now finished, but has awful performance, and a separate fix for a caching issue. I've split out #92278 to fix the caching issue.

Since this PR will fix nightly incremental ICEs that several users have been reporting, we might want to merge this soon, and then work on winning back the performance lost by increased query invalidation.

petrochenkov · 2021-12-26T02:03:51Z

Blocked on #92278.

Aaron1011 · 2022-01-02T22:27:47Z

Now that #92203 has been merged, let's see what the latest performance looks like:

@bors try @rust-timer queue

rust-timer · 2022-01-02T22:27:48Z

Awaiting bors try build completion.

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

bors · 2022-01-02T22:27:55Z

⌛ Trying commit 7d18371 with merge fc922e0fe35a7aaa4bf5dbbe516ffd78dde591c2...

bors · 2022-01-02T23:59:42Z

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

rust-timer · 2022-01-02T23:59:43Z

Queued fc922e0fe35a7aaa4bf5dbbe516ffd78dde591c2 with parent 8f3238f, future comparison URL.

rust-timer · 2022-01-03T01:30:53Z

Finished benchmarking commit (fc922e0fe35a7aaa4bf5dbbe516ffd78dde591c2): comparison url.

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

Very large regression in instruction counts (up to 453.8% on incr-patched: println builds of webrender)

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 try 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

cjgillot · 2022-01-03T18:35:30Z

compiler/rustc_middle/src/ty/assoc.rs

@@ -43,7 +43,6 @@ impl AssocItemContainer {
 #[derive(Copy, Clone, Debug, PartialEq, HashStable, Eq, Hash)]
 pub struct AssocItem {
    pub def_id: DefId,
-    #[stable_hasher(project(name))]
    pub ident: Ident,


IIRC, this field is used for name resolution. So it really only needs the Symbol and the SyntaxContext. In case of diagnostics, the span can be recovered with the def_ident_span query.
Should we introduce a struct HygienicIdent { name: Symbol, ctxt: SyntaxContext } for this purpose? This could avoid part of the regression due to moved code.

I came to the same conclusion about the Span. I'm working on a branch locally where I've switched to just storing a Symbol, and using def_ident_span to reconstruct the Ident in the few cases where it's actually needed.

cjgillot · 2022-01-03T18:35:42Z

compiler/rustc_middle/src/ty/mod.rs

@@ -1502,7 +1502,6 @@ pub struct VariantDef {
    /// If this variant is a struct variant, then this is `None`.
    pub ctor_def_id: Option<DefId>,
    /// Variant or struct name.
-    #[stable_hasher(project(name))]
    pub ident: Ident,


Aaron1011 · 2022-01-12T16:41:59Z

I went with a better approach in #92533

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 22, 2021

rust-highfive assigned petrochenkov Dec 22, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 22, 2021

This comment has been minimized.

Sign in to view

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 23, 2021

cjgillot self-assigned this Dec 23, 2021

This was referenced Dec 23, 2021

Store a DefId instead of an AdtDef in AggregateKind::Adt #92203

Merged

ICE: panicked at 'index out of bounds: the len is 153 but the index is 167', compiler/rustc_query_impl/src/on_disk_cache.rs #92258

Closed

Aaron1011 added 5 commits December 24, 2021 12:53

Stop ignoring Span field when hashing some Idents

ae17c8a

This causes us to miss legitimate evaluatiosn (e.g. an upstream `ExpnId` no longer exists), leading to ICEs when decoding stale values from the incremental cache.

Adjust some tests

4ca8136

Fix hashing cache issue

12bab02

Adjust incremental tests again

3c7678b

Fmt and warnings

b50fd63

Aaron1011 force-pushed the hash-more-spans-only branch from e11ef37 to b50fd63 Compare December 24, 2021 21:49

This comment has been minimized.

Sign in to view

Ignore line length in test

7d18371

Aaron1011 mentioned this pull request Dec 24, 2021

HashStable caching does not take into account StableHashingContext settings #92266

Closed

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

rustbot added perf-regression Performance regression. 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 Dec 25, 2021

petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 25, 2021

petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 26, 2021

Aaron1011 mentioned this pull request Dec 26, 2021

ICE on nightly-2021-12-21 and later: change source file timestamp, get "failed to lookup SourceFile in new context" #92304

Closed

compiler-errors mentioned this pull request Dec 28, 2021

thread 'rustc' panicked at 'index out of bounds: the len is 90 but the index is 97', compiler/rustc_query_impl/src/on_disk_cache.rs:712:18 #92373

Closed

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

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 Jan 3, 2022

cjgillot reviewed Jan 3, 2022

View reviewed changes

petrochenkov mentioned this pull request Jan 4, 2022

Hash Ident spans in all HIR structures #92534

Merged

Eijebong mentioned this pull request Jan 12, 2022

ICE: bincode v2 alpha cannot be used with nightly rustc #92470

Open

Aaron1011 closed this Jan 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Stop ignoring `Span` field when hashing some `Ident`s #92210

[WIP] Stop ignoring `Span` field when hashing some `Ident`s #92210

Aaron1011 commented Dec 22, 2021

rust-highfive commented Dec 22, 2021

This comment has been minimized.

This comment has been minimized.

Aaron1011 commented Dec 24, 2021

rust-timer commented Dec 24, 2021

bors commented Dec 24, 2021

bors commented Dec 24, 2021

rust-log-analyzer commented Dec 24, 2021

Aaron1011 commented Dec 24, 2021

bors commented Dec 24, 2021

bors commented Dec 25, 2021

rust-timer commented Dec 25, 2021

rust-timer commented Dec 25, 2021

Aaron1011 commented Dec 25, 2021

petrochenkov commented Dec 26, 2021

Aaron1011 commented Jan 2, 2022

rust-timer commented Jan 2, 2022

bors commented Jan 2, 2022

bors commented Jan 2, 2022

rust-timer commented Jan 2, 2022

rust-timer commented Jan 3, 2022

cjgillot Jan 3, 2022

Aaron1011 Jan 3, 2022

cjgillot Jan 3, 2022

Aaron1011 commented Jan 12, 2022

[WIP] Stop ignoring Span field when hashing some Idents #92210

[WIP] Stop ignoring Span field when hashing some Idents #92210

Conversation

Aaron1011 commented Dec 22, 2021

rust-highfive commented Dec 22, 2021

This comment has been minimized.

This comment has been minimized.

Aaron1011 commented Dec 24, 2021

rust-timer commented Dec 24, 2021

bors commented Dec 24, 2021

bors commented Dec 24, 2021

rust-log-analyzer commented Dec 24, 2021

Aaron1011 commented Dec 24, 2021

bors commented Dec 24, 2021

bors commented Dec 25, 2021

rust-timer commented Dec 25, 2021

rust-timer commented Dec 25, 2021

Aaron1011 commented Dec 25, 2021

petrochenkov commented Dec 26, 2021

Aaron1011 commented Jan 2, 2022

rust-timer commented Jan 2, 2022

bors commented Jan 2, 2022

bors commented Jan 2, 2022

rust-timer commented Jan 2, 2022

rust-timer commented Jan 3, 2022

cjgillot Jan 3, 2022

Choose a reason for hiding this comment

Aaron1011 Jan 3, 2022

Choose a reason for hiding this comment

cjgillot Jan 3, 2022

Choose a reason for hiding this comment

Aaron1011 commented Jan 12, 2022

[WIP] Stop ignoring `Span` field when hashing some `Ident`s #92210

[WIP] Stop ignoring `Span` field when hashing some `Ident`s #92210