-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Cache derive proc macro expansion with incremental query #145354
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
base: master
Are you sure you want to change the base?
Conversation
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Cache derive proc macro expansion with incremental query
/// Must be called while the `enter` function is active. | ||
fn with<F, R>(f: F) -> R | ||
where | ||
F: for<'a, 'b> FnOnce(&'b mut ExtCtxt<'a>, DeriveClient) -> R, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ExtCtxt contains a bunch of things that would need to be tracked by the query system for sound caching. And the rest could either be retrieved from the tcx
or be created from scratch to avoid having to use a thread local to bypass the query system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was discussed a bit in #129102 (comment) and a few comments right above it.
This comment has been minimized.
This comment has been minimized.
💔 Test for 21e746d failed: CI. Failed jobs:
|
This comment has been minimized.
This comment has been minimized.
@bors try |
This comment has been minimized.
This comment has been minimized.
Cache derive proc macro expansion with incremental query
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
💔 Test for b712f62 failed: CI. Failed jobs:
|
let macro_def_id = invoc_expn_data.macro_def_id.unwrap(); | ||
let proc_macro_crate_hash = tcx.crate_hash(macro_def_id.krate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be done from inside the query, to force a dependency on crate_hash
, not as part of the key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, my knowledge of the query infrastructure is quite limited 😅 How would that work? The key would be just LocalExpnId
and &TokenStream
, and then inside the query I would just call tcx.crate_hash
and throw away the results? 🤔
I looked into the benchmark failure on diesel, seems like I hold queries wrong somehow :) Any suggestions on what could be the case? Maybe the derived
|
Ok I did a bit of debugging, and found a fun fact: |
Ok I found it, it's this. @nnethercote Do you think it's possible to make |
Technically, removing that I'm not actually sure where else |
Could you please specify what do you mean by "macro rules matching logic"? I tried removing I also thought that another alternative would be to wrap |
This comment has been minimized.
This comment has been minimized.
Cache derive proc macro expansion with incremental query
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ochenkov Derive `PartialEq` for `InvisibleOrigin` For rust-lang#145354, we need `PartialEq` for `TokenStream` to "just work". However, due to the special comparison implementation that was used for `InvisibleOrigin`, this wasn't the case. So I derived `PartialEq` for `InvisibleOrigin`, and used the previous special comparison logic only on the single place where it was actually required. r? `@petrochenkov`
…ochenkov Derive `PartialEq` for `InvisibleOrigin` For rust-lang#145354, we need `PartialEq` for `TokenStream` to "just work". However, due to the special comparison implementation that was used for `InvisibleOrigin`, this wasn't the case. So I derived `PartialEq` for `InvisibleOrigin`, and used the previous special comparison logic only on the single place where it was actually required. r? ``@petrochenkov``
Finished benchmarking commit (ece5ebe): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 0.9%, secondary 2.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -4.1%, secondary -3.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 465.026s -> 466.363s (0.29%) |
…ochenkov Derive `PartialEq` for `InvisibleOrigin` For rust-lang#145354, we need `PartialEq` for `TokenStream` to "just work". However, due to the special comparison implementation that was used for `InvisibleOrigin`, this wasn't the case. So I derived `PartialEq` for `InvisibleOrigin`, and used the previous special comparison logic only on the single place where it was actually required. r? ```@petrochenkov```
…ochenkov Derive `PartialEq` for `InvisibleOrigin` For rust-lang#145354, we need `PartialEq` for `TokenStream` to "just work". However, due to the special comparison implementation that was used for `InvisibleOrigin`, this wasn't the case. So I derived `PartialEq` for `InvisibleOrigin`, and used the previous special comparison logic only on the single place where it was actually required. r? ````@petrochenkov````
…ochenkov Derive `PartialEq` for `InvisibleOrigin` For rust-lang#145354, we need `PartialEq` for `TokenStream` to "just work". However, due to the special comparison implementation that was used for `InvisibleOrigin`, this wasn't the case. So I derived `PartialEq` for `InvisibleOrigin`, and used the previous special comparison logic only on the single place where it was actually required. r? `````@petrochenkov`````
…ochenkov Derive `PartialEq` for `InvisibleOrigin` For rust-lang#145354, we need `PartialEq` for `TokenStream` to "just work". However, due to the special comparison implementation that was used for `InvisibleOrigin`, this wasn't the case. So I derived `PartialEq` for `InvisibleOrigin`, and used the previous special comparison logic only on the single place where it was actually required. r? ``````@petrochenkov``````
…ochenkov Derive `PartialEq` for `InvisibleOrigin` For rust-lang#145354, we need `PartialEq` for `TokenStream` to "just work". However, due to the special comparison implementation that was used for `InvisibleOrigin`, this wasn't the case. So I derived `PartialEq` for `InvisibleOrigin`, and used the previous special comparison logic only on the single place where it was actually required. r? ```````@petrochenkov```````
…ochenkov Derive `PartialEq` for `InvisibleOrigin` For rust-lang#145354, we need `PartialEq` for `TokenStream` to "just work". However, due to the special comparison implementation that was used for `InvisibleOrigin`, this wasn't the case. So I derived `PartialEq` for `InvisibleOrigin`, and used the previous special comparison logic only on the single place where it was actually required. r? ````````@petrochenkov````````
…ochenkov Derive `PartialEq` for `InvisibleOrigin` For rust-lang#145354, we need `PartialEq` for `TokenStream` to "just work". However, due to the special comparison implementation that was used for `InvisibleOrigin`, this wasn't the case. So I derived `PartialEq` for `InvisibleOrigin`, and used the previous special comparison logic only on the single place where it was actually required. r? `````````@petrochenkov`````````
Rollup merge of #146090 - Kobzol:invisible-origin-eq, r=petrochenkov Derive `PartialEq` for `InvisibleOrigin` For #145354, we need `PartialEq` for `TokenStream` to "just work". However, due to the special comparison implementation that was used for `InvisibleOrigin`, this wasn't the case. So I derived `PartialEq` for `InvisibleOrigin`, and used the previous special comparison logic only on the single place where it was actually required. r? `````````@petrochenkov`````````
8540970
to
356d426
Compare
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
The job Click to see the possible cause of the failure (guessed by this bot)
|
#146090 has been merged. I'm still unsure how to properly test this though (as per the PR description). |
Derive `PartialEq` for `InvisibleOrigin` For rust-lang/rust#145354, we need `PartialEq` for `TokenStream` to "just work". However, due to the special comparison implementation that was used for `InvisibleOrigin`, this wasn't the case. So I derived `PartialEq` for `InvisibleOrigin`, and used the previous special comparison logic only on the single place where it was actually required. r? `````````@petrochenkov`````````
This is a revival of #129102, originally implemented by @futile. Since it looks like they are not active currently, I'd like to push this work forward.
The first commit is squashed and rebased work from the original PR, with author attribution to futile. The rest of the commits are some additional comments that I created mostly for myself to understand what happens here. I also did some cleanups based on Vadim's review comments on the original PR, plus I refactored the TLS access a bit using
scoped_tls
.The biggest issue, as usually, are tests... I tried using
#[rustc_clean(..., loaded_from_disk = "derive_macro_expansion")]
, but the problem is that since this query cannot recover the original key from its hash, and thus its fingerprintstyle isFingerprintStyle::Opaque
, this crashes when I try to useloaded_from_disk
. Any suggestions from someone who actually understands the query system would be welcome 😅To answer one review question from the original PR: the
Hash
implementation forTokenStream
is indeed called, and it is needed since the stream forms a part of a query key. Not sure about theEncoder
Hash
implementation though.The last commit is WIP to enable the functionality by default for rustc-perf, I'll do another perf. run here.
TODO: document the new unstable flag
r? @petrochenkov