feat(base): implement sorted collections #869
Conversation
Signed-off-by: Dario Valdespino <[email protected]>
WalkthroughWalkthroughThe changes involve a significant restructuring of tree-based collections in Kotlin, enhancing Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| /** | ||
| * Rotate a [node] in the given [direction] (to the right if `true`, to the left if `false`). The [node] _must_ have | ||
| * a child in the direction opposing the rotation (a left child for right-rotation, right child for left-rotation), | ||
| * which is used as the "pivot" for the operation, otherwise an exception will be thrown. | ||
| * | ||
| * Rotation affects the node's parent and its "pivot" child, the following figure visually depicts left-rotation | ||
| * (right-rotation is a mirror case and has the same result, only in the opposite direction): | ||
| * | ||
| * ``` | ||
| * initial | reparent N, R | reparent <CR> | reparent [N] (final) | ||
| * ----------|---------------|---------------|--------------------- | ||
| * / | / | / | / | ||
| * [N] | [N] (R) | [N] (R) | (R) | ||
| * / \ | / / | / \ | / | ||
| * L (R) | L <CR> | L <CR> | [N] | ||
| * / | | | / \ | ||
| * <CR> | | | L <CR> | ||
| * ----------|------(1)------|------(2)------|-----(3)------------- | ||
| * ↑ rotate [N] to the left | ||
| * ``` | ||
| * | ||
| * > See this method's source code for implementation details on (1), (2), and (3). | ||
| * | ||
| * Rotation is used to maintain the properties of the Red/Black Tree after a node is [inserted][rebalanceAdded] or | ||
| * [removed][rebalanceRemoved], and can be paired with recoloring when necessary. | ||
| */ | ||
| private fun rotate(node: Node<K, V>, direction: Boolean) { | ||
| val pivot = node.child(!direction) ?: error("node is missing a ${if (direction) "left" else "right"} child") | ||
| val orphan = pivot.child(direction) | ||
|
|
||
| node.parent?.replace(node, pivot) // (1) | ||
| if (node == root) root = pivot | ||
|
|
||
| node.setChild(!direction, orphan) // (2) | ||
| pivot.setChild(direction, node) // (3) | ||
| } | ||
| } |
There was a problem hiding this comment.
New rotation method with visual documentation :D
There was a problem hiding this comment.
This is amazing. For once I understand Red Black trees lol
darvld
changed the title
| override val keys: MutableSet<K> by lazy(::KeySet) | ||
| override val entries: MutableSet<MutableEntry<K, V>> by lazy(::EntrySet) |
There was a problem hiding this comment.
Sets returned by keys and entries share the same nodes as the map itself, no additional collections are allocated, and removing keys/entries from the sets will also update the map (following JVM maps behavior)
| /** A stubbed [SortedMap] implementation, meant to be used as an optimized return value for [emptySortedMap]. */ | ||
| private object EmptySortedMap : SortedMap<Nothing, Nothing> { | ||
| override fun get(key: Nothing): Nothing? = null | ||
|
|
||
| override val entries: Set<Entry<Nothing, Nothing>> get() = emptySet() | ||
| override val keys: Set<Nothing> get() = emptySet() | ||
| override val size: Int get() = 0 | ||
| override val values: Collection<Nothing> get() = emptyList() | ||
|
|
||
| override fun isEmpty(): Boolean = true | ||
| override fun containsValue(value: Nothing): Boolean = false | ||
| override fun containsKey(key: Nothing): Boolean = false | ||
| } |
There was a problem hiding this comment.
Specialized, stubbed implementation for emptySortedMap(), avoids allocating an actual map.
| private val bundles: Map<Int, BundleInfo> = TreeMap(), | ||
| private val knownPathMap: Map<String, VfsObjectInfo> = TreeMap(), | ||
| private val bundles: Map<Int, BundleInfo> = emptyMap(), | ||
| private val knownPathMap: Map<String, VfsObjectInfo> = emptyMap(), |
There was a problem hiding this comment.
The previous sorted map implementation (TreeMap) was leaked here, replaced with proper API usage (emptyMap() since there no need for a specialized sorted map here)
darvld
force-pushed
the
fix/tree-map
branch
2 times, most recently
from
042d3f5 to
2c5c15c
Compare
Signed-off-by: Dario Valdespino <[email protected]>
Signed-off-by: Dario Valdespino <[email protected]>
Signed-off-by: Dario Valdespino <[email protected]>
Signed-off-by: Dario Valdespino <[email protected]>
Signed-off-by: Dario Valdespino <[email protected]>
Signed-off-by: Dario Valdespino <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #869 +/- ##
==========================================
- Coverage 36.12% 35.75% -0.38%
==========================================
Files 506 499 -7
Lines 16117 15759 -358
Branches 2140 2129 -11
==========================================
- Hits 5823 5634 -189
+ Misses 9894 9728 -166
+ Partials 400 397 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 82 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (17)
- packages/base/api/base.api (6 hunks)
- packages/base/src/commonMain/kotlin/elide/struct/RedBlackTree.kt (1 hunks)
- packages/base/src/commonMain/kotlin/elide/struct/SortedMaps.kt (1 hunks)
- packages/base/src/commonMain/kotlin/elide/struct/SortedSets.kt (1 hunks)
- packages/base/src/commonMain/kotlin/elide/struct/TreeMap.kt (1 hunks)
- packages/base/src/commonMain/kotlin/elide/struct/TreeSet.kt (1 hunks)
- packages/base/src/commonMain/kotlin/elide/struct/api/MutableSortedMap.kt (1 hunks)
- packages/base/src/commonMain/kotlin/elide/struct/api/MutableSortedSet.kt (1 hunks)
- packages/base/src/commonMain/kotlin/elide/struct/codec/TreeMapCodec.kt (1 hunks)
- packages/base/src/commonMain/kotlin/elide/struct/codec/TreeSetCodec.kt (1 hunks)
- packages/base/src/commonTest/kotlin/elide/struct/SortedMapTest.kt (14 hunks)
- packages/graalvm/src/main/kotlin/elide/runtime/gvm/internals/vfs/EmbeddedGuestVFSImpl.kt (2 hunks)
- packages/serverless/api/serverless.api (1 hunks)
- packages/serverless/src/commonMain/kotlin/elide/http/HttpHeaders.kt (2 hunks)
- packages/serverless/src/commonMain/kotlin/elide/http/HttpMapping.kt (1 hunks)
- packages/serverless/src/commonMain/kotlin/elide/http/MutableHttpHeaders.kt (2 hunks)
- packages/serverless/src/commonMain/kotlin/elide/http/MutableHttpMapping.kt (1 hunks)
Files not reviewed due to errors (4)
- packages/serverless/src/commonMain/kotlin/elide/http/MutableHttpHeaders.kt (no review received)
- packages/serverless/src/commonMain/kotlin/elide/http/HttpHeaders.kt (no review received)
- packages/base/src/commonMain/kotlin/elide/struct/TreeMap.kt (no review received)
- packages/base/src/commonTest/kotlin/elide/struct/SortedMapTest.kt (no review received)
Files skipped from review due to trivial changes (1)
- packages/graalvm/src/main/kotlin/elide/runtime/gvm/internals/vfs/EmbeddedGuestVFSImpl.kt
Additional comments not posted (23)
packages/base/src/commonMain/kotlin/elide/struct/api/MutableSortedSet.kt (1)
19-19: Interface definition forMutableSortedSetcorrectly extends bothMutableSetandSortedSetwith a type constraint that ensures elements are comparable.packages/base/src/commonMain/kotlin/elide/struct/api/MutableSortedMap.kt (1)
19-21: Interface definition forMutableSortedMapcorrectly extends bothMutableMapandSortedMapwith a type constraint that ensures keys are comparable.packages/serverless/src/commonMain/kotlin/elide/http/HttpMapping.kt (2)
18-18: Import ofemptySortedMapis correctly used to provide a default empty sorted map for theHttpMappingclass.
26-26:HttpMappingclass effectively uses Kotlin's delegation feature to implementSortedMapfunctionality through thebackingfield.packages/serverless/src/commonMain/kotlin/elide/http/MutableHttpMapping.kt (2)
20-20: Import ofmutableSortedMapOfis correctly used to provide a default mutable sorted map for theMutableHttpMappingclass.
28-28:MutableHttpMappingclass effectively uses Kotlin's delegation feature to implementMutableSortedMapfunctionality through thebackingfield.packages/base/src/commonMain/kotlin/elide/struct/codec/TreeSetCodec.kt (2)
17-17: Import ofSetSerializeris correctly used to provide serialization functionality for theTreeSetCodecclass.
23-27:TreeSetCodecclass effectively uses delegation to implement serialization forTreeSetusing aSetSerializer.packages/base/src/commonMain/kotlin/elide/struct/codec/TreeMapCodec.kt (1)
23-24:TreeMapCodecclass effectively uses delegation to implement serialization forTreeMapusing aMapSerializer.packages/base/src/commonMain/kotlin/elide/struct/SortedSets.kt (2)
19-30:EmptySortedSetobject is correctly implemented as a stub for an empty sorted set, providing optimized return values.
32-52: Utility functions for creating empty, read-only, and mutable sorted sets are correctly implemented and use theTreeSetclass effectively.packages/base/src/commonMain/kotlin/elide/struct/TreeSet.kt (1)
16-58:TreeSetclass is correctly implemented as aMutableSortedSetand extendsRedBlackTree, providing efficient operations and correct serialization.packages/base/src/commonMain/kotlin/elide/struct/SortedMaps.kt (3)
34-38: LGTM! The use ofUNCHECKED_CASTis appropriate here to return a type-safe empty map without unnecessary allocations.
48-50: LGTM! Efficiently initializes aTreeMapand populates it from a collection of pairs.
60-62: LGTM! Consistently handles vararg pairs to populate aTreeMap, ensuring API flexibility.packages/base/src/commonMain/kotlin/elide/struct/RedBlackTree.kt (4)
31-31: Ensure the constructor's visibility aligns with intended use cases.Verification successful
The verification process confirms that the internal constructor of the
RedBlackTreeclass is not being used externally, which aligns with the intended use cases. This ensures that the encapsulation and intended access restrictions of the constructor are maintained correctly.
- No external invocations of the
RedBlackTreeinternal constructor were found in the Kotlin files.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the internal constructor is not inadvertently exposed or misused. # Test: Search for the constructor usage. Expect: No external usage if intended for internal use only. rg --type kotlin 'RedBlackTree\('Length of output: 35
51-61: Consider the implications of using@Volatileon properties in Kotlin/Native environments.Verification successful
The use of
@Volatilein theRedBlackTree.ktfile is appropriate and necessary for ensuring thread safety in a multi-threaded environment. This annotation prevents thread-local caching of the variables, which is crucial for the correct operation of concurrent data structures like the red-black tree implemented here. The usage is consistent with other parts of the codebase where thread safety is a concern.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the `@Volatile` annotation is necessary and correctly used in the context of Kotlin/Native. # Test: Check for multiplatform compatibility issues related to `@Volatile`. rg --type kotlin '@Volatile' -C 3Length of output: 5695
171-173: Use@JvmFieldwith@Volatileonly if accessing from Java code, otherwise it might be unnecessary.
391-439: Double-check the rebalancing logic inrebalanceRemovedfor correctness and efficiency.Verification successful
The provided script output shows the implementation of the
rebalanceRemovedmethod in theRedBlackTree.ktfile, which is crucial for maintaining the properties of a red-black tree after node removal. The method handles various cases such as when the sibling of the node is red, when the sibling and its children are black, and when the sibling has at least one red child. These cases involve recoloring and rotating nodes to restore the tree's properties.From the output, it appears that the method is implemented with recursive calls to handle the propagation of the "double black" condition up the tree, which is typical in red-black tree algorithms to handle cases where the tree becomes unbalanced due to deletions.
The logic in the method seems to correctly address the different scenarios that can occur during the rebalancing process after a node is removed. The method uses tail recursion, which is optimized in Kotlin to handle deeper recursive calls efficiently. This suggests that the method is both correct and efficient in terms of handling the rebalancing of the tree.
Given the detailed implementation provided and the correct handling of various edge cases in the rebalancing process, there are no apparent issues or inefficiencies in the rebalancing logic as implemented in the
rebalanceRemovedmethod.Therefore, the review comment asking to double-check the rebalancing logic for correctness and efficiency can be considered verified with no issues found.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the rebalancing logic in `rebalanceRemoved`. # Test: Check for potential inefficiencies or errors in rebalancing logic. rg --type kotlin 'rebalanceRemoved' -C 20Length of output: 15180
packages/base/api/base.api (4)
228-238: Ensure that the API forRedBlackTreecorrectly exposes only the intended methods and properties.Verification successful
The Kotlin implementation of
RedBlackTreeprovides a detailed structure of the class and its nestedNodeclass. The API file, however, abstracts much of this detail, focusing on the methods that manipulate the tree rather than the properties of the nodes themselves.Analysis of API File vs Kotlin Implementation:
Methods in API File:
addNode: Expected to add a node to the tree.equals: Standard equality check.findNodeByKey: Expected to find a node by key.getNodeCount: Expected to return the count of nodes.hashCode: Standard hash code generation.nodes: Likely returns a sequence of nodes.removeNode: Expected to remove a specific node.removeNodeByKey: Expected to remove a node by key.reset: Likely resets the tree to an empty state.Methods in Kotlin Implementation:
equalsandhashCodeare implemented in theNodeclass, not directly in theRedBlackTreeclass.- Other methods like
addNode,findNodeByKey, etc., are not explicitly shown in the provided snippet but are likely part of theRedBlackTreeclass as they are typical for such data structures.Conclusion:
The API file seems to correctly abstract the functionality of the
RedBlackTreewithout exposing internal details like theNodeclass or its properties, which is typical for an API file. The methods listed are appropriate for the operations one would expect to perform on a red-black tree.Based on the information provided and the typical structure and functionality of a red-black tree, the API exposure in
base.apiforRedBlackTreeappears to be consistent with its implementation in Kotlin. The methods exposed are those that would be used externally, without exposing the internal workings of the tree, which aligns with good API design principles.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the API exposure for `RedBlackTree` is consistent with its implementation. # Test: Compare API file with the Kotlin implementation to ensure consistency. rg --type kotlin 'class RedBlackTree' -A 50Length of output: 5422
240-255: Check for consistency in the API definition ofRedBlackTree$Nodewith its actual implementation.Verification successful
The provided output from the script execution reveals the Kotlin class definition for
RedBlackTree$Nodewithin theelide/struct/RedBlackTree.ktfile. This definition includes properties such askey,value,color,left,right, andparent, along with methods likesetValue,equals, andhashCode. This matches the API definition provided in the initial context, confirming that the properties and methods are consistent between the API definition and the actual implementation.Thus, the API definition of
RedBlackTree$Nodeis consistent with its actual implementation in the Kotlin class.Final Conclusion:
- The API definition of
RedBlackTree$Nodematches its implementation in the Kotlin class as described in theelide/struct/RedBlackTree.ktfile.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure that the `RedBlackTree$Node` API matches the Kotlin class definition. # Test: Compare the API definition with the Kotlin class implementation. rg --type kotlin 'class Node' -A 30Length of output: 59079
278-300: Review theTreeMapAPI for completeness and correctness, especially method signatures and visibility.Verification successful
The
TreeMapAPI appears to be complete and correct in terms of method signatures and visibility. The methods provided cover typical functionalities expected from a sorted map, including operations for adding, removing, and querying elements, as well as methods for retrieving keys, values, and entries. The visibility of methods (public, final, synthetic) is appropriate for their intended use, and additional functionalities like the inner classKeySetand the codec class for serialization support the main functionalities of the TreeMap.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the completeness and correctness of the `TreeMap` API. # Test: Check for missing or incorrect method signatures in `TreeMap`. rg --type kotlin 'class TreeMap' -A 50Length of output: 7019
Line range hint
307-321: Ensure that theTreeSetAPI is fully aligned with its implementation, particularly in terms of method availability and signatures.Verification successful
The verification of the
TreeSetAPI against its Kotlin implementation indicates that the API is generally aligned with the implementation. The differences observed, such as additional methods likeremoveAllandretainAllin the Kotlin implementation, are standard for Kotlin collections and do not suggest any misalignment. The synthetic methods in the API are likely for Java interoperability and are not explicitly listed in the Kotlin code, which is expected.
- The
sizeproperty in Kotlin corresponds to thegetSizemethod in the API, reflecting typical Java-Kotlin interoperability where properties in Kotlin appear as methods in Java.- All other methods (
add,addAll,contains,remove,clear,isEmpty,iterator) are consistently present in both the API and the implementation, with signatures that align well considering Java-Kotlin interoperability.Overall, the
TreeSetAPI appears to be correctly implemented in Kotlin, with all essential functionalities present and correctly reflected in the API documentation.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Confirm that the `TreeSet` API aligns with the Kotlin implementation. # Test: Validate method signatures and availability in `TreeSet`. rg --type kotlin 'class TreeSet' -A 50Length of output: 5011
packages/base/src/commonMain/kotlin/elide/struct/RedBlackTree.kt
Outdated
Show resolved
Hide resolved
Signed-off-by: Dario Valdespino <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
Out of diff range and nitpick comments (1)
packages/base/src/commonMain/kotlin/elide/struct/RedBlackTree.kt (1)
28-30: Clarify the concurrency safety in the class documentation.The class documentation states that the class is not safe for concurrent modifications. It might be helpful to explicitly mention that external synchronization is required if used in a multi-threaded environment.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/base/src/commonMain/kotlin/elide/struct/RedBlackTree.kt (1 hunks)
Additional comments not posted (1)
packages/base/src/commonMain/kotlin/elide/struct/RedBlackTree.kt (1)
263-293: Verify thread safety foraddNodemethod.The
addNodemethod modifies the tree structure and potentially triggers a rebalance. Given the lack of internal synchronization mechanisms, this could lead to race conditions in a multi-threaded environment. Verify that the class is intended to be used in a single-threaded context or recommend adding synchronization.Verification successful
The output from the executed script confirms that the
RedBlackTreeclass in theRedBlackTree.ktfile explicitly mentions that it is not safe for concurrent modifications. This aligns with the concerns raised in the review comment about the lack of internal synchronization mechanisms and the potential for race conditions in a multi-threaded environment.Given this information, the review comment is accurate in highlighting the potential issues with thread safety in the
addNodemethod. The class documentation itself advises against concurrent modifications, which supports the suggestion to verify the intended single-threaded usage or to consider adding synchronization mechanisms if multi-threaded support is necessary.Final Response:
The review comment regarding the thread safety of the
addNodemethod in theRedBlackTreeclass is verified as accurate. The class documentation explicitly states that it is not safe for concurrent modifications. This supports the concerns raised about potential race conditions in a multi-threaded environment.
- The class documentation in
RedBlackTree.ktexplicitly mentions the lack of thread safety.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for mentions of thread safety or single-threaded usage in the class documentation. # Test: Search for relevant keywords in `RedBlackTree.kt`. rg --type kotlin 'thread|concurrent' packages/base/src/commonMain/kotlin/elide/struct/RedBlackTree.ktLength of output: 255
Summary
This PR fixes the sorted map implementation in the
basemodule, adds an implementation for sorted sets, and includes additional performance fixes and API refactoring:New
RedBlackTreeabstract base class (based on the previousAbstractTreeMapimplementation), with reworked internal logic and documentation. Note that the class ispublicbut its constructor isinternal, as it is not meant to be inherited by third-party code.MutableSortedMapandMutableSortedSetnow inherit their immutable counterparts, matching the pattern set by the Kotlin standard library collections.AbstractTreeMap,TreeMap, andMutableTreeMaphave been merged intoTreeMap, which now implementsMutableSortedMap(and transitivelySortedMap) and extends fromRedBlackTree.MutableTreeSetandTreeSethave been merged intoTreeSet, which now implementsMutableSortedSet(and transitivelySortedSet) and extends fromRedBlackTree.TreeMapno longer allocates new collections forMutableMap.keysandMutableMap.entriesproperties, instead the returned mutable sets are backed by a specialized implementation which shares the same tree with the map:elide/packages/base/src/commonMain/kotlin/elide/struct/RedBlackTreeMap.kt
Lines 33 to 40 in 332e1d3
Using
sortedMapOf(),mutableSortedMapOf()and similar factory functions is the recommended way to create a sorted collection.TreeMapandTreeSetcan also be constructed directly but this is discouraged, they are exposed aspublicto allow advanced use cases in the future, where APIs unique to those implementations are added (similar to howHashMapprovides additional features in the JVM).Calling
emptySortedMap()andemptySortedSet()now returns special stubbed objects for better performance:elide/packages/base/src/commonMain/kotlin/elide/struct/SortedMaps.kt
Lines 20 to 21 in 332e1d3