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

Make expansions stable for incr. comp. #86676

Merged
merged 12 commits into from
Jul 17, 2021
Merged

Conversation

cjgillot
Copy link
Contributor

This PR aims to make expansions stable for incr. comp. by using the same architecture as definitions:

  • the interned identifier ExpnId contains a CrateNum and a crate-local id;
  • bidirectional maps ExpnHash <-> ExpnId are setup;
  • incr. comp. on-disk cache saves and reconstructs expansions using their ExpnHash.

I tried to use as many LocalExpnId as I could in the resolver code, but I may have missed a few opportunities.

All this will allow to use an ExpnId as a query key, and to force this query without recomputing caller queries. For instance, this will be used to implement #85999.

r? @petrochenkov

@rust-highfive
Copy link
Collaborator

Some changes occurred in src/tools/rustfmt.

cc @calebcartwright

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 27, 2021
@cjgillot
Copy link
Contributor 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 Jun 27, 2021
@bors
Copy link
Contributor

bors commented Jun 27, 2021

⌛ Trying commit 48374baa140a0358612b4c1be2f7d8716620519d with merge 10d3de9d420a0b3963c1d7182283d29ff22412d5...

@bors
Copy link
Contributor

bors commented Jun 27, 2021

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

@rust-timer
Copy link
Collaborator

Queued 10d3de9d420a0b3963c1d7182283d29ff22412d5 with parent 9cdb2d3, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (10d3de9d420a0b3963c1d7182283d29ff22412d5): comparison url.

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

  • Moderate improvement in instruction counts (up to -1.7% on full builds of deeply-nested-async-check)

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. 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 -perf-regression

@rustbot
Copy link
Collaborator

rustbot commented Jun 28, 2021

Error: Label perf-regression can only be set by Rust team members

Please let @rust-lang/release know if you're having trouble with this bot.

@petrochenkov petrochenkov removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 28, 2021
@petrochenkov
Copy link
Contributor

(I'll get to this next weekend, most likely.)

@bors

This comment has been minimized.

@bors

This comment has been minimized.

@petrochenkov
Copy link
Contributor

Phew, I did a first pass over this PR just to understand what's going on, and it took a ~whole working day because the PR does a lot, it would be better to break it into parts if possible.

@petrochenkov
Copy link
Contributor

petrochenkov commented Jul 9, 2021

The fundamental difference between hashes used for definitions and expansions right now is that in case of definitions we a hashing something that is "resistant to small changes" (a def path), but in case of expansions we are just accumulating the whole ExpnData into the hash.
So if we, for example, move the location of a macro call slightly, its expansion hash will change as well.
But it doesn't really affects the infra (all the ExpnHash <-> ExpnId maps, etc), so we'll be able to change what exactly goes into the hash after landing this PR, that's good.

@petrochenkov
Copy link
Contributor

General comments:

  • I'd prefer to see a newtype a la DefIndex for local indices instead of a raw u32s.
  • Commits like e3abfd1 need comments, otherwise it's not clear why we are introducing duplication / copypaste for no apparent reason.
  • It would be good to avoid back and forth like changing ExpnId to LocalExpnId in one commit and then changing it back in some later commit.
  • Style nits: please use impl Trait in the first commit touching code in which it's usable; please don't use Self outside of generics, it almost always makes readability worse.

I doubt that it's possible to extract some "pure refactoring" parts from this PR, because the whole PR is kind of a gradual refactoring, but if it's possible, then it would be great.

@petrochenkov 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 Jul 9, 2021
@cjgillot
Copy link
Contributor Author

Phew, I did a first pass over this PR just to understand what's going on, and it took a ~whole working day because the PR does a lot, it would be better to break it into parts if possible.

Sorry for the lack of explanations.

The fundamental difference between hashes used for definitions and expansions right now is that in case of definitions we a hashing something that is "resistant to small changes" (a def path), but in case of expansions we are just accumulating the whole ExpnData into the hash.
So if we, for example, move the location of a macro call slightly, its expansion hash will change as well.
But it doesn't really affects the infra (all the ExpnHash <-> ExpnId maps, etc), so we'll be able to change what exactly goes into the hash after landing this PR, that's good.

I agree with this concern. I am trying to design an extension to #84373 which makes spans stable enough to resolve it.

General comments:

  • I'd prefer to see a newtype a la DefIndex for local indices instead of a raw u32s.
  • Commits like e3abfd1 need comments, otherwise it's not clear why we are introducing duplication / copypaste for no apparent reason.
  • It would be good to avoid back and forth like changing ExpnId to LocalExpnId in one commit and then changing it back in some later commit.
  • Style nits: please use impl Trait in the first commit touching code in which it's usable; please don't use Self outside of generics, it almost always makes readability worse.

I added a few commits and the current ones to avoid such back-and-forth. I could not find which functions you are referring to with impl Trait.

I doubt that it's possible to extract some "pure refactoring" parts from this PR, because the whole PR is kind of a gradual refactoring, but if it's possible, then it would be great.

I extracted #87044 if it helps.

@petrochenkov
Copy link
Contributor

I could not find which functions you are referring to with impl Trait.

fn decode_expn_id and all similar functions taking impl Fn(Mut,Once) callbacks as arguments.

@petrochenkov
Copy link
Contributor

One remaining question about duplicate tables - #86676 (comment).
Although I now see that the local table's size is used for assigning fresh local IDs at least.

@cjgillot
Copy link
Contributor Author

One remaining question about duplicate tables - #86676 (comment).
Although I now see that the local table's size is used for assigning fresh local IDs at least.

I have two reasons for keeping separate foreign tables:

  • they are expected to be sparse, where the local table is full;
  • they should eventually be removed, to fetch the information directly from the metadata decoder like DefIds do.

If these are satisfactory, I will add them as comments.

@petrochenkov
Copy link
Contributor

r=me after squashing all the review stuff into main commits.

@cjgillot
Copy link
Contributor Author

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Jul 17, 2021

📌 Commit b35ceee has been approved by petrochenkov

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 17, 2021
@bors
Copy link
Contributor

bors commented Jul 17, 2021

⌛ Testing commit b35ceee with merge 68511b5...

@bors
Copy link
Contributor

bors commented Jul 17, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants