Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Store a Symbol instead of an Ident in VariantDef/FieldDef #92533

Merged
merged 1 commit into from
Jan 12, 2022

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Jan 3, 2022

The field is also renamed from ident to name. In most cases,
we don't actually need the Span. A new ident method is added
to VariantDef and FieldDef, which constructs the full Ident
using tcx.def_ident_span(). This method is used in the cases
where we actually need an Ident.

This makes incremental compilation properly track changes
to the Span, without all of the invalidations caused by storing
a Span directly via an Ident.

@rust-highfive
Copy link
Collaborator

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 3, 2022
@rust-highfive
Copy link
Collaborator

r? @matthewjasper

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 3, 2022
@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 3, 2022
@bors
Copy link
Contributor

bors commented Jan 3, 2022

⌛ Trying commit b7e5ddf59cf8e5c979da2087676b54758f3aadbc with merge 7ba15b7f514570c7f7491ce79be1890d314aa5b1...

@rust-log-analyzer

This comment has been minimized.

@cjgillot cjgillot self-assigned this Jan 3, 2022
@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 3, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Aaron1011
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Jan 3, 2022

⌛ Trying commit c9f2899c677383aa1c5c722a4b24b71ea950801a with merge c1f5740893c9cdd25535dac455ea84154d41ef11...

@petrochenkov petrochenkov self-assigned this Jan 4, 2022
@bors
Copy link
Contributor

bors commented Jan 4, 2022

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

@rust-timer
Copy link
Collaborator

Queued c1f5740893c9cdd25535dac455ea84154d41ef11 with parent ddabe07, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c1f5740893c9cdd25535dac455ea84154d41ef11): comparison url.

Summary: This change led to large relevant improvements 🎉 in compiler performance.

  • Large improvement in instruction counts (up to -5.8% on incr-unchanged builds of issue-46449)

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.

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 4, 2022
@Aaron1011
Copy link
Member Author

The perf improvements appear to be coming entirely from performing fewer Span encodings/decodings. This suggests that we might be able to get some performance wins by further optimizing Span encoding/decoding.

compiler/rustc_typeck/src/astconv/mod.rs Outdated Show resolved Hide resolved
Aaron1011 added a commit to Aaron1011/rust that referenced this pull request Jan 19, 2022
This is the same idea as rust-lang#92533, but for `AssocItem` instead
of `VariantDef`/`FieldDef`.

With this change, we no longer have any uses of
`#[stable_hasher(project(...))]`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 25, 2022
Store a `Symbol` instead of an `Ident` in `AssocItem`

This is the same idea as rust-lang#92533, but for `AssocItem` instead
of `VariantDef`/`FieldDef`.

With this change, we no longer have any uses of
`#[stable_hasher(project(...))]`
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jan 27, 2022
This is the same idea as rust-lang#92533, but for `AssocItem` instead
of `VariantDef`/`FieldDef`.

With this change, we no longer have any uses of
`#[stable_hasher(project(...))]`
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jan 27, 2022
Store a `Symbol` instead of an `Ident` in `AssocItem`

This is the same idea as rust-lang#92533, but for `AssocItem` instead
of `VariantDef`/`FieldDef`.

With this change, we no longer have any uses of
`#[stable_hasher(project(...))]`
@Aaron1011 Aaron1011 deleted the variant-symbol branch January 31, 2022 03:33
@ehuss
Copy link
Contributor

ehuss commented Feb 8, 2022

@Aaron1011 Due to the large number of reports of people getting an ICE that seems to be fixed by this, would it make sense to nominate this for a beta backport to 1.59?

@Aaron1011
Copy link
Member Author

Aaron1011 commented Feb 8, 2022

While I don't think backporting this will cause any issues, the diff is somewhat large, and might require adjusting if beta has additional uses of field.ident. Also, backporting this alone might not help much, since some of the ICEs are probably actualy fixed by #93095

@petrochenkov
Copy link
Contributor

petrochenkov commented Feb 9, 2022

Also, backporting this alone might not help much, since some of the ICEs are probably actualy fixed by #93095

And some of them are still not fixed because I still see ICEs like this every day on master when building rustc.
That's a mistake, stage1 rustc is still built by the old bootstrap compiler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-incr-comp Area: Incremental compilation merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.