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

tree_arena: Use hashbrown::HashMap as drop-in replacement to optimize tree-access #774

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

Philipp-M
Copy link
Contributor

As noted in #772 (review), this seems to be a really cheap but quite effective optimization, as we have hashbrown already in our dependency tree.
I've noticed speedups of the rewrite passes of up to >100%.
On average about 20-50% and is also definitely noticeable (smoother and less latency).

Another quite interesting observation with my (admittedly not really reproducible) performance tests,
is that the safe tree arena is actually faster than the unsafe version (roughly 10-30%), and tells yet another time that benches are really important while optimizing...

Rough overview of my performance tests/benches:
I've mostly hovered over 10000 buttons in the mason example while testing, but every second or so a new button is spawned and also this is quite a bit faster.
I did a quick'n dirty Instant::elapsed in the event loop (so rendering is not included, but I think there's also a speedup as we're iterating the tree).

Copy link
Contributor

@waywardmonkeys waywardmonkeys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't count me as the approving approval for a merge, but I'm +1 on this sort of change as long as you don't care about the DoS resistance ... this is a very common technique for good performance gain (that I use a lot elsewhere).

@Philipp-M
Copy link
Contributor Author

you don't care about the DoS resistance

I don't think we really have to care about that (even if it would be an issue), as we're (almost) controlling the keys, i.e. the key is just an auto-increment in most places.

I've seen that it can be manual in Grid, Flex and SizedBox, not sure if that should even be the case...
Maybe we should remove that possibility, as I currently don't see an advantage for this...

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I think we can land this. It is surprising quite how hash-lookup dominated we are.

@KGrewal1
Copy link
Contributor

KGrewal1 commented Dec 9, 2024

I understand it may not be reproducible, but would be interested in looking more at your perf tests: I wonder if in combination with the faster hashmap, and by mapping to a vector of children (instead of NodeId to child), plus possibly adding an iterator over children, if more speedups can be found #752 (comment)

@Philipp-M
Copy link
Contributor Author

It is surprising quite how hash-lookup dominated we are.

Yeah definitely, thought it would be measurable, but that it has this much impact...

I understand it may not be reproducible, but would be interested in looking more at your perf tests

Basically I used the mason example set it to "unlimited power" (data.active = true) with data.count = 10000 and then a simple elapsed = Instant::now() - now_at_beginning_of_handle_signals at the end of MasonryState::handle_signals, filtered too small values out (e.g. when nothing has changed), and to get a better picture, averaged these values over multiple frames (should probably look into more sophisticated ways to profile).

I wonder if in combination with the faster hashmap, and by mapping to a vector of children

Yeah there seems to be a potential optimizing the lookup of children, when just the underlying hashmap does have this much impact, but should probably be discussed further on zulip (TreeArena topic?) or other PRs.

@Philipp-M Philipp-M added this pull request to the merge queue Dec 9, 2024
Merged via the queue into linebender:main with commit 8e05367 Dec 9, 2024
17 checks passed
@Philipp-M Philipp-M deleted the hashbrown-tree-arena branch December 9, 2024 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants