make declarative macro expansion a part of query system #125356
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
rustbot has assigned @compiler-errors. Use |
rustbot
added
S-waiting-on-review
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
SparrowLii
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
The build was successful with some UI tests failed. I think this is a good start.
|
bjorn3
reviewed
May 22, 2024
bjorn3
reviewed
May 22, 2024
bjorn3
reviewed
May 22, 2024
petrochenkov
changed the title
|
UPD: This PR is doing a different thing than I initially thought.
|
|
The |
compiler/rustc_expand/src/expand.rs
Outdated
| @@ -387,6 +393,18 @@ pub struct MacroExpander<'a, 'b> { | |||
| monotonic: bool, // cf. `cx.monotonic_expander()` | |||
| } | |||
|
|
|||
| pub fn expand_legacy_bang<'tcx>( | |||
| tcx: TyCtxt<'tcx>, | |||
| key: (LocalExpnId, Span, LocalExpnId), | |||
There was a problem hiding this comment.
The first LocalExpnId is unique for every macro call, so you won't get any reuse on this key.
Or you want to get reuse only between different compilations, when incremental is enabled?
In that case you do need the query system, not just a cache in resolver.
There was a problem hiding this comment.
I would be interested to see if there are any matching ExpnId hashes and any reuse at all in the simplest case (empty macro, two compilations, no changes between them).
There was a problem hiding this comment.
Yes, my main target is in two different compilations with incremental enabled. I think each macro should be expected to be expanded only once in a single compilation.
I cleaned up the Span in the key. Incremental compilation theoretically should not rely on any Span.
Yea, I think we need to do more testing of the effect of this change on two different compilations(let the query results be saved in disk).
petrochenkov
added
S-waiting-on-author
|
The job Click to see the possible cause of the failure (guessed by this bot) |
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
May 25, 2024
cleanup dependence of `ExtCtxt` in transcribe when macro expansion part of rust-lang#125356 We can remove `transcribe`’s dependence on `ExtCtxt` to facilitate subsequent work (such as moving macro expansion into the incremental compilation system) r? `@petrochenkov` Thanks for the reviewing!
fmease
added a commit
to fmease/rust
that referenced
this pull request
May 26, 2024
cleanup dependence of `ExtCtxt` in transcribe when macro expansion part of rust-lang#125356 We can remove `transcribe`’s dependence on `ExtCtxt` to facilitate subsequent work (such as moving macro expansion into the incremental compilation system) r? ``@petrochenkov`` Thanks for the reviewing!
jhpratt
added a commit
to jhpratt/rust
that referenced
this pull request
May 26, 2024
cleanup dependence of `ExtCtxt` in transcribe when macro expansion part of rust-lang#125356 We can remove `transcribe`’s dependence on `ExtCtxt` to facilitate subsequent work (such as moving macro expansion into the incremental compilation system) r? `@petrochenkov` Thanks for the reviewing!
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
May 26, 2024
cleanup dependence of `ExtCtxt` in transcribe when macro expansion part of rust-lang#125356 We can remove `transcribe`’s dependence on `ExtCtxt` to facilitate subsequent work (such as moving macro expansion into the incremental compilation system) r? ``@petrochenkov`` Thanks for the reviewing!
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
May 26, 2024
cleanup dependence of `ExtCtxt` in transcribe when macro expansion part of rust-lang#125356 We can remove `transcribe`’s dependence on `ExtCtxt` to facilitate subsequent work (such as moving macro expansion into the incremental compilation system) r? ```@petrochenkov``` Thanks for the reviewing!
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
May 26, 2024
cleanup dependence of `ExtCtxt` in transcribe when macro expansion part of rust-lang#125356 We can remove `transcribe`’s dependence on `ExtCtxt` to facilitate subsequent work (such as moving macro expansion into the incremental compilation system) r? ````@petrochenkov```` Thanks for the reviewing!
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 26, 2024
cleanup dependence of `ExtCtxt` in transcribe when macro expansion part of rust-lang#125356 We can remove `transcribe`’s dependence on `ExtCtxt` to facilitate subsequent work (such as moving macro expansion into the incremental compilation system) r? `@petrochenkov` Thanks for the reviewing!
workingjubilee
added a commit
to workingjubilee/rustc
that referenced
this pull request
May 26, 2024
cleanup dependence of `ExtCtxt` in transcribe when macro expansion part of rust-lang#125356 We can remove `transcribe`’s dependence on `ExtCtxt` to facilitate subsequent work (such as moving macro expansion into the incremental compilation system) r? `@petrochenkov` Thanks for the reviewing!
jhpratt
added a commit
to jhpratt/rust
that referenced
this pull request
May 26, 2024
cleanup dependence of `ExtCtxt` in transcribe when macro expansion part of rust-lang#125356 We can remove `transcribe`’s dependence on `ExtCtxt` to facilitate subsequent work (such as moving macro expansion into the incremental compilation system) r? ``@petrochenkov`` Thanks for the reviewing!
oli-obk
reviewed
May 27, 2024
Comment on lines
+861
to
+866
| pub macro_map: RwLock< | ||
| FxHashMap< | ||
| LocalExpnId, | ||
| (TokenStream, Span, Lrc<dyn TcxMacroExpander + sync::DynSync + sync::DynSend>), | ||
| >, | ||
| >, |
There was a problem hiding this comment.
This should not be public, but instead have a set of functions that ensure it can only be added to. It's also not safe for incremental (if macro expansions adds entries here, and the macro expansion query is not invoked again, but the result loaded from cache, the entries here won't get re-added)
oli-obk
reviewed
May 27, 2024
| @@ -115,6 +118,12 @@ rustc_queries! { | |||
| desc { "triggering a delayed bug for testing incremental" } | |||
| } | |||
|
|
|||
| query expand_legacy_bang(key: (LocalExpnId, LocalExpnId)) -> Result<(&'tcx TokenStream, usize), CanRetry> { | |||
There was a problem hiding this comment.
these IDs are very easily going to change. Is there no better identifier? I guess the query is already eval_always, but it doesn't seem quite right. Intuitively I would have assumed some stable identifier for a macro and a TokenStream as input, which is that not possible?
There was a problem hiding this comment.
Thanks!
Do you mean using the macro's DefId and input TokenStream as identifier? I also think this is good, but TokenStream may contain NonTerminal, which is not hashable during the macro expansion stage. Unless we implement Hash for all ast stuffs.
There was a problem hiding this comment.
By the way, why are LocalExpnIds easier to change than DefIds?
There was a problem hiding this comment.
The StableHash impl on DefId doesn't actually hash the raw indices inside the DefId, but instead hashes the DefPathHash which is derived from the DefPath. The latter only changes when you rename items or their parent modules or when you have multiple items with the same name nested inside a single item. I believe the LocalExpnId contains that it is the Nth expansion in the current compilation session, which depends on both expansion order and all code that is expanded before the current macro expansion.
There was a problem hiding this comment.
I also think this is good, but
TokenStreammay containNonTerminal, which is not hashable during the macro expansion stage
I was unable to find NonTerminal. Also TokenTree implements HashStable, so it should be possible to stable-hash TokenStream, too, as it's just a wrapper around Lrc<Vec<TokenTree>>.
There was a problem hiding this comment.
FYI, nonterminals are currently being removed in #124141.
After that token streams will no longer contain pieces of AST.
There was a problem hiding this comment.
There's analogue of DefPathHash for expansion ids called ExpnHash.
It was introduced in #87044 and #86676 (you may be interested in reading those threads).
This hash is stable in the sense that it is suitable for working with multiple compilations, but it is probably less resistant to small changes in code than DefPathHash (#86676 (comment)).
Although it may be more resistant now than back in 2021 because spans going into the incremental system are item-relative now.
Getting performance improvements from incremental expansion reuse may require using some other more change-resistant hashing scheme.
There was a problem hiding this comment.
Although it may be more resistant now than back in 2021 because spans going into the incremental system are item-relative now.
Only starting from HIR, right? Macro expansion is before HIR lowering.
Getting performance improvements from incremental expansion reuse may require using some other more change-resistant hashing scheme.
An option is a stencil mechanism like in Cranelift. Basically you split both the macro expansion input into a stencil and stencil parameters. Any external reference which doesn't affect macro expansion (like spans assuming no unstable features to introspect spans are used) are stored in the stencil parameters and the stencil refers to them by an index into the stencil parameters. And after macro expansion you combine the expansion result and the stencil parameters again to get a regular TokenStream. Only the stencil needs to be hashed. If an external reference changes, the stencil parameters will change too, but the offset into the stencil parameters as stored in the stencil itself remains the same, so the cache will not be invalidated.
There was a problem hiding this comment.
Only starting from HIR, right?
Not anymore:
https://github.com/rust-lang/rust/blob/master/compiler/rustc_expand/src/expand.rs#L597-L604
There was a problem hiding this comment.
I'd rather use LocalExpnId here, or at least leave a fixme.
Making LocalExpnId as stable as DefId (or almost) is a long term goal, and the signature should use it when we reach that state.
GuillaumeGomez
added a commit
to GuillaumeGomez/rust
that referenced
this pull request
May 27, 2024
cleanup dependence of `ExtCtxt` in transcribe when macro expansion part of rust-lang#125356 We can remove `transcribe`’s dependence on `ExtCtxt` to facilitate subsequent work (such as moving macro expansion into the incremental compilation system) r? ```@petrochenkov``` Thanks for the reviewing!
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 27, 2024
Rollup merge of rust-lang#125530 - SparrowLii:expand2, r=petrochenkov cleanup dependence of `ExtCtxt` in transcribe when macro expansion part of rust-lang#125356 We can remove `transcribe`’s dependence on `ExtCtxt` to facilitate subsequent work (such as moving macro expansion into the incremental compilation system) r? ```@petrochenkov``` Thanks for the reviewing!
|
☔ The latest upstream changes (presumably #125611) made this pull request unmergeable. Please resolve the merge conflicts. |
9 tasks
|
Hi, wanted to ask if you could use any kind of help with this PR, don't want to take away any work of course! :) I'm looking to help enable proc-macro caching, and this looks like it's doing half the required work already, thanks a lot! 👍 |
|
Hey, I went ahead and did a rebase of these commits onto current master. I opened it as a PR to your fork: SparrowLii#1 Hope this helps! |
|
I got the itch to try figuring out what caused the remaining errors, which turned into a solution for them that should further/finish the changes from this PR. I hope that is ok! This is the new PR, which is based on the changes from here: #128605 |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 6, 2024
Make declarative macro expansion a part of query system (cont. of rust-lang#125356) This is a continuation of the effort to make declarative macro expansion a part of the query system from rust-lang#125356 by `@SparrowLii.` #### Description from that PR: > This is an attempt to make the macro expansion a part of incremental compilation. > > Processing of procedural macros is difficult since they may involve interactions with the external environment. Declaring macros is a good start. > > **It is not yet possible to test the effect of this PR on incremental compilation since the new query is declared as eval_always.** #### Status of this PR: * It is rebased against a much more recent `master` commit * It contains the original commits from rust-lang#125356 (in a rebased form) * It adds the missing implementation for retrying macro matching that provides (better) error diagnostics * This was done by refactoring `rustc_expand::mbe::diagnostics::failed_to_match_macro()` to only require a `ParseSess` instead of an `ExtCtxt`. Otherwise, `ExtCtxt` would need to be in the interface of `TcxMacroExpander`, which is not possible, because `ExtCtxt` is part of `rustc_expand`, which depends on the crate that contains `TcxMacroExpander`, `rustc_middle`, and thus would introduce a circular dependency. * This refactoring moved the retrying down into the `impl TcxMacroExpander for MacroRulesMacroExpander` (this is just a change compared to the original PR, otherwise not important to know). * This PR passes `./x test tests/ui`, which produced errors before that were all due to the missing implementation of retry macro matching. * This PR does **not** yet contain changes for the open discussions from rust-lang#125356. I wanted to fix the tests first before tackling them, and I also need to figure out what the best solutions for them are. I'd welcome any help/support with this, e.g., opening up a corresponding discussion on this PR with a summary/the final decided fix if available :) In general, I'm new to working on rustc, so would be thankful for a bit more background/explanations in discussions and comments :) Thanks! :) (tangentially relevant: rust-lang#99515)
|
@futile Thank you very much for your work. I can't spend much time on continuing the incremental compilation work for the time being due to some other work. Feel free to close #125356 if your work can replace it. (Notify me or @petrochenkov ) (I was waiting for #124141 to be finished. According to oli's opinion, using TokenStream as the query input parameter may be a better way.) |
|
I'll close this in favor of the more complete version (#128747) then. |
Nice to hear back from you, and thanks! :) Yeah, it seems like waiting for #124141 should definitely make this easier. My attempt in #128747 to use Anyway, thanks a lot for your work, you already did most of it, and I was able to learn a lot through it! :) |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an attempt to make the macro expansion a part of incremental compilation.
Processing of procedural macros is difficult since they may involve interactions with the external environment. Declaring macros is a good start.
It is not yet possible to test the effect of this PR on incremental compilation since the new query is declared as
eval_always.