Cache pretty-print/retokenize result to avoid compile time blowup #79338
Conversation
|
r? @oli-obk (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
Fixes rust-lang#79242 If a `macro_rules!` recursively builds up a nested nonterminal (passing it to a proc-macro at each step), we will end up repeatedly pretty-printing/retokenizing the same nonterminals. Unfortunately, the 'probable equality' check we do has a non-trivial cost, which leads to a blowup in compilation time. As a workaround, we cache the result of the 'probable equality' check, which eliminates the compilation time blowup for the linked issue. This commit only touches a single file (other than adding tests), so it should be easy to backport. The proper solution is to remove the pretty-print/retokenize hack entirely. However, this will almost certainly break a large number of crates that were relying on hygiene bugs created by using the reparsed `TokenStream`. As a result, we will definitely not want to backport such a change.
Aaron1011
force-pushed
the
fix/token-reparse-cache
branch
from
4d6bad4 to
6e466ef
Compare
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 6e466ef with merge 569ac7301f28de845062cbd2af59b659283a4223... |
|
☀️ Try build successful - checks-actions |
|
Queued 569ac7301f28de845062cbd2af59b659283a4223 with parent 40cf721, future comparison URL. |
|
Finished benchmarking try commit (569ac7301f28de845062cbd2af59b659283a4223): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
|
@bors r+ |
|
📌 Commit 6e466ef has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
…k, r=petrochenkov Replace pretty-print/compare/retokenize hack with targeted workarounds Based on rust-lang#78296 cc rust-lang#43081 The 'pretty-print/compare/retokenize' hack is used to try to avoid passing an outdated `TokenStream` to a proc-macro when the underlying AST is modified in some way (e.g. cfg-stripping before derives). Unfortunately, retokenizing throws away spans (including hygiene information), which causes issues of its own. Every improvement to the accuracy of the pretty-print/retokenize comparison has resulted in non-trivial ecosystem breakage due to hygiene changes. In extreme cases, users deliberately wrote unhygienic `macro_rules!` macros (likely because they did not realize that the compiler's behavior was a bug). Additionaly, the comparison between the original and pretty-printed/retoknized token streams comes at a non-trivial runtime cost, as shown by rust-lang#79338 This PR removes the pretty-print/compare/retokenize logic from `nt_to_tokenstream`. We only discard the original `TokenStream` under two circumstances: * Inner attributes are used (detected by examining the AST) * `cfg`/`cfg_attr` processing modifies the AST. This is detected by making the visitor update a flag when it performs a modification, instead of trying to detect the modification after-the-fact. Note that a 'matching' `cfg` (e.g. `#[cfg(not(FALSE)]`) does not actually get removed from the AST, allowing us to preserve the original `TokenStream`. In all other cases, we preserve the original `TokenStream`. This could use a bit of refactoring/renaming - opening for a Crater run. r? `@ghost`
Fixes #79242
If a
macro_rules!recursively builds up a nested nonterminal(passing it to a proc-macro at each step), we will end up repeatedly
pretty-printing/retokenizing the same nonterminals. Unfortunately, the
'probable equality' check we do has a non-trivial cost, which leads to a
blowup in compilation time.
As a workaround, we cache the result of the 'probable equality' check,
which eliminates the compilation time blowup for the linked issue. This
commit only touches a single file (other than adding tests), so it
should be easy to backport.
The proper solution is to remove the pretty-print/retokenize hack
entirely. However, this will almost certainly break a large number of
crates that were relying on hygiene bugs created by using the reparsed
TokenStream. As a result, we will definitely not want to backportsuch a change.