Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DRAFT] - Try to optimize preorder iteration with pool allocation #121

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

theo-lw
Copy link
Contributor

@theo-lw theo-lw commented Oct 26, 2021

Preorder iteration allocates O(size of tree) nodes. However, there are some cases where only O(height of tree) nodes are alive at a given time. I'm trying to see if using a pool allocator can help avoid excess allocations in this case.

@matklad
Copy link
Member

matklad commented Oct 31, 2021

Yeah, in general, it is a reasonable assumption that at any given point at time there are only few live nodes for any given tree. (although might be good to check that using rust-analyzer's analysis_stats). So it'd be cool to just add nodes to / pop them from a free-list. Ideally, tree traversal which doesn't retain nodes shouldn't do any allocation or atomic ops.

In the original implementation, this was achieved by using a thread-local free-list of nodes. As it turned out several years later, at least my implementation of that apporoach was slower than just allocating the nodes.

@theo-lw
Copy link
Contributor Author

theo-lw commented Oct 31, 2021

In the original implementation, this was achieved by using a thread-local free-list of nodes. As it turned out several years later, at least my implementation of that apporoach was slower than just allocating the nodes.

Yeah right now my implementation isn't faster than just allocating the nodes. Maybe this is a fruitless endeavour.

@theo-lw
Copy link
Contributor Author

theo-lw commented Oct 31, 2021

With my latest changes I do see a small speedup when running analysis-stats on rowan:

Without these changes:

Screen Shot 2021-10-31 at 6 45 26 PM

With these changes:

Screen Shot 2021-10-31 at 6 44 37 PM

Will probably need more benchmarks to see if this is truly significant.

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

Successfully merging this pull request may close these issues.

2 participants