close
Skip to content

perf: optimize allocation strategies of output/parser/event#22078

Open
landaire wants to merge 1 commit intorust-lang:masterfrom
landaire-contrib:push-lnvzwpkxmypz
Open

perf: optimize allocation strategies of output/parser/event#22078
landaire wants to merge 1 commit intorust-lang:masterfrom
landaire-contrib:push-lnvzwpkxmypz

Conversation

@landaire
Copy link
Copy Markdown

@landaire landaire commented Apr 18, 2026

This change does a couple of different, but semi-related things. Happy to split this change up into multiple PRs if desired to keep these in isolation.

Note: This is my first contribution to rust-analyzer and also was mostly done by Claude. All tests pass and from my human analysis of the change things look good. This description written by a human -- tables assembled by Claude.

Memory profiling done on an M5 MacBook Pro.

Event Size Shrinkage

Through memory profiling, it was realized that the parser::Parser, a large consumer of memory, retains many parser::Events that are forced to be a 24-byte structure, mostly because of the Error variant which held a String. Claude reworked this pattern to instead use an internal Vec<String> and the Error variant holds a 32-bit index into this vector.

Claude also noted that forward_parent field in Event::Start is always non-zero and can be changed to a NonZeroU32 for further memory savings.

baseline after
Parser::new bytes 2.40 GiB 818 MiB
calls 76,546 76,546

Output.event optimization

The Output.event vec uses a naive push strategy for allocation, but can be pre-allocated to avoid unnecessary memory growth from the vector's reallocation strategy.

baseline after
Output::enter_node (grow path) 584 MiB / 214k 0
Output::with_event_capacity (new) 209 MiB / 48k
net 584 MiB / 214k 209 MiB / 48k

Overall results:

metric baseline after both wins Δ
total bytes allocated 8.33 GiB 6.36 GiB −23.6%
total blocks 55,653,282 55,488,471 −164,811
at-gmax live bytes 1.06 GiB 1.06 GiB
at-gmax live blocks 10,567,626 10,567,623
at-end live bytes 633 MiB 631 MiB −2 MiB

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 18, 2026
@ChayimFriedman2
Copy link
Copy Markdown
Contributor

ChayimFriedman2 commented Apr 19, 2026

First, do you have any benchmark proving that parsing indeed became faster with these changes?

Second, about shrinking Event: Event is very short-living, therefore I don't see the benefit in shrinking it. I also don't expect to have an effect from cache.

I have no problems merging the with_capacity() change alone, though, it can never hurt.

@landaire
Copy link
Copy Markdown
Author

@ChayimFriedman2 sorry, not sure I follow on this:

First, do you have any benchmark proving that parsing indeed became faster with these changes?

Maybe the perf tag is misleading but this is entirely about memory usage. I didn't do any runtime speed benchmarking. If you'd like me to do some, I'm happy to see what the implications of this change is.

Second, about shrinking Event: Event is very short-living, therefore I don't see the benefit in shrinking it

While it might be short-living, it has a pretty major impact on peak memory usage by RA which I imagine does indeed have cascading effects. Especially for individuals on more resource-constrained systems (Apple is once again shipping MacBooks with 8GiB of RAM after all).

@ChayimFriedman2
Copy link
Copy Markdown
Contributor

It shouldn't have an impact even on peak memory usage because we aren't parsing many files concurrently, at most the number of cores. Unless profiling will show otherwise, of course.

@landaire
Copy link
Copy Markdown
Author

Ah I see what you're saying. Yeah I think the Event might come down to misinterpreting the dhat output and doing a bit too much at once. Let me run things again and see if my original interpretation still holds true.

@Veykril
Copy link
Copy Markdown
Member

Veykril commented Apr 19, 2026

The changes to Event do have the benefit of the type becoming a POD with no drop glue, which I think can have a beneficial effect when the event vector gets resized. I am fine with these changes personally. Though profiling would of course be nice either way.

@ChayimFriedman2
Copy link
Copy Markdown
Contributor

Not when it is resized but when it's dropped, still this can be beneficial.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants