proc-macro-srv: Reimplement token trees via immutable trees #21097
Conversation
Veykril
changed the title
|
This is basically finished, but I realized we have a severe lack of tests here so I'll add some more thorough testing later. |
Veykril
force-pushed
the
push-zpqupukpkrts
branch
3 times, most recently
from
fb1f83e to
4774e6e
Compare
Veykril
force-pushed
the
push-zpqupukpkrts
branch
from
4774e6e to
b5fc29e
Compare
rustbot
added
the
S-waiting-on-review
Shourya742
left a comment
Shourya742
left a comment
There was a problem hiding this comment.
LGTM. Just some questions.
| let id = self.token_id_of(punct.span); | ||
| self.punct.push(PunctRepr { | ||
| char: punct.ch as char, | ||
| spacing: if punct.joint { tt::Spacing::Joint } else { tt::Spacing::Alone }, |
There was a problem hiding this comment.
Curious, why we don't encode JointHidden variant?
There was a problem hiding this comment.
JointHidden becomes Alone after a proc-macro roundtrip, so this preserves that logic. The hidden part effectively means its hidden from proc-macros in this sense.
| span: proc_macro_srv::DelimSpan { | ||
| open: read_span(repr.open), | ||
| close: read_span(repr.close), | ||
| // FIXME |
There was a problem hiding this comment.
Shouldn’t Reader and Writer really be restricted to a SpanTransformer<Table = SpanDataIndexMap and Span = Span>? That’s the only setup that actually gives us the full span information needed to reconstruct things like entire, right?
I am also curios why is SpanTransformer even implemented for SpanId in the first place?
There was a problem hiding this comment.
Our macro setup doesn't really support entire at all, and afaik, its not really used upstream (in rustc) either anymore so it might be removed in the future.
As for SpanId, it's the legacy span type. r-a doesn't use it anymore but we keep it for backwards compatibility for RustRover (as well as if we need to break things, we can always fall back to that mode).
ChayimFriedman2
left a comment
ChayimFriedman2
left a comment
There was a problem hiding this comment.
Mostly skimmed, but LGTM.
4 participants
For macros, rust has the concept of TokenTrees and TokenStreams which are basically a tree-like form of the underlying tokenized input with
(){}[]delimiters forming Groups (internal nodes) and all other tokens forming leaf nodes of the tree.In rust-analyzer today these trees are implemented as a flattened list using indices to reduce their memory impact as we keep a lot of these memoized (implementation). This comes at the cost of mutating operations being more expensive though and we especially feel this in proc-macro expansion.
As it turns out, one of the primitive and very commonly used operations on token streams in proc-macros is concatenation, an operation that with our model is notoriously expensive as we need to do a deepclone of the first stream, append the second stream and rewrite the internal indices. Combine this with deeply recursive concatenations as is common with the
quote!macro and we run into issues where the proc-macro server can take 8 seconds to expand these 8 derive invocations ofnum_derive::FromPrimitiveon decent machines.Fortunately, the proc-macro server has no reason to use the same implementation of TokenStreams and TokenTrees as rust-analyzer since it doesn't persist them in memory anways. So instead, to gain good concatenation performance, we can do with what rustc does and store them as immutable trees with shared internal nodes.
Before this change cache priming took 56 seconds on Zed's codebase, with the new proc-macro server it now takes 45.