-
Notifications
You must be signed in to change notification settings - Fork 81
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
Reimplement SortedSet for JS/native to improve performance #1167
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good aside from minor style nitpicking. Correctness is checked by tests, so it should be good here.
But before merging, I'd like to see comparison before/after to make sure that additional memory consumption is justified.
compose/ui/ui/src/jsNativeMain/kotlin/androidx/compose/ui/node/SortedSet.jsNative.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/jsNativeMain/kotlin/androidx/compose/ui/node/SortedSet.jsNative.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/jsNativeMain/kotlin/androidx/compose/ui/node/SortedSet.jsNative.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/jsNativeMain/kotlin/androidx/compose/ui/node/SortedSet.jsNative.kt
Outdated
Show resolved
Hide resolved
I've run this test on
The results are:
* with Looking at the code, |
The disparity between |
Proposed Changes
Reimplement
SortedSet
via min-heap + hashmap in thejsNative
source-set.This should give it the following performance:
add
,remove
: O(logN), due to the heap.first
: O(1), due to the heap.contains
: O(1), thanks to the hash mapPreviously the performance was:
add
,remove
: O(N), due to the array.first
: О(1)contains
: O(logN), due to the binary search in the array.Testing
Test: Ran the existing tests and added some new ones.