-
Notifications
You must be signed in to change notification settings - Fork 13.3k
proc_macro: use crossbeam channels for the proc_macro cross-thread bridge #99123
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
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.
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.
@pnkfelix I'm running out of ideas... what if you move this to the end? (as in, maybe the order of the fields or their offsets, in this huge
struct
, is causing weird things)It could even be its presence - the flag could be removed and
exec_strategy
(the sole user of the flag, incompiler/rustc_expand/src/proc_macro.rs
) made to, idk, usestd::env::var
instead? (or just hardcode the choice, but that might cause changes in optimizations)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.
Wacky! I moved the field to the end of the struct, and that made the two methods with the two highest instruction counts (
parse_tt
andMacroRulesMacroExpander::expand
) each have lower instruction-counts (Ir) according tocachegrind
, but the overall instruction-count increased:MacroExpander
::expand
I don't really want to spend too much more trying to dissect this thing that is probably just an LLVM codegen oddity, but I will at least try eliminating the field entirely as you suggest.
(As an aside, the fact that this kind of field shuffling had such an enormous impact on
parse_tt
does make me wonder if there's some kind of low-hanging fruit embedded in the code there, e.g. maybe the code is accessing the session when it should be locally stashing an unchanging value? But isn't that the kind of thing you'd like to believe your compiler is doing for you automatically? 😆 )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.
I suspect that the code is hot, and also that register spilling is happening in a different way or inlining choices are a little different. I.e. the kind of thing that's hard to control, and even if you manage to get it in the faster state now a tiny change later on might make it go back to the slower state.