Skip to content
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

Reorder arguments in quote_token and quote_token_spanned #210

Merged
merged 3 commits into from Mar 17, 2022

Conversation

lqd
Copy link
Contributor

@lqd lqd commented Mar 17, 2022

This PR implements the suggestion in #209 (review) to reorder the arguments to these macros, and avoid some parsing of idents during rule matching.

The tests succeed but I'm not familiar with quote's internals, and may have made a mistake, especially on the more complicated rules.

If it's correct, this would result in a small improvement:

Benchmark & Profile Scenario % Change Significance Factor ?
ctor-0.1.21 check incr-unchanged -3.00% 15.00x
pest_generator-2.1.3 check incr-unchanged -2.20% 11.02x
num-derive-0.3.3 check incr-unchanged -2.05% 10.25x
ctor-0.1.21 check full -1.88% 9.39x
mockall_derive-0.11.0 check incr-unchanged -1.82% 9.08x
ctor-0.1.21 check incr-full -1.64% 8.21x
scroll_derive-0.11.0 check incr-unchanged -1.56% 7.82x
pest_generator-2.1.3 check full -1.32% 6.59x
num-derive-0.3.3 check full -1.19% 5.97x
futures-macro-0.3.19 check incr-unchanged -1.19% 5.97x
pest_generator-2.1.3 check incr-full -1.14% 5.70x
num-derive-0.3.3 check incr-full -1.09% 5.47x
mockall_derive-0.11.0 check full -0.94% 4.72x
scroll_derive-0.11.0 check full -0.85% 4.26x
mockall_derive-0.11.0 check incr-full -0.78% 3.91x
scroll_derive-0.11.0 check incr-full -0.69% 3.44x
futures-macro-0.3.19 check incr-full 0.05% 0.27x
futures-macro-0.3.19 check full -0.04% 0.21x

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure you're changing every $crate::quote_token_spanned! call site. There are some that are missed in this PR. (Maybe also some $crate::quote_token!? I didn't look too carefully)

@nnethercote
Copy link
Contributor

It looks just how I expected, modulo the missed changes.

I was also going to try putting $tokens last for quote_token_with_context and quote_token_with_context_spanned, to see if that helped as well. (No point doing anything with the higher-up macros like quote_each_token and quote_tokens_with_context because they only have one rule, so there's no need to fail fast.)

I would also put a brief comment above quote_tokens!, something like "$tokens comes at the end so that failing rules will fail as quickly as possible. This helps reduce compile times for crates that use quote! heavily.`

@lqd
Copy link
Contributor Author

lqd commented Mar 17, 2022

Thanks to the both of you for the review, I'll make the changes asap.

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dtolnay dtolnay merged commit 5b7afe8 into dtolnay:master Mar 17, 2022
@lqd
Copy link
Contributor Author

lqd commented Mar 17, 2022

Oh you were super quick, I was completing the latest couple changes nick mentioned :)

I'll open another PR in a few minutes.

@lqd lqd deleted the reorder_arguments branch March 17, 2022 22:58
@lqd
Copy link
Contributor Author

lqd commented Mar 18, 2022

Testing on the same 6 crates, and in a micro-benchmark: the quote_token_with_context and quote_token_with_context_spanned reorderings don't seem to make much of a difference. The branch is here if anyone is interested, but it's probably no use to open a PR.

Repository owner locked and limited conversation to collaborators Feb 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants