Fully serialize AdtDef #91924

Aaron1011 · 2021-12-14T17:38:43Z

This avoids needing to invoke the adt_def query during
the decoding of another query's result.

Split out from #91919
See #91696 (comment)

rust-highfive · 2021-12-14T17:38:46Z

r? @matthewjasper

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

Aaron1011 · 2021-12-14T17:59:43Z

@bors try @rust-timer queue

rust-timer · 2021-12-14T17:59:44Z

Awaiting bors try build completion.

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

bors · 2021-12-14T17:59:51Z

⌛ Trying commit 7f7ed6bdec7b4413223ec7b615ca4150c0ed97a6 with merge d3a40c62879ad9441b6cb79f94c654d32481cd91...

bors · 2021-12-14T19:35:08Z

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

rust-timer · 2021-12-14T19:35:10Z

Queued d3a40c62879ad9441b6cb79f94c654d32481cd91 with parent 404c847, future comparison URL.

rust-timer · 2021-12-14T21:07:45Z

Finished benchmarking commit (d3a40c62879ad9441b6cb79f94c654d32481cd91): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

Very large improvement in instruction counts (up to -24.9% on incr-unchanged builds of issue-46449)
Very large regression in instruction counts (up to 15.7% on incr-full builds of webrender-wrench)

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-14T21:14:44Z

Surprisingly, this appears to be a net win for disk usage (at least on the few benchmarks I checked). The increase in the query cache size is less than the decrease in the the size of the dep graph (as there are thousands of fewer query executions being performed).

cjgillot

Should we mark adt_def with cache_on_disk_if { true } for it to benefit from this serialization?

cjgillot · 2021-12-15T18:28:38Z

compiler/rustc_middle/src/ty/adt.rs

@@ -117,7 +117,7 @@ impl PartialEq for AdtDef {
    // `AdtDef`s are always interned, and this is part of `TyS` equality.
    #[inline]
    fn eq(&self, other: &Self) -> bool {
-        ptr::eq(self, other)
+        self.did == other.did


Why can't we keep the pointer equality here?

Using pointer equality breaks compilation of stage1 libc: I get errors like:

error[E0308]: mismatched types --> /home/aaron/.cargo/registry/src/github.com-1ecc6299db9ec823/compiler_builtins-0.1.65/src/int/specialized_div_rem/mod.rs:125:12 | 125 | if let Some(quo) = duo.checked_div(div) { | ^^^^^^^^^ -------------------- this expression has type `Option<u64>` | | | expected enum `Option`, found a different enum `Option` | = note: expected enum `Option<u64>` found enum `Option<_>`

I believe we need a 'normal' PartialEq impl in order for interning to work properly, and we need interning because current code is relying on pointers to an AdtDef being unique (since they all currently get allocated through a query).

I believe we need a 'normal' PartialEq impl in order for interning to work properly

if you have the time, could you try a perf run which only removes the Hash and Eq impl and does not touch any queries? Would be great to test that before merging this PR

I opened #92040 for a perf run with just Hash and PartialEq changed

Looking at the perf results, it seems that the performance impact is acceptable.

This avoids needing to invoke the `adt_def` query during the decoding of another query's result.

Aaron1011 · 2021-12-15T22:01:45Z

@bors try @rust-timer queue

rust-timer · 2021-12-15T22:01:47Z

Awaiting bors try build completion.

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

bors · 2021-12-15T22:01:53Z

⌛ Trying commit 70fba90 with merge e89932a1d84ef4783a5137927a85447161a9fbb8...

bors · 2021-12-15T22:51:41Z

💔 Test failed - checks-actions

rust-timer · 2021-12-17T02:33:02Z

Finished benchmarking commit (017ff4233fce7f99092ad20a3e1be45bf3d8e08a): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

Very large improvement in instruction counts (up to -25.0% on incr-unchanged builds of issue-46449)
Very large regression in instruction counts (up to 15.8% on incr-full builds of webrender-wrench)

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-17T04:02:51Z

Caching the adt_def query on disk seems to have had essentially no effect.

Aaron1011 · 2021-12-18T14:57:10Z

A significant number of people have been hitting the incremental compilation bug that promoted this PR. I'd like to get this merged soon, so that we can unblock the fix in #91919

The only significant regression is webrender-wrench-check. Given that the actual wall-clock diff is very small (0.85 to 1.03 seconds), I think we should merge this.

michaelwoerister

This looks good to me. The regression in webrender is not ideal, but many other benchmarks are improved. The more important thing is that we unblock #91919 though.

michaelwoerister · 2021-12-20T09:06:21Z

compiler/rustc_middle/src/ty/adt.rs

@@ -117,7 +117,7 @@ impl PartialEq for AdtDef {
    // `AdtDef`s are always interned, and this is part of `TyS` equality.
    #[inline]
    fn eq(&self, other: &Self) -> bool {
-        ptr::eq(self, other)
+        self.did == other.did


Looking at the perf results, it seems that the performance impact is acceptable.

michaelwoerister · 2021-12-20T09:17:59Z

@bors r+

bors · 2021-12-20T09:18:01Z

📌 Commit 00ce6dc has been approved by michaelwoerister

bors · 2021-12-20T10:35:51Z

⌛ Testing commit 00ce6dc with merge 84f962a...

bors · 2021-12-20T13:41:14Z

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing 84f962a to master...

rust-timer · 2021-12-20T16:35:26Z

Finished benchmarking commit (84f962a): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

Very large improvement in instruction counts (up to -25.0% on incr-unchanged builds of issue-46449)
Very large regression in instruction counts (up to 15.8% on incr-full builds of webrender-wrench)

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

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

rust-highfive assigned matthewjasper Dec 14, 2021

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

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

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Dec 14, 2021

Aaron1011 mentioned this pull request Dec 14, 2021

Don't perform any new queries while reading a query result on disk #91919

Merged

cjgillot self-assigned this Dec 15, 2021

cjgillot reviewed Dec 15, 2021

View reviewed changes

Aaron1011 added 2 commits December 15, 2021 16:32

Fully serialize AdtDef

f5f9054

This avoids needing to invoke the `adt_def` query during the decoding of another query's result.

Cache adt_def query result on disk

70fba90

Aaron1011 force-pushed the serialize-adt-def branch from 7f7ed6b to 70fba90 Compare December 15, 2021 22:01

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

This comment has been minimized.

Sign in to view

bors 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 15, 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 Dec 17, 2021

Aaron1011 removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 19, 2021

michaelwoerister reviewed Dec 20, 2021

View reviewed changes

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 Dec 20, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 20, 2021

bors merged commit 84f962a into rust-lang:master Dec 20, 2021

rustbot added this to the 1.59.0 milestone Dec 20, 2021

bors mentioned this pull request Dec 20, 2021

Remove PartialOrd, Ord from DefId #90749

Closed

Aaron1011 mentioned this pull request Dec 21, 2021

Change AdtDef's PartialEq and Hash impls to use DefId #92040

Closed

Mark-Simulacrum mentioned this pull request Dec 21, 2021

failed to lookup SourceFile in new context #92163

Closed

Aaron1011 mentioned this pull request Dec 22, 2021

Spurious cache failure: 'no entry found for key', compiler/rustc_metadata/src/rmeta/decoder.rs:1660:13 #92192

Closed

This was referenced Jan 5, 2022

Compiler panicked unexpectedly on Option::unwrap() #92576

Closed

ICE: called Option::unwrap() on a None value', compiler\rustc_hir\src\definitions.rs:452:14 #90682

Closed

PatchMixolydic mentioned this pull request Jan 10, 2022

Panic when using an undeclared module rust-lang/rust-clippy#8256

Closed

Aaron1011 mentioned this pull request Jan 17, 2022

thread 'rustc' panicked at 'called Option::unwrap() on a None value', #92978

Closed

This was referenced Feb 26, 2023

ICE unwrap in compiler/rustc_hir/src/definitions.rs:452:14 #90715

Closed

Unexpected compiler panic after changing struct's name #92606

Closed

Cache-related ICE #92452

Closed

Rust compilation fails #93370

Closed

ICE when running cargo test #94028

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fully serialize AdtDef #91924

Fully serialize AdtDef #91924

Aaron1011 commented Dec 14, 2021 •

edited

rust-highfive commented Dec 14, 2021

Aaron1011 commented Dec 14, 2021

rust-timer commented Dec 14, 2021

bors commented Dec 14, 2021

bors commented Dec 14, 2021

rust-timer commented Dec 14, 2021

rust-timer commented Dec 14, 2021

Aaron1011 commented Dec 14, 2021

cjgillot left a comment

cjgillot Dec 15, 2021

Aaron1011 Dec 15, 2021

lcnr Dec 17, 2021

Aaron1011 Dec 18, 2021

michaelwoerister Dec 20, 2021

Aaron1011 commented Dec 15, 2021

rust-timer commented Dec 15, 2021

bors commented Dec 15, 2021

This comment has been minimized.

bors commented Dec 15, 2021

rust-timer commented Dec 17, 2021

Aaron1011 commented Dec 17, 2021

Aaron1011 commented Dec 18, 2021

michaelwoerister left a comment

michaelwoerister Dec 20, 2021

michaelwoerister commented Dec 20, 2021

bors commented Dec 20, 2021

bors commented Dec 20, 2021

bors commented Dec 20, 2021

rust-timer commented Dec 20, 2021

Fully serialize AdtDef #91924

Fully serialize AdtDef #91924

Conversation

Aaron1011 commented Dec 14, 2021 • edited

rust-highfive commented Dec 14, 2021

Aaron1011 commented Dec 14, 2021

rust-timer commented Dec 14, 2021

bors commented Dec 14, 2021

bors commented Dec 14, 2021

rust-timer commented Dec 14, 2021

rust-timer commented Dec 14, 2021

Aaron1011 commented Dec 14, 2021

cjgillot left a comment

Choose a reason for hiding this comment

cjgillot Dec 15, 2021

Choose a reason for hiding this comment

Aaron1011 Dec 15, 2021

Choose a reason for hiding this comment

lcnr Dec 17, 2021

Choose a reason for hiding this comment

Aaron1011 Dec 18, 2021

Choose a reason for hiding this comment

michaelwoerister Dec 20, 2021

Choose a reason for hiding this comment

Aaron1011 commented Dec 15, 2021

rust-timer commented Dec 15, 2021

bors commented Dec 15, 2021

This comment has been minimized.

bors commented Dec 15, 2021

rust-timer commented Dec 17, 2021

Aaron1011 commented Dec 17, 2021

Aaron1011 commented Dec 18, 2021

michaelwoerister left a comment

Choose a reason for hiding this comment

michaelwoerister Dec 20, 2021

Choose a reason for hiding this comment

michaelwoerister commented Dec 20, 2021

bors commented Dec 20, 2021

bors commented Dec 20, 2021

bors commented Dec 20, 2021

rust-timer commented Dec 20, 2021

Aaron1011 commented Dec 14, 2021 •

edited