Store token trees in contiguous Vec instead of as a tree
#18327
Conversation
rustbot
added
the
S-waiting-on-review
|
Related https://rust-lang.zulipchat.com/#narrow/channel/185405-t-compiler.2Frust-analyzer/topic/mbe.20transcription I tried this before (i also saw no perf impact like you are, with a slight memory usage decrease iirc but the branch seems to be lost now), for some reason I was unable to actually integrate it without rewriting the mbe crate at the time though. Cant recall why that was. But neat either way. Though I guess this will heavily clash with #17830 now 😬 (wonder when Ill be able to get back to that PR, probably not this year) |
Perhaps that was the hardness of traversing the tree correctly? It was very ugly in my early attempts, but when I discovered the secret sauce (a |
|
Might very well have been the traversal yes |
ChayimFriedman2
force-pushed
the
flat-tt
branch
5 times, most recently
from
4bda4a9
to
3796848
Compare
|
☔ The latest upstream changes (presumably #17954) made this pull request unmergeable. Please resolve the merge conflicts. |
I expected this to be faster (due to less allocations and better cache locality), but benchmarked it is not (neither it is slower). Memory usage, however, drops by ~50mb (of `analysis-stats .`). I guess tt construction is just not hot. This also simplifies using even less memory for token trees by compressing equal span, which I plan to do right after. Some workflows are more easily expressed with a flat tt, while some are better expressed with a tree. With the right helpers, though (which was mostly a matter of trial and error), even the worst workflows become very easy indeed.
ChayimFriedman2
force-pushed
the
flat-tt
branch
from
3796848
to
9ff252b
Compare
|
☔ The latest upstream changes (presumably #18366) made this pull request unmergeable. Please resolve the merge conflicts. |
As part of my attempts to optimize #18074, I noticed that token trees contain a significant portion of our memory usage (around 200mb out of 1650mb). As such, I went to optimize them.
TokenTreeis quite large (64 bytes), and a significant part of this is spans. I came up an idea to shrink spans, but it will be easier to do if token trees were contiguous in memory. Given that this seemed like something worth doing perf-wise anyway, I went to do this :)I expected this to be faster (due to less allocations and better cache locality), but benchmarked it is not (neither it is slower). I guess tt construction is just not hot. Memory usage, however, drops by ~50mb (of
analysis-stats .), probably due toTokenTreeshrinking to 48 bytes.Some workflows are more easily expressed with a flat tt, while some are better expressed with a tree. With the right helpers, though (which was mostly a matter of trial and error), even the worst workflows become very easy indeed.