Optimize children iteration by reusing NodeData if possible #171
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This allows users of the API to apply a filter on the SyntaxKind before materializing concrete SyntaxNode/Token objects, which require a memory allocation for the NodeData. For slint, this removes ~400k allocations when parsing a largish project, about 50% of all rowan allocations (850k down to 450k).
Veykril
reviewed
Oct 28, 2024
When possible, reuse the allocated NodeData instead of allocating a new one for each iteration. This can be done as long as the refcount is 1 - we can then just rewire the values in NodeData to point to the new item. This removes ~220k allocations when compiling a largish slint file, about half of all rowan allocations that happen during iteration, i.e. we go from 450k down to 230k.
This makes this API actually useful from the outside - there is no lifetime problem with the matcher callback, and we can remap the raw Kind to the Language::Kind on the fly.
milianw
force-pushed
the
optimize-children-iteration
branch
from
60a632a to
ab5463e
Compare
milianw
added a commit
to milianw/rowan
that referenced
this pull request
Oct 31, 2024
Fixes an assertion in debug builds which I accidentally introduced when attending the review comments for [1] in [2] - instead of only removing the increment, I also removed the decrement which was wrong - `std::mem::forget` only allows us to remove the increment, but the decrement is still needed before free since we are in a place of code that is by definition only run when the rc value is set to 1. See also `can_take_ptr`. I did not spot this earlier since I ran the integration test on a release build, where the assertion was disabled. It's sad that the rowan repo itself doesn't have any big test coverage in this repo itself, but rather relies on external repos for testing purposes... [1]: rust-analyzer#171 (comment) [2]: https://github.com/rust-analyzer/rowan/compare/60a632ad984ab451e32058169193511154c675a9..ab5463e2749330be6846886c21e98c83caca8598 Fixes: rust-analyzer#172
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The first patch here is a rebased and slightly adapted version of #119 (the adaption is the addition of missing forwards in
api.rs). I believe that part should be reviewed in the existing pull request which hopefully could be updated by @theo-lw (feel free to take the patch here if it's to your likings).The second patch in here is new - it's an additional optimization which allows us to further reduce the number of memory allocations required for rowan tree visitation in
slintby another 50%. I.e. combined, the two patches go from 850k allocations down to 450k down to 230k, a total reduction by ~73%!