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.

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
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
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
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
82 changes: 39 additions & 43 deletions src/document/crdt/rga_tree_split.ts
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 All @@ -533,22 +545,30 @@ export class RGATreeSplit<T extends RGATreeSplitValue> {
* @param editedAt - edited time
* @param value - value
* @param maxCreatedAtMapByActor - maxCreatedAtMapByActor
* @returns `[RGATreeSplitPos, Map<string, TimeTicket>, Array<Change>]`
* @returns `[RGATreeSplitPos, Map<string, TimeTicket>, Array<GCPair>, Array<Change>]`
*/
public edit(
range: RGATreeSplitPosRange,
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 @@ -865,7 +886,7 @@ export class RGATreeSplit<T extends RGATreeSplitValue> {
);

const createdAtMapByActor = new Map();
const removedNodeMap = new Map();
const removedNodes = new Map();
// First we need to collect indexes for change.
const changes = this.makeChanges(nodesToKeep, editedAt);
for (const node of nodesToDelete) {
Expand All @@ -877,13 +898,13 @@ export class RGATreeSplit<T extends RGATreeSplitValue> {
) {
createdAtMapByActor.set(actorID, node.getID().getCreatedAt());
}
removedNodeMap.set(node.getID().toIDString(), node);
removedNodes.set(node.getID().toIDString(), node);
node.remove(editedAt);
}
// Finally remove index nodes of tombstones.
this.deleteIndexNodes(nodesToKeep);

return [changes, createdAtMapByActor, removedNodeMap];
return [changes, createdAtMapByActor, removedNodes];
}

private filterNodes(
Expand Down Expand Up @@ -987,31 +1008,6 @@ export class RGATreeSplit<T extends RGATreeSplitValue> {
}
}

/**
* `getRemovedNodesLen` returns size of removed nodes.
*/
public getRemovedNodesLen(): number {
return this.removedNodeMap.size;
}

/**
* `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++;
}
}

return count;
}

/**
* `purge` physically purges the given node from RGATreeSplit.
*/
Expand Down
1 change: 1 addition & 0 deletions src/document/crdt/rht.ts
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,7 @@ export class RHT {
public purge(child: RHTNode) {
const node = this.nodeMapByKey.get(child.getKey());
if (node == undefined || node.toIDString() != child.toIDString()) {
// TODO(hackerwins): Should we return an error when the child is not found?
return;
}

raararaara marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Loading
Loading