refactor!: Replace immutable-chunkmap with dual tree states
#495
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
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.
I'm doing this because I want to minimize the temporary allocations that are needed when applying a tree update. Ideally, if the tree update doesn't add any new data, there should be zero temporary allocations; we should be able to reuse existing collections to apply the update in-place. Since our event handling is built around having a full, consistent snapshot of the tree before the update, and that is now quite a fundamental feature of our design, I've concluded that we need to keep two copies of the tree state at all times. So that means double the memory usage. I'm betting that in most cases, the application uses most of its memory on things other than the accessibility trees, and that on truly memory-constrained systems, the gains from having fewer temporary allocations (and thus less risk of memory fragmentation) outweigh the downside of doubled memory usage for the tree state.
Importantly, the amount of copying that has to be done on each update is proportional to the number of nodes that were actually added, changed, or removed, not the size of the whole tree.
As a bonus, this also eliminates a third-party dependency for which we were the primary user, and reduces compiled code size (by 17.5 KB on x64 Windows).
I have more work to do to minimize temporary allocations, but am submitting this PR now to find out if we're in agreement about going in this direction.