Count objects performance improvement #167
Merged
Conversation
This file contains hidden or 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
…which is good enough for tests but the real world example shows that it needs some additional changes.
…and backport all capabilities like progress reporting and interruptability to something that's semantically similar.
…which forms the basis for having a single-threaded version of this
This opens a pathway to using something that's not a dashmap
…to allow for a single-threaded implementation with a RefCell. Unfortunately we can't just use a mutable HashSet without duplicating everything thanks to the &mut requirement or without unsafe code (i.e. storing a pointer and just turning it into a mutable ref)
…which is still not optimal due to RefCell, but probably the cost of that is neglectable or can be made up for with a faster hash. However, it's not exactly faster and it doesn't max out one core either.
…even though that doesn't really translate into much performance, despite technically saving millions of allocations. Maybe allocators are already pretty good at this.
Byron
added a commit
that referenced
this pull request
Aug 22, 2021
…tions…" This reverts commit 8d49976. Wait for servo/uluru#22 to land.
Byron
added a commit
that referenced
this pull request
Aug 22, 2021
…even though that doesn't really translate into much performance, despite technically saving millions of allocations. Maybe allocators are already pretty good at this.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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 current implementation suffers for being more flexible than it needs to be.
For one, it's implemented as iterator even though that's not required as counts are never streamed.
Secondly, it is forced to use thread-safe data structures which greatly slow down operation in a
single thread.
Unfortunately, it didn't get noticeably faster - using a RefCell in 40s of time about 2s go to the RefCell. PackCaches currently do a lot of allocations (and throw them away when the LRU portion kicks in), which could be improved with a free-list. But to do that, it requires support in the statically allocated version.
Furthermore it might be possible to improve cache efficiency by caching whole objects even though my strong feeling is that this won't do much as objects are never visited twice when traversing trees. When handling tree diffs, a better cache is definitely possible though, but that's out of scope here.