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.

Sign up for GitHub

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

Jump to bottom

Remove node from indexes during GC #191

Merged
merged 2 commits into from
Nov 4, 2024
Merged

Remove node from indexes during GC #191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4e974ab
Remove node from indexes during GC
hiddenviewer Oct 31, 2024
8a926d4
Add test case for removing node from indexes during GC
hiddenviewer Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions Sources/Document/CRDT/CRDTText.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,11 +313,17 @@ final class CRDTText: CRDTElement {
}

/**
* `checkWeight` returns false when there is an incorrect weight node.
* for debugging purpose.
* `getTreeByIndex` returns the tree by index for debugging.
*/
func getTreeByIndex() -> SplayTree<CRDTTextValue> {
return self.rgaTreeSplit.getTreeByIndex()
}

/**
* `getTreeByID` returns the tree by ID for debugging.
*/
func checkWeight() -> Bool {
self.rgaTreeSplit.checkWeight()
func getTreeByID() -> LLRBTree<RGATreeSplitNodeID, RGATreeSplitNode<CRDTTextValue>> {
return self.rgaTreeSplit.getTreeByID()
}

/**
Expand Down
16 changes: 12 additions & 4 deletions Sources/Document/CRDT/RGATreeSplit.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,11 +535,17 @@ class RGATreeSplit<T: RGATreeSplitValue> {
}

/**
* `checkWeight` returns false when there is an incorrect weight node.
* for debugging purpose.
* `getTreeByIndex` returns the tree by index for debugging purpose.
*/
public func getTreeByIndex() -> SplayTree<T> {
return self.treeByIndex
}

/**
* `getTreeByID` returns the tree by ID for debugging purpose.
*/
public func checkWeight() -> Bool {
self.treeByIndex.checkWeight()
public func getTreeByID() -> LLRBTree<RGATreeSplitNodeID, RGATreeSplitNode<T>> {
return self.treeByID
}

/**
Expand Down Expand Up @@ -849,6 +855,8 @@ extension RGATreeSplit: GCParent {
guard let node = node as? RGATreeSplitNode<T> else {
return
}
self.treeByIndex.delete(node)
self.treeByID.remove(node.id)

node.prev?.setNext(node.next)
node.next?.setPrev(node.prev)
Expand Down
14 changes: 10 additions & 4 deletions Sources/Document/Json/JSONText.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,11 +247,17 @@ public class JSONText {
}

/**
* `checkWeight` returns false when there is an incorrect weight node.
* for debugging purpose.
* `getTreeByIndex` returns the tree by index for debugging.
*/
func getTreeByIndex() -> SplayTree<CRDTTextValue>? {
return self.text?.getTreeByIndex()
}

/**
* `getTreeByID` returns the tree by ID for debugging.
*/
public func checkWeight() -> Bool {
self.text?.checkWeight() ?? false
func getTreeByID() -> LLRBTree<RGATreeSplitNodeID, RGATreeSplitNode<CRDTTextValue>>? {
return self.text?.getTreeByID()
Comment on lines +250 to +260
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Test coverage is adequate, but checkWeight method still exists

The new debugging methods getTreeByIndex and getTreeByID are well tested in DocumentTests.swift, particularly in garbage collection scenarios. However, I notice that:

  • getTreeByID is used to verify tree size after various operations including garbage collection
  • checkWeight method still exists in SplayTree.swift and is being used in integration tests
  • Both debugging methods coexist, which contradicts the summary about checkWeight being replaced

This suggests the PR summary might be incorrect about the removal of checkWeight.

🔗 Analysis chain

Verify test coverage for the new debugging methods.

Let's ensure the new debugging methods are properly covered by tests, especially in garbage collection scenarios.

Let's examine the actual test implementations and the remaining checkWeight usage to make a more informed decision about the test coverage.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for test files that might use these new debugging methods
rg -l "getTreeByIndex|getTreeByID" --type swift "Tests/"

# Look for any remaining uses of the old checkWeight method
rg -l "checkWeight" --type swift

Length of output: 248

Script:

#!/bin/bash
# Check the actual test coverage for these methods
rg "getTreeByIndex|getTreeByID" -B 2 -A 2 Tests/Integration/TextIntegrationTests.swift Tests/Unit/Document/DocumentTests.swift

# Check the remaining checkWeight implementation and its usage
rg "checkWeight" -B 2 -A 2 Tests/Integration/TextIntegrationTests.swift Sources/Util/SplayTree.swift

Length of output: 3295

}

/**
Expand Down
4 changes: 2 additions & 2 deletions Tests/Integration/TextIntegrationTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,8 +238,8 @@ final class TextIntegrationTests: XCTestCase {
try await c2.sync()
try await c1.sync()

let d1Check = await(d1.getRoot().k1 as? JSONText)?.checkWeight() ?? false
let d2Check = await(d2.getRoot().k1 as? JSONText)?.checkWeight() ?? false
let d1Check = await(d1.getRoot().k1 as? JSONText)?.getTreeByIndex()?.checkWeight() ?? false
let d2Check = await(d2.getRoot().k1 as? JSONText)?.getTreeByIndex()?.checkWeight() ?? false
XCTAssertTrue(d1Check)
XCTAssertTrue(d2Check)
}
Expand Down
26 changes: 26 additions & 0 deletions Tests/Unit/Document/DocumentTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1056,6 +1056,32 @@ class DocumentTests: XCTestCase {
XCTAssertEqual(length, 0)
}

func test_should_purge_node_from_indexes_during_GC() async throws {
let doc = Document(key: "test-doc")

try await doc.update { root, _ in
root.k1 = JSONText()
}
var size = await(doc.getRoot().k1 as? JSONText)?.getTreeByID()?.size
XCTAssertEqual(size, 1)

try await doc.update { root, _ in
(root.k1 as? JSONText)?.edit(0, 0, "ABC")
}
size = await(doc.getRoot().k1 as? JSONText)?.getTreeByID()?.size
XCTAssertEqual(size, 2)

try await doc.update { root, _ in
(root.k1 as? JSONText)?.edit(1, 3, "")
}
size = await(doc.getRoot().k1 as? JSONText)?.getTreeByID()?.size
XCTAssertEqual(size, 3)

await doc.garbageCollect(TimeTicket.max)
size = await(doc.getRoot().k1 as? JSONText)?.getTreeByID()?.size
XCTAssertEqual(size, 2)
}

func test_escapes_string_for_object() async throws {
let target = Document(key: "test-doc")
try await target.update { root, _ in
Expand Down
Loading