-
-
Notifications
You must be signed in to change notification settings - Fork 432
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
feat(ast_tools): pre-compile #[ast]
macro output
#5796
base: main
Are you sure you want to change the base?
feat(ast_tools): pre-compile #[ast]
macro output
#5796
Conversation
Your org has enabled the Graphite merge queue for merging into mainAdd the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @overlookmotel and the rest of your teammates on Graphite |
661db42
to
f80c7af
Compare
CodSpeed Performance ReportMerging #5796 will not alter performanceComparing Summary
|
e02fd02
to
17cd903
Compare
f80c7af
to
5fa7b5a
Compare
5fa7b5a
to
502fd10
Compare
502fd10
to
a40e8c7
Compare
a40e8c7
to
90746ab
Compare
/// TokenTree::Group(Group::new(Delimiter::Parenthesis, args)) | ||
/// ].into_iter()) | ||
/// ``` | ||
macro_rules! code { |
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 is super smart! I like what I'm seeing👍
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.
Thanks. But unfortunately it's not moving the needle on performance.
I suspect (but don't know) that creating TokenTree
s and TokenStream
s is inherently expensive due to how proc macros interface with the compiler. There may just be no way to make proc macros cheap.
Or maybe the #[ast]
macro just isn't that costly in the first place, so optimizing it is never going to pay dividends.
Thanks for taking a look. I'm going to pause on this though for now as am planning to spend this week focused on transformer. Must... not... get... distracted...
By the way, these 2 articles were the inspiration. They do a very good job of explaining how proc macros work internally. I always thought it involved serialization, but it turns out not:
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.
Or maybe the #[ast] macro just isn't that costly in the first place, so optimizing it is never going to pay dividends.
I think this is more likely; No matter how much proc macro overhead costs us, If we do enough work in a macro eventually caching it would be faster. I suspect we have a better chance of getting some juice out of this approach in #4112 However it might be a wild goose chase.
And thanks for the links, They seem really interesting especially the second part(gave them a cursory look), I'll give them a read.
Work in progress.
Generate code for
#[ast]
macro with anoxc_ast_tools
generator.oxc_ast_macros
crate now has no dependencies (nosyn
etc).The guts of it is the
code!
macro. This is the same asquote!
, except it evaluates to aTokenStream
of code to create theTokenStream
whichquote!
evaluates to.Unfortunately I'm not seeing an improvement in compile times, because other dependencies of
oxc_ast
also usesyn
, so it doesn't drop from the dependency graph.The generated code is 3000 lines, and could likely be shrunk down, which also could help.The generated code is now 600 lines, and now there is a 5% reduction in debug build time foroxc_ast
crate - but that's still a disappointingly small improvement.Am opening this PR not necessarily with the intent of merging it, but to discuss whether this is maybe a useful approach. @rzvxa would appreciate your feedback.
Note: This technique would allow
#[generate_derive]
to inject the derivedimpl
next to the type def, same as#[derive]
does, which would solve the problem of types/fields needing to bepub(crate)
.