search

dtolnay / quote

Rust quasi-quoting
Apache License 2.0
1.32k stars 90 forks source link

Reorder arguments in `quote_token` and `quote_token_spanned` #210

Closed lqd closed 2 years ago

lqd commented 2 years ago

This PR implements the suggestion in https://github.com/dtolnay/quote/pull/209#pullrequestreview-912775088 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
nnethercote commented 2 years ago

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 commented 2 years ago

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

lqd commented 2 years ago

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

I'll open another PR in a few minutes.

lqd commented 2 years ago

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.