Experimental: Add Derive Proc-Macro Caching #129102

futile · 2024-08-14T17:42:44Z

On-Disk Caching For Derive Proc-Macro Invocations

This PR adds on-disk caching for derive proc-macro invocations using rustc's query system to speed up incremental compilation.

The implementation is (intentionally) a bit rough/incomplete, as I wanted to see whether this helps with performance before fully implementing it/RFCing etc.

I did some ad-hoc performance testing.

Rough, Preliminary Eval Results:

Using a version built through DEPLOY=1 src/ci/docker/run.sh dist-x86_64-linux (which I got from here).

Some Small Personal Project:

# with -Zthreads=0 as well
$ touch src/main.rs && cargo +dist check

Caused a re-check of 1 crate (the only one).

Result:

Configuration	Time (avg. ~5 runs)
Uncached	~0.54s
Cached	~0.54s

No visible difference.

Bevy:

$ touch crates/bevy_ecs/src/lib.rs && cargo +dist check

Caused a re-check of 29 crates.

Result:

Configuration	Time (avg. ~5 runs)
Uncached	~6.4s
Cached	~5.3s

Roughly 1s, or ~17% speedup.

Polkadot-Sdk:

Basically this script (not mine): https://github.com/coderemotedotdev/rustc-profiles/blob/d61ad38c496459d82e35d8bdb0a154fbb83de903/scripts/benchmark_incremental_builds_polkadot_sdk.sh

TL;DR: Two full cargo check runs to fill the incremental caches (for cached & uncached). Then 10 repetitions of touch $some_file && cargo +uncached check && cargo +cached check.

$ cargo update # `time` didn't build because compiler too new/dep too old
$ ./benchmark_incremental_builds_polkadot_sdk.sh # see above

Huge workspace with ~190 crates. Not sure how many were re-built/re-checkd on each invocation.

Result:

Configuration	Time (avg. 10 runs)
Uncached	99.4s
Cached	67.5s

Very visible speedup of 31.9s or ~32%.

-> Based on these results I think it makes sense to do a rustc-perf run and see what that reports.

Current Limitations/TODOs

I left some FIXME(pr-time)s in the code for things I wanted to bring up/draw attention to in this PR. Usually when I wasn't sure if I found a (good) solution or when I knew that there might be a better way to do something; See the diff for these.

High-Level Overview of What's Missing For "Real" Usage:

Add caching for Bang- and Attr-proc macros (currently only Derive).
- Not a big change, I just focused on derive-proc macros for now, since I felt like these should be most cacheable and are used very often in practice.
Allow marking specific macros as "do not cache" (currently only all-or-nothing).
- Extend the unstable option to support, e.g., -Z cache-derive-macros=some_pm_crate::some_derive_macro_fn for easy testing using the nightly compiler.
- After Testing: Add a #[proc_macro_cacheable] annotation to allow proc-macro authors to "opt-in" to caching (or sth. similar). Would probably need an RFC?
- Might make sense to try to combine this with Tracking Issue for proc_macro::{tracked_env, tracked_path} #99515, so that external dependencies can be picked up and be taken into account as well (Would maybe need a 2-tiered query for that, so we can save its dependencies after running the proc macro).
How to deal with (currently) un-hashable TokenStreams?
- From Cache declarative macro expansion on disk (for incremental comp.). Based on #128605 #128747 we know that some TokenStreams aren't hashable/stable-hashable/encodable(?) for now. We have to a) detect that, which might require catching a panic (for Hash::hash() I believe), or b) otherwise establishing TryHash etc. traits that are more failure resistant.

So, just since you were in the loop on the attempt to cache declarative macro expansions:

r? @petrochenkov

Please feel free to re-/unassign!

Finally: I hope this isn't too big a PR, I'll also show up in Zulip since I read that that is usually appreciated. Thanks a lot for taking a look! :)

(Kind of related/very similar approach, old declarative macro caching PR: #128747)

Kobzol · 2024-08-14T19:02:56Z

@bors try @rust-timer queue

…ng, r=<try> Experimental: Add Derive Proc-Macro Caching # On-Disk Caching For Derive Proc-Macro Invocations This PR adds on-disk caching for derive proc-macro invocations using rustc's query system to speed up incremental compilation. The implementation is (intentionally) a bit rough/incomplete, as I wanted to see whether this helps with performance before fully implementing it/RFCing etc. I did some ad-hoc performance testing. ## Rough, Preliminary Eval Results: Using a version built through `DEPLOY=1 src/ci/docker/run.sh dist-x86_64-linux` (which I got from [here](https://rustc-dev-guide.rust-lang.org/building/optimized-build.html#profile-guided-optimization)). ### [Some Small Personal Project](https://github.com/futile/ultra-game): ```console # with -Zthreads=0 as well $ touch src/main.rs && cargo +dist check ``` Caused a re-check of 1 crate (the only one). Result: | Configuration | Time (avg. ~5 runs) | |--------|--------| | Uncached | ~0.54s | | Cached | ~0.54s | No visible difference. ### [Bevy](https://github.com/bevyengine/bevy): ```console $ touch crates/bevy_ecs/src/lib.rs && cargo +dist check ``` Caused a re-check of 29 crates. Result: | Configuration | Time (avg. ~5 runs) | |--------|--------| | Uncached | ~6.4s | | Cached | ~5.3s | Roughly 1s, or ~17% speedup. ### [Polkadot-Sdk](https://github.com/paritytech/polkadot-sdk): Basically this script (not mine): https://github.com/coderemotedotdev/rustc-profiles/blob/d61ad38c496459d82e35d8bdb0a154fbb83de903/scripts/benchmark_incremental_builds_polkadot_sdk.sh TL;DR: Two full `cargo check` runs to fill the incremental caches (for cached & uncached). Then 10 repetitions of `touch $some_file && cargo +uncached check && cargo +cached check`. ```console $ cargo update # `time` didn't build because compiler too new/dep too old $ ./benchmark_incremental_builds_polkadot_sdk.sh # see above ``` _Huge_ workspace with ~190 crates. Not sure how many were re-built/re-checkd on each invocation. Result: | Configuration | Time (avg. 10 runs) | |--------|--------| | Uncached | 99.4s | | Cached | 67.5s | Very visible speedup of 31.9s or ~32%. --- **-> Based on these results I think it makes sense to do a rustc-perf run and see what that reports.** --- ## Current Limitations/TODOs I left some `FIXME(pr-time)`s in the code for things I wanted to bring up/draw attention to in this PR. Usually when I wasn't sure if I found a (good) solution or when I knew that there might be a better way to do something; See the diff for these. ### High-Level Overview of What's Missing For "Real" Usage: * [ ] Add caching for `Bang`- and `Attr`-proc macros (currently only `Derive`). * Not a big change, I just focused on `derive`-proc macros for now, since I felt like these should be most cacheable and are used very often in practice. * [ ] Allow marking specific macros as "do not cache" (currently only all-or-nothing). * Extend the unstable option to support, e.g., `-Z cache-derive-macros=some_pm_crate::some_derive_macro_fn` for easy testing using the nightly compiler. * After Testing: Add a `#[proc_macro_cacheable]` annotation to allow proc-macro authors to "opt-in" to caching (or sth. similar). Would probably need an RFC? * Might make sense to try to combine this with rust-lang#99515, so that external dependencies can be picked up and be taken into account as well. --- So, just since you were in the loop on the attempt to cache declarative macro expansions: r? `@petrochenkov` Please feel free to re-/unassign! Finally: I hope this isn't too big a PR, I'll also show up in Zulip since I read that that is usually appreciated. Thanks a lot for taking a look! :) (Kind of related/very similar approach, old declarative macro caching PR: rust-lang#128747)

bors · 2024-08-14T19:04:07Z

⌛ Trying commit d47fa70 with merge 6d8226b...

bors · 2024-08-14T20:59:08Z

☀️ Try build successful - checks-actions
Build commit: 6d8226b (6d8226bea18aa0e8df52c967e24efd7c5ee92169)

rust-timer · 2024-08-15T01:15:20Z

Finished benchmarking commit (6d8226b): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

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 may lead 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-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.6%, 0.6%]	1
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-2.8%	[-7.9%, -1.3%]	17
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	-2.6%	[-7.9%, 0.6%]	18

Max RSS (memory usage)

Results (primary -0.3%, secondary -1.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.4%	[1.4%, 1.4%]	1
Regressions ❌ (secondary)	3.2%	[3.2%, 3.2%]	1
Improvements ✅ (primary)	-1.1%	[-1.2%, -1.0%]	2
Improvements ✅ (secondary)	-2.5%	[-5.3%, -0.7%]	7
All ❌✅ (primary)	-0.3%	[-1.2%, 1.4%]	3

Cycles

Results (primary -3.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.9%	[-10.7%, -1.6%]	16
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.9%	[-10.7%, -1.6%]	16

Binary size

Results (primary -0.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.1%, -0.1%]	4

Bootstrap: 753.65s -> 756.621s (0.39%)
Artifact size: 341.43 MiB -> 341.67 MiB (0.07%)

futile · 2024-08-16T13:57:13Z

Ok, I think the performance results look pretty great!

Here are the results for instructions:

Instructions

Summary

	Range	Mean	Count
Regressions	0.20%, 0.60%	0.40%	2
Improvements	-7.87%, -0.24%	-2.68%	18
All	-7.87%, 0.60%	-2.37%	20

Primary benchmarks

Benchmark	Profile	Scenario	% Change	Significance Factor
cargo-0.60.0	check	incr-unchanged	-7.87%	86.97x
cargo-0.60.0	check	incr-patched: println	-7.04%	97.31x
cargo-0.60.0	debug	incr-unchanged	-3.95%	26.54x
cargo-0.60.0	opt	incr-unchanged	-3.76%	31.39x
cargo-0.60.0	debug	incr-patched: println	-3.62%	38.61x
diesel-1.4.8	check	incr-unchanged	-2.36%	34.49x
diesel-1.4.8	check	incr-patched: println	-2.29%	24.18x
webrender-2022	check	incr-unchanged	-2.24%	21.62x
webrender-2022	check	incr-patched: println	-2.07%	19.32x
diesel-1.4.8	debug	incr-unchanged	-1.98%	27.36x
diesel-1.4.8	debug	incr-patched: println	-1.94%	19.31x
diesel-1.4.8	opt	incr-unchanged	-1.83%	19.58x
diesel-1.4.8	opt	incr-patched: println	-1.76%	15.07x
webrender-2022	debug	incr-unchanged	-1.44%	10.68x
webrender-2022	debug	incr-patched: println	-1.36%	12.79x
webrender-2022	opt	incr-unchanged	-1.29%	11.78x
cargo-0.60.0	opt	incr-patched: println	-1.27%	6.98x
cargo-0.60.0	check	incr-full	0.60%	4.13x

Secondary benchmarks

Benchmark	Profile	Scenario	% Change	Significance Factor
tt-muncher	check	incr-full	-0.24%	4.14x
tt-muncher	doc	full	0.20%	4.12x

Conclusion

So overall, lots of incr-unchanged and incr-patched (all println) saw noticeable improvements (check, debug and opt), which is similar to what my own rough eval showed. At the same time, only cargo as check saw a regression of 0.60% for incr-full. That is really nice, because incr-full contains the overhead of caching, but not yet any speedup. So seeing that there is only one 'small' regression for incr-full means that incr-full isn't even slowed down noticeably for most of the benchmarks.

The secondary benchmark results look weird, because incr-full got faster, even though there should be some overhead, and doc as full got slower, even though non-incremental builds shouldn't be affected. Not really sure what's going on here.

Also, I think the benchmarks don't contain a lot of "big" projects (like bevy etc.), which might see the biggest speedup from this caching. cargo looks like one of the bigger benchmarks to me, and its check wall-time went from 1.18s -> 1.06s, so it wasn't even high to begin with (although I have no idea how beefy the machine is!). But that probably just means that benefits for large projects might be even nicer :)

Next Steps

Given these results, what's the opinion here? I would think it makes sense to review + clean up the implementation, extend it to all proc-macro types, and to add an unstable option that allows not caching certain proc-macros (e.g., -Zcache-proc-macros-skip=...). Then a next step could be getting this into nightly, and letting people try it out (it would be off by default, of course)?

Then after that, as mentioned before, it might need a new attribute like #[proc_macro_cacheable] to let proc-macro authors opt-into the caching (since we don't want to change the default case). Would this require an RFC for the new attribute at that point?

Anyway, it would be awesome if someone could take a look at/review the diff so far, and to also get some feedback on possible next steps. Thanks a lot already! :)

P.S.: Big kudos to this blog post which found these possible gains in the first place! Also to @SparrowLii, whose initial implementation for declarative macro caching provided me the much-needed basic structure for this implementation as well!

memoryruins · 2024-08-16T16:12:41Z

@futile

Then after that, as mentioned before, it might need a new attribute like #[proc_macro_cacheable] to let proc-macro authors opt-into the caching (since we don't want to change the default case). Would this require an RFC for the new attribute at that point?

As far as I've seen, the default case has been to assume that caching can happen (and cases such as stateful proc-macros have not been supported), even if some proc-macros haven't treated it this way. Examples issues of this are #44034 (comment) and #63804 (comment), and #44034 (comment) (and the following comments) notes the IDE case, where language servers such as rust-analyzer and RustRover may cache proc-macro expansions already.

Not a reference, but another example of how an IDE has treated them https://blog.jetbrains.com/rust/2022/12/05/what-every-rust-developer-should-know-about-macro-support-in-ides/#what-every-rust-macro-implementor-should-take-into-account

If you still want to write a procedural macro, avoid stateful computations and I/O. It’s never guaranteed when and how many times macro expansion is going to be invoked.

I'm not arguing for or against a flag/attribute, nor am I a reviewer, but saw this wonderful PR and thought to add some context I've seen to help make a decision. It's possible there have been further discussions elsewhere that I have missed.

Thank you for exploring an implementation of caching proc-macro expansions!

futile · 2024-08-16T20:53:02Z

As far as I've seen, the default case has been to assume that caching can happen (and cases such as stateful proc-macros have not been supported), even if some proc-macros haven't treated it this way. Examples issues of this are #44034 (comment) and #63804 (comment), and #44034 (comment) (and the following comments) notes the IDE case, where language servers such as rust-analyzer and RustRover may cache proc-macro expansions already.

Not a reference, but another example of how an IDE has treated them https://blog.jetbrains.com/rust/2022/12/05/what-every-rust-developer-should-know-about-macro-support-in-ides/#what-every-rust-macro-implementor-should-take-into-account

Thank you for bringing that up, I wasn't aware of it, definitely helps! :) The reason I wanted to not just "break" existing libraries is to not make life unnecessarily harder for libraries like sqlx, which rely on this behavior to a certain point. I think it makes sense to coordinate with #99515, which would give proc macro authors something clear to rely on, and would at the same time help to reduce the "fallout" from starting to cache proc macro invocations.

But this probably means that a new opt-in attribute/etc. will not be necessary, so that's really nice, thanks! :)

I'm not arguing for or against a flag/attribute, nor am I a reviewer, but saw this wonderful PR and thought to add some context I've seen to help make a decision. It's possible there have been further discussions elsewhere that I have missed.

Thank you for exploring an implementation of caching proc-macro expansions!

Thanks, very appreciated! :)

futile · 2024-08-19T15:46:55Z

Updated with some cleanup. Got rid of some unnecessary code, and split out a small change to error handling in the query system into #129271.

Otherwise same as before, ready for review :)

…-panic, r=michaelwoerister Prevent double panic in query system, improve diagnostics I stumbled upon a double-panic in the query system while working on something else (rust-lang#129102), which hid the real error cause for what I was debugging. This PR remedies that, so unwinding should be able to present more errors. It shouldn't really be relevant for code that doesn't ICE.

Rollup merge of rust-lang#129271 - futile:query-system/prevent-double-panic, r=michaelwoerister Prevent double panic in query system, improve diagnostics I stumbled upon a double-panic in the query system while working on something else (rust-lang#129102), which hid the real error cause for what I was debugging. This PR remedies that, so unwinding should be able to present more errors. It shouldn't really be relevant for code that doesn't ICE.

rustbot · 2024-08-20T17:04:59Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

tests/incremental/derive_macro_expansion/proc_macro_unchanged.rs

petrochenkov · 2024-08-20T17:18:12Z

tests/incremental/derive_macro_expansion/proc_macro_unchanged.rs

+// The proc-macro (for `Nothing`) prints a message to stderr when invoked,
+// and this message should only be present during the second invocation
+// (which has `cfail2` set via cfg).
+// FIXME(pr-time): Properly have the test check this, but how? UI-test that tests for `.stderr`?


Not sure why do you want to test stderr output, incremental tests have directives for testing invalidation.

Yep, that's what the "Properly have the test check this, but how?" was for, I simply didn't see how I can check this. There are directives, but I didn't figure out a way to make them work for this case. Because the directive needs to be attached to the proc macro output I think, but that was weird somehow. I will try again and see what exactly the problem was with that.

Tried again, but couldn't figure out something that works, i.e., that succeeds with proc-macro caching enabled, but fails with proc-macro caching disabled.

I suspect that the #[rustc_clean] attribute needs to be used instead of #[rustc_partition_codegened] because the incremental system eventually figures out that the second run of derive(Nothing) effectively produced the same data as the first one, so the codegen doesn't need to happen again.

Also, run-make tests (tests/run-make) allow to run the compiler twice and look at stderr.
There are even some examples of running the compiler in incremental mode there (search by "incremental").

tests/incremental/derive_macro_expansion/proc_macro_unchanged.rs

compiler/rustc_session/src/options.rs

compiler/rustc_expand/src/derive_macro_expansion.rs

compiler/rustc_expand/src/proc_macro.rs

compiler/rustc_expand/src/derive_macro_expansion.rs

petrochenkov · 2024-08-20T18:32:25Z

compiler/rustc_middle/src/query/mod.rs

@@ -103,6 +104,13 @@ pub use plumbing::{IntoQueryParam, TyCtxtAt, TyCtxtEnsure, TyCtxtEnsureWithValue
 // Queries marked with `fatal_cycle` do not need the latter implementation,
 // as they will raise an fatal error on query cycles instead.
 rustc_queries! {
+    query derive_macro_expansion(key: (LocalExpnId, Svh, &'tcx TokenStream)) -> Result<&'tcx TokenStream, ()> {
+        // eval_always


I suspect that eval_always may indeed be needed here because the query may access untracked data, but I'm not sure, this whole PR will need another review from some query system / incremental compilation expert.

Just so I understand correctly, eval_always would prevent caching completely, no? Or will it always execute the query, but, if the output stays the same, not mark dependent nodes as "require recomputation"?

Some relevant docs:

https://rustc-dev-guide.rust-lang.org/queries/incremental-compilation-in-detail.html#query-modifiers

https://rustc-dev-guide.rust-lang.org/queries/incremental-compilation-in-detail.html?highlight=eval_always#the-projection-query-pattern

Or will it always execute the query, but, if the output stays the same, not mark dependent nodes as "require recomputation"?

This sounds more reasonable.

On another hand, with eval_always the query will never try loading the on disk cache, which is the whole point of this PR.

petrochenkov · 2024-08-20T21:14:43Z

This will clearly need an opt-in opt out attribute (or better a modifier for #[proc_macro*] attributes), but it can be done in a separate PR.
I'm not sure if we should enable caching by default even on edition boundary with a migration lint, it may be too easy to get an incorrect build while not knowing or forgetting about the macro caching.

futile · 2024-08-24T23:07:13Z

Thank you for the review! I'm currently a bit busy, but hope I'll get to addressing all the feedback in the code soon! :)

This will clearly need an opt-in opt out attribute (or better a modifier for #[proc_macro*] attributes), but it can be done in a separate PR.

Yep, that was also my initial thought. I guess it might make sense to also ask on, e.g., internals.r-l.o, or on Zulip, to gather opinions on this? But I'll do that when we get that far.

Regarding the modifier: I scouted that out at first as well, but I think one or two of the proc-macro types already use modifiers (I'm thinking of #[proc_macro_bang(some_modifier)] etc.), and it felt like that might be confusing when mixed together.

I'm not sure if we should enable caching by default even on edition boundary with a migration lint, it may be too easy to get an incorrect build while not knowing or forgetting about the macro caching.

Note that, just as #129102 (comment) mentions, this is already the case. If a proc macro depends on some file/external state, but nothing else about the code has changed, cargo build will already not do anything, and thus also not execute the proc macro.

That doesn't mean I'm against being careful about adding caching for proc macros, on the contrary, I think it makes sense to spread information about this proactively before anything is merged. Just that so far these guarantees have strictly not been there either, also rust-analyzer is already not re-executing proc-macros all the time.

I think coordinating with #99515 makes a lot of sense, because that would add a way to properly track external dependencies on files and env-vars, which would actually give proc-macro authors a reliable option in general.

but I'm not sure, this whole PR will need another review from some query system / incremental compilation expert.

Would you have someone in mind? For now I'll fix this round of feedback first, so got a bit to do anyway. Also wanted to add caching for the other proc-macro types as well (does that make sense already?). Otherwise, what would be the correct way to find somebody? I can definitely ask on Zulip, if that is a good way to go about this.

Again, thanks for the review!

…ros`

futile · 2024-12-28T23:44:47Z

@rustbot ready

Happy holidays if you're celebrating! Took some time, please let me know if I missed any of the feedback :)

bors · 2025-02-06T14:37:07Z

☔ The latest upstream changes (presumably #136471) made this pull request unmergeable. Please resolve the merge conflicts.

petrochenkov · 2025-02-20T18:33:58Z

compiler/rustc_ast/src/tokenstream.rs

+
+impl Hash for TokenStream {
+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        Encodable::encode(self, &mut HashEncoder { hasher: state });


(Also the use of (Span)Encoder here is strange, I don't think it's supposed to be used like this.)

This comment is still relevant, why did you use the Encoder traits for implementing hashing?
Is this Hash implementation actually used? What happens if you put a panic into it?
Can it be implemented in terms of the already existing HashStable implementation for TokenStream?

petrochenkov · 2025-02-20T18:38:07Z

tests/incremental/derive_macro_expansion/proc_macro_unchanged.rs

+// The proc-macro (for `Nothing`) prints a message to stderr when invoked,
+// and this message should only be present during the second invocation
+// (which has `cfail2` set via cfg).
+// FIXME(pr-time): Properly have the test check this, but how? UI-test that tests for `.stderr`?


Also, run-make tests (tests/run-make) allow to run the compiler twice and look at stderr.
There are even some examples of running the compiler in incremental mode there (search by "incremental").

petrochenkov · 2025-02-20T18:49:06Z

compiler/rustc_middle/src/query/mod.rs

@@ -103,6 +104,13 @@ pub use plumbing::{IntoQueryParam, TyCtxtAt, TyCtxtEnsure, TyCtxtEnsureWithValue
 // Queries marked with `fatal_cycle` do not need the latter implementation,
 // as they will raise an fatal error on query cycles instead.
 rustc_queries! {
+    query derive_macro_expansion(key: (LocalExpnId, Svh, &'tcx TokenStream)) -> Result<&'tcx TokenStream, ()> {
+        // eval_always


On another hand, with eval_always the query will never try loading the on disk cache, which is the whole point of this PR.

petrochenkov · 2025-02-20T18:55:15Z

compiler/rustc_expand/src/proc_macro.rs

+            assert_eq!(invoc_expn_data.call_site, span);
+
+            // FIXME(pr-time): Is this the correct way to check for incremental compilation (as
+            // well as for `cache_proc_macros`)?


petrochenkov · 2025-02-20T19:03:33Z

compiler/rustc_expand/src/proc_macro.rs

+            // FIXME(pr-time): Just using the crate hash to notice when the proc-macro code has
+            // changed. How to *correctly* depend on exactly the macro definition?
+            // I.e., depending on the crate hash is just a HACK (and leaves garbage in the
+            // incremental compilation dir).


Ah, now I understand.
In the query key

&'tcx TokenStream checks whether the macro input changed

LocalExpnId checks whether the macro call context changed

and the proc macro crate's Svh checks whether the macro definition changed

petrochenkov · 2025-02-20T19:06:19Z

compiler/rustc_expand/src/proc_macro.rs

-                    });
-                    return ExpandResult::Ready(vec![]);
-                }
+        let res = ty::tls::with(|tcx| {


Wrapping into tls::with is also unnecessary if we are not in incremental + macro caching mode.

The tcx.arena.alloc(stream) part can also be omitted.

petrochenkov · 2025-02-20T19:10:04Z

compiler/rustc_expand/src/proc_macro.rs

+                // FIXME(pr-time): Just using the crate hash to notice when the proc-macro code has
+                // changed. How to *correctly* depend on exactly the macro definition?
+                // I.e., depending on the crate hash is just a HACK, and ideally the dependency would be
+                // more narrow.


I don't think there's any more precise way.
Proc macro is an executable code, it can run arbitrary functions from the macro crate itself or other crates, we don't have any system for incremental tracking of runtime dependencies, so we can only assume that if anything in the crate changes it can change the macro logic as well.

petrochenkov · 2025-02-20T19:11:26Z

compiler/rustc_expand/src/proc_macro.rs

-                    });
-                    return ExpandResult::Ready(vec![]);
+        let res = ty::tls::with(|tcx| {
+            // FIXME(pr-time): without flattened some (weird) tests fail, but no idea if it's correct/enough


If flattening is not enabled then some hashing/encoding/decoding function panics due to encountering an AST piece in the token stream?

petrochenkov · 2025-02-20T19:14:50Z

@rustbot ready

Happy holidays if you're celebrating! Took some time, please let me know if I missed any of the feedback :)

Sorry for the long delay, it weren't exactly holidays, but now I should be back to reviewing.

petrochenkov · 2025-02-21T14:08:45Z

compiler/rustc_expand/src/proc_macro.rs

@@ -31,9 +37,9 @@ impl<T> pm::bridge::server::MessagePipe<T> for MessagePipe<T> {
    }
 }

-fn exec_strategy(ecx: &ExtCtxt<'_>) -> impl pm::bridge::server::ExecutionStrategy {
+pub fn exec_strategy(sess: &Session) -> impl pm::bridge::server::ExecutionStrategy {


Suggested change

pub fn exec_strategy(sess: &Session) -> impl pm::bridge::server::ExecutionStrategy {

fn exec_strategy(sess: &Session) -> impl pm::bridge::server::ExecutionStrategy {

petrochenkov · 2025-02-21T14:11:06Z

compiler/rustc_expand/src/proc_macro.rs

+    );
+
+    let proc_macro_backtrace = ecx.ecfg.proc_macro_backtrace;
+    let strategy = crate::proc_macro::exec_strategy(tcx.sess);


Suggested change

let strategy = crate::proc_macro::exec_strategy(tcx.sess);

let strategy = exec_strategy(tcx.sess);

petrochenkov · 2025-02-21T14:11:15Z

compiler/rustc_expand/src/proc_macro.rs

+
+    let proc_macro_backtrace = ecx.ecfg.proc_macro_backtrace;
+    let strategy = crate::proc_macro::exec_strategy(tcx.sess);
+    let server = crate::proc_macro_server::Rustc::new(ecx);


Suggested change

let server = crate::proc_macro_server::Rustc::new(ecx);

let server = proc_macro_server::Rustc::new(ecx);

petrochenkov · 2025-02-21T14:13:52Z

compiler/rustc_expand/src/proc_macro.rs

+
+/// Sets `context` as the new current `CONTEXT` for the duration of the function `f`.
+#[inline]
+pub(crate) fn enter_context<'a, F, R>(context: (&mut ExtCtxt<'a>, CLIENT), f: F) -> R


Suggested change

pub(crate) fn enter_context<'a, F, R>(context: (&mut ExtCtxt<'a>, CLIENT), f: F) -> R

fn enter_context<'a, F, R>(context: (&mut ExtCtxt<'a>, CLIENT), f: F) -> R

petrochenkov · 2025-02-21T14:19:27Z

compiler/rustc_expand/src/proc_macro.rs

+    with_context(|(ecx, client)| expand_derive_macro(tcx, invoc_id, input, ecx, *client))
+}
+
+type CLIENT = pm::bridge::client::Client<pm::TokenStream, pm::TokenStream>;


Suggested change

type CLIENT = pm::bridge::client::Client<pm::TokenStream, pm::TokenStream>;

type DeriveClient = pm::bridge::client::Client<pm::TokenStream, pm::TokenStream>;

This alias can be used in a couple more places in this file.

petrochenkov · 2025-02-21T14:27:14Z

compiler/rustc_expand/src/proc_macro.rs

-                    });
-                    return ExpandResult::Ready(vec![]);
-                }
+        let res = ty::tls::with(|tcx| {


The tcx.arena.alloc(stream) part can also be omitted.

petrochenkov · 2025-02-21T14:49:16Z

compiler/rustc_expand/src/proc_macro.rs

+// based on rust/compiler/rustc_middle/src/ty/context/tls.rs
+thread_local! {
+    /// A thread local variable that stores a pointer to the current `CONTEXT`.
+    static TLV: Cell<(*mut (), Option<CLIENT>)> = const { Cell::new((ptr::null_mut(), None)) };


This looks more like scoped_thread_local!, because we put a value into it outside the query call, and then retrieve it immediately inside the call and don't need it anymore.
Maybe it will help to avoid some manual type erasure.

rustbot assigned petrochenkov Aug 14, 2024

This comment has been minimized.

Sign in to view

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

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Aug 15, 2024

futile force-pushed the experimental/proc-macro-caching branch 2 times, most recently from 64058f4 to abb1ba2 Compare August 19, 2024 15:09

futile mentioned this pull request Aug 19, 2024

Prevent double panic in query system, improve diagnostics #129271

Merged

petrochenkov marked this pull request as ready for review August 20, 2024 17:04

petrochenkov reviewed Aug 20, 2024

View reviewed changes

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 Aug 20, 2024

futile added 11 commits December 24, 2024 15:20

wip: try also hashing query output

d781c4b

prepare for PR (tidy should pass, caching=yes by default for rustc-perf)

a81aaed

wip: Undo unnecessary refactoring

91c0d1b

rebase + fix unreachable pub-items, inline some CONTEXT-things

8521154

chore: Run x fmt after rebase, should probably squash.

17dcadf

fix: comment + unused warning

35beb09

style: move import to correct place

382c5d0

chore: remove old comment

b71c42d

don't flatten proc macro output, let's see how it goes

a359e3c

only retrieve expn_data once

f768697

fix: Ensure incremental compilation is also enabled

bb3bddf

futile force-pushed the experimental/proc-macro-caching branch from e7cbfd7 to bb3bddf Compare December 24, 2024 14:27

futile added 5 commits December 28, 2024 16:44

refactor!: Rename cache-all-derive-macros option to `cache-proc-mac…

77944c9

…ros`

nit: remove comment

de0fad9

refactor: Require ecx less

d6c2527

refactor: Only call invoc_id.expn_data() once, inline some res

00cff94

refactor: Don't enter context if not caching (don't need it then)

abcc4a2

futile force-pushed the experimental/proc-macro-caching branch from 441c4ab to abcc4a2 Compare December 28, 2024 16:55

refactor: Move derive_macro_expansion.rs to end of proc_macro.rs

e93beac

This comment has been minimized.

Sign in to view

fix: Make non-context expansion actually work

cd82e38

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

petrochenkov reviewed Feb 20, 2025

View reviewed changes

petrochenkov reviewed Feb 21, 2025

View reviewed changes

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 Feb 21, 2025

	pub fn exec_strategy(sess: &Session) -> impl pm::bridge::server::ExecutionStrategy {
	fn exec_strategy(sess: &Session) -> impl pm::bridge::server::ExecutionStrategy {

	let strategy = crate::proc_macro::exec_strategy(tcx.sess);
	let strategy = exec_strategy(tcx.sess);

	let server = crate::proc_macro_server::Rustc::new(ecx);
	let server = proc_macro_server::Rustc::new(ecx);

	pub(crate) fn enter_context<'a, F, R>(context: (&mut ExtCtxt<'a>, CLIENT), f: F) -> R
	fn enter_context<'a, F, R>(context: (&mut ExtCtxt<'a>, CLIENT), f: F) -> R

	type CLIENT = pm::bridge::client::Client<pm::TokenStream, pm::TokenStream>;
	type DeriveClient = pm::bridge::client::Client<pm::TokenStream, pm::TokenStream>;

Experimental: Add Derive Proc-Macro Caching #129102

Are you sure you want to change the base?

Experimental: Add Derive Proc-Macro Caching #129102

Conversation

futile commented Aug 14, 2024 • edited Loading

On-Disk Caching For Derive Proc-Macro Invocations

Rough, Preliminary Eval Results:

Some Small Personal Project:

Bevy:

Polkadot-Sdk:

Current Limitations/TODOs

High-Level Overview of What's Missing For "Real" Usage:

Kobzol commented Aug 14, 2024

This comment has been minimized.

bors commented Aug 14, 2024

bors commented Aug 14, 2024

This comment has been minimized.

rust-timer commented Aug 15, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

futile commented Aug 16, 2024 • edited Loading

Instructions

Summary

Primary benchmarks

Secondary benchmarks

Conclusion

Next Steps

memoryruins commented Aug 16, 2024

futile commented Aug 16, 2024

futile commented Aug 19, 2024

rustbot commented Aug 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Aug 20, 2024

futile commented Aug 24, 2024

This comment has been minimized.

futile commented Dec 28, 2024

bors commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Feb 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

futile commented Aug 14, 2024 •

edited

Loading

futile commented Aug 16, 2024 •

edited

Loading

petrochenkov Feb 20, 2025 •

edited

Loading