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

Apply GCPair to TreeNode, TextNode #819

Merged
merged 9 commits into from
May 24, 2024
Merged

Apply GCPair to TreeNode, TextNode #819

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d851b4a
Apply GCPair
raararaara May 23, 2024
31d6359
Cleanup code
raararaara May 23, 2024
22678e3
Fix missing logic in rga.GCPairs()
raararaara May 23, 2024
dfc74f7
Revert rga.canDelete() condition
raararaara May 23, 2024
a5ebd2f
Revise codes
hackerwins May 23, 2024
5750dbe
Refactored purge method to use specific type
raararaara May 23, 2024
9bb9a91
Merge remote-tracking branch 'origin/apply-gc' into apply-gc
raararaara May 23, 2024
4eefe91
Add tests for correct gc count cases
raararaara May 24, 2024
34efeea
Remove only
raararaara May 24, 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
9 changes: 0 additions & 9 deletions src/document/change/context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import { CRDTRoot } from '@yorkie-js-sdk/src/document/crdt/root';
import {
CRDTContainer,
CRDTElement,
CRDTGCElement,
} from '@yorkie-js-sdk/src/document/crdt/element';
import { Operation } from '@yorkie-js-sdk/src/document/operation/operation';
import { ChangeID } from '@yorkie-js-sdk/src/document/change/change_id';
Expand Down Expand Up @@ -103,14 +102,6 @@ export class ChangeContext<P extends Indexable = Indexable> {
this.root.registerRemovedElement(deleted);
}

/**
* `registerElementHasRemovedNodes` register GC element has removed node for
* garbage collection.
*/
public registerElementHasRemovedNodes(elem: CRDTGCElement): void {
this.root.registerElementHasRemovedNodes(elem);
}

/**
* `registerGCPair` registers the given pair to hash table.
*/
hackerwins marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
8 changes: 0 additions & 8 deletions src/document/crdt/element.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,11 +157,3 @@ export abstract class CRDTContainer extends CRDTElement {
*/
abstract getByID(createdAt: TimeTicket): CRDTElement | undefined;
}

/**
* `CRDTGCElement` represents element which has garbage collecting method.
*/
export abstract class CRDTGCElement extends CRDTElement {
abstract getRemovedNodesLen(): number;
abstract purgeRemovedNodesBefore(ticket: TimeTicket): number;
}
hackerwins marked this conversation as resolved.
Show resolved Hide resolved
116 changes: 57 additions & 59 deletions src/document/crdt/rga_tree_split.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
TimeTicket,
TimeTicketStruct,
} from '@yorkie-js-sdk/src/document/time/ticket';
import { GCChild, GCPair, GCParent } from '@yorkie-js-sdk/src/document/crdt/gc';

export interface ValueChange<T> {
actor: ActorID;
Expand Down Expand Up @@ -243,9 +244,10 @@ export type RGATreeSplitPosRange = [RGATreeSplitPos, RGATreeSplitPos];
/**
* `RGATreeSplitNode` is a node of RGATreeSplit.
*/
export class RGATreeSplitNode<
T extends RGATreeSplitValue,
> extends SplayNode<T> {
export class RGATreeSplitNode<T extends RGATreeSplitValue>
extends SplayNode<T>
implements GCChild
{
private id: RGATreeSplitNodeID;
private removedAt?: TimeTicket;

Expand Down Expand Up @@ -438,10 +440,15 @@ export class RGATreeSplitNode<
* `canDelete` checks if node is able to delete.
*/
public canDelete(editedAt: TimeTicket, maxCreatedAt: TimeTicket): boolean {
return (
const justRemoved = !this.removedAt;
if (
!this.getCreatedAt().after(maxCreatedAt) &&
(!this.removedAt || editedAt.after(this.removedAt))
);
) {
return justRemoved;
}

Comment on lines +443 to +450
Copy link

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The logic in the canDelete method in src/document/crdt/rga_tree_split.ts appears to be different from the one in src/document/crdt/tree.ts. Specifically, the justRemoved flag is used in the former but not in the latter. This discrepancy suggests that the justRemoved flag might be incorrectly used or unnecessary.

  • src/document/crdt/rga_tree_split.ts:

    public canDelete(editedAt: TimeTicket, maxCreatedAt: TimeTicket): boolean {
  const justRemoved = !this.removedAt;
  if (
    !this.getCreatedAt().after(maxCreatedAt) &&
    (!this.removedAt || editedAt.after(this.removedAt))
  ) {
    return justRemoved;
  }
  return false;
}

  • src/document/crdt/tree.ts:

    public canDelete(editedAt: TimeTicket, maxCreatedAt: TimeTicket): boolean {
  return (
    !this.getCreatedAt().after(maxCreatedAt) &&
    (!this.removedAt || editedAt.after(this.removedAt))
  );
}

The justRemoved flag in src/document/crdt/rga_tree_split.ts might be causing unintended behavior. Please review the necessity of this flag and consider aligning the logic with the implementation in src/document/crdt/tree.ts.

  • src/document/crdt/rga_tree_split.ts: Review the use of justRemoved in the canDelete method.
Analysis chain

Clarify the logic in canDelete method.

The logic in canDelete seems to be using a justRemoved flag incorrectly. Please verify if this is intended or if there's a better way to handle the deletion logic.

Scripts Executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the logic of `canDelete` method in `RGATreeSplitNode` class.

# Test: Search for usage of `canDelete` method. Expect: Correct logic handling in all usages.
rg --type typescript $'canDelete'

Length of output: 67

Script:

#!/bin/bash
# Description: Verify the logic of `canDelete` method in `RGATreeSplitNode` class.

# Test: Search for usage of `canDelete` method. Expect: Correct logic handling in all usages.
rg 'canDelete' --glob '*.ts'

Length of output: 577

Script:

#!/bin/bash
# Description: Extract the definitions and usages of the `canDelete` method in `RGATreeSplitNode` class.

# Extract the definition of the `canDelete` method in `src/document/crdt/tree.ts`
rg 'public canDelete' -A 10 src/document/crdt/tree.ts

# Extract the definition of the `canDelete` method in `src/document/crdt/rga_tree_split.ts`
rg 'public canDelete' -A 10 src/document/crdt/rga_tree_split.ts

# Extract the usages of the `canDelete` method in `src/document/crdt/tree.ts`
rg 'canDelete' -A 5 src/document/crdt/tree.ts

# Extract the usages of the `canDelete` method in `src/document/crdt/rga_tree_split.ts`
rg 'canDelete' -A 5 src/document/crdt/rga_tree_split.ts

Length of output: 1818

return false;
}

/**
Expand Down Expand Up @@ -491,6 +498,13 @@ export class RGATreeSplitNode<
this.value = value.substring(0, offset) as T;
return value.substring(offset, value.length) as T;
}

/**
* `toIDString` returns a string that can be used as an ID for this position.
*/
public toIDString(): string {
return this.id.toIDString();
}
}

/**
Expand All @@ -500,17 +514,15 @@ export class RGATreeSplitNode<
* the block is split.
*
*/
export class RGATreeSplit<T extends RGATreeSplitValue> {
export class RGATreeSplit<T extends RGATreeSplitValue> implements GCParent {
private head: RGATreeSplitNode<T>;
private treeByIndex: SplayTree<T>;
private treeByID: LLRBTree<RGATreeSplitNodeID, RGATreeSplitNode<T>>;
private removedNodeMap: Map<string, RGATreeSplitNode<T>>;

constructor() {
this.head = RGATreeSplitNode.create(InitialRGATreeSplitNodeID);
this.treeByIndex = new SplayTree();
this.treeByID = new LLRBTree(RGATreeSplitNode.createComparator());
this.removedNodeMap = new Map();

this.treeByIndex.insert(this.head);
this.treeByID.put(this.head.getID(), this.head);
Expand Down Expand Up @@ -540,15 +552,23 @@ export class RGATreeSplit<T extends RGATreeSplitValue> {
editedAt: TimeTicket,
value?: T,
maxCreatedAtMapByActor?: Map<string, TimeTicket>,
): [RGATreeSplitPos, Map<string, TimeTicket>, Array<ValueChange<T>>] {
): [
RGATreeSplitPos,
Map<string, TimeTicket>,
Array<GCPair>,
Array<ValueChange<T>>,
] {
// 01. split nodes with from and to
const [toLeft, toRight] = this.findNodeWithSplit(range[1], editedAt);
const [fromLeft, fromRight] = this.findNodeWithSplit(range[0], editedAt);

// 02. delete between from and to
const nodesToDelete = this.findBetween(fromRight, toRight);
const [changes, maxCreatedAtMap, removedNodeMapByNodeKey] =
this.deleteNodes(nodesToDelete, editedAt, maxCreatedAtMapByActor);
const [changes, maxCreatedAtMap, removedNodes] = this.deleteNodes(
nodesToDelete,
editedAt,
maxCreatedAtMapByActor,
);

const caretID = toRight ? toRight.getID() : toLeft.getID();
let caretPos = RGATreeSplitPos.of(caretID, 0);
Expand Down Expand Up @@ -580,11 +600,12 @@ export class RGATreeSplit<T extends RGATreeSplitValue> {
}

// 04. add removed node
for (const [key, removedNode] of removedNodeMapByNodeKey) {
this.removedNodeMap.set(key, removedNode);
const pairs: Array<GCPair> = [];
for (const [, removedNode] of removedNodes) {
pairs.push({ parent: this, child: removedNode });
}

return [caretPos, maxCreatedAtMap, changes];
return [caretPos, maxCreatedAtMap, pairs, changes];
}

/**
Expand Down Expand Up @@ -988,58 +1009,35 @@ export class RGATreeSplit<T extends RGATreeSplitValue> {
}

/**
* `getRemovedNodesLen` returns size of removed nodes.
* `purge` physically purges the given node from RGATreeSplit.
*/
public getRemovedNodesLen(): number {
return this.removedNodeMap.size;
}
public purge(node: GCChild): void {
if (node instanceof RGATreeSplitNode<T>) {
const prev = node.getPrev();
const next = node.getNext();
const insPrev = node.getInsPrev();
const insNext = node.getInsNext();

/**
* `purgeRemovedNodesBefore` physically purges nodes that have been removed.
*/
public purgeRemovedNodesBefore(ticket: TimeTicket): number {
let count = 0;
for (const [, node] of this.removedNodeMap) {
if (node.getRemovedAt() && ticket.compare(node.getRemovedAt()!) >= 0) {
this.treeByIndex.delete(node);
this.purge(node);
this.treeByID.remove(node.getID());
this.removedNodeMap.delete(node.getID().toIDString());
count++;
if (prev) {
prev.setNext(next);
}
if (next) {
next.setPrev(prev);
}
}

return count;
}

/**
* `purge` physically purges the given node from RGATreeSplit.
*/
public purge(node: RGATreeSplitNode<T>): void {
const prev = node.getPrev();
const next = node.getNext();
const insPrev = node.getInsPrev();
const insNext = node.getInsNext();

if (prev) {
prev.setNext(next);
}
if (next) {
next.setPrev(prev);
}
node.setPrev(undefined);
node.setNext(undefined);

node.setPrev(undefined);
node.setNext(undefined);
if (insPrev) {
insPrev.setInsNext(insNext);
}

if (insPrev) {
insPrev.setInsNext(insNext);
}
if (insNext) {
insNext.setInsPrev(insPrev);
}

if (insNext) {
insNext.setInsPrev(insPrev);
node.setInsPrev(undefined);
node.setInsNext(undefined);
}

node.setInsPrev(undefined);
node.setInsNext(undefined);
}
}
hackerwins marked this conversation as resolved.
Show resolved Hide resolved
58 changes: 17 additions & 41 deletions src/document/crdt/root.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,11 @@ import {
import {
CRDTContainer,
CRDTElement,
CRDTGCElement,
} from '@yorkie-js-sdk/src/document/crdt/element';
import { CRDTObject } from '@yorkie-js-sdk/src/document/crdt/object';
import { GCPair } from '@yorkie-js-sdk/src/document/crdt/gc';
import { CRDTText } from '@yorkie-js-sdk/src/document/crdt/text';
import { CRDTTree } from '@yorkie-js-sdk/src/document/crdt/tree';

/**
* `CRDTElementPair` is a structure that represents a pair of element and its
Expand Down Expand Up @@ -64,13 +65,6 @@ export class CRDTRoot {
*/
private removedElementSetByCreatedAt: Set<string>;

/**
* `elementHasRemovedNodesSetByCreatedAt` is a hash set that contains the
* creation time of the element that has removed nodes. It is used to find
* the element that has removed nodes when executing garbage collection.
*/
private elementHasRemovedNodesSetByCreatedAt: Set<string>;

/**
* `gcPairMap` is a hash table that maps the IDString of GCChild to the
* element itself and its parent.
Expand All @@ -81,9 +75,20 @@ export class CRDTRoot {
this.rootObject = rootObject;
this.elementPairMapByCreatedAt = new Map();
this.removedElementSetByCreatedAt = new Set();
this.elementHasRemovedNodesSetByCreatedAt = new Set();
this.gcPairMap = new Map();
this.registerElement(rootObject, undefined);

rootObject.getDescendants((elem) => {
if (elem.getRemovedAt()) {
this.registerRemovedElement(elem);
}
if (elem instanceof CRDTText || elem instanceof CRDTTree) {
for (const pair of elem.GCPairs()) {
this.registerGCPair(pair);
}
}
return false;
});
}

/**
Expand Down Expand Up @@ -197,16 +202,6 @@ export class CRDTRoot {
this.removedElementSetByCreatedAt.add(element.getCreatedAt().toIDString());
}

/**
* `registerElementHasRemovedNodes` registers the given GC element to the
* hash set.
*/
public registerElementHasRemovedNodes(elem: CRDTGCElement): void {
this.elementHasRemovedNodesSetByCreatedAt.add(
elem.getCreatedAt().toIDString(),
);
}

/**
* `registerGCPair` registers the given pair to hash table.
*/
Expand Down Expand Up @@ -261,12 +256,6 @@ export class CRDTRoot {

count += seen.size;

for (const createdAt of this.elementHasRemovedNodesSetByCreatedAt) {
const pair = this.elementPairMapByCreatedAt.get(createdAt)!;
const elem = pair.element as CRDTGCElement;
count += elem.getRemovedNodesLen();
}

count += this.gcPairMap.size;

return count;
Expand Down Expand Up @@ -296,27 +285,14 @@ export class CRDTRoot {
}
}

for (const createdAt of this.elementHasRemovedNodesSetByCreatedAt) {
const pair = this.elementPairMapByCreatedAt.get(createdAt)!;
const elem = pair.element as CRDTGCElement;

const removedNodeCnt = elem.purgeRemovedNodesBefore(ticket);
if (elem.getRemovedNodesLen() === 0) {
this.elementHasRemovedNodesSetByCreatedAt.delete(
elem.getCreatedAt().toIDString(),
);
}
count += removedNodeCnt;
}

for (const [, pair] of this.gcPairMap) {
const removedAt = pair.child.getRemovedAt();
if (removedAt !== undefined && ticket.compare(removedAt) >= 0) {
pair.parent.purge(pair.child);
}

this.gcPairMap.delete(pair.child.toIDString());
count += 1;
this.gcPairMap.delete(pair.child.toIDString());
count += 1;
}
}

return count;
hackerwins marked this conversation as resolved.
Show resolved Hide resolved
hackerwins marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Loading
Loading