Skip to content

Commit

Permalink
Remove node from indexes during GC (#860)
Browse files Browse the repository at this point in the history
While improving GC structure, we missed removing nodes from internal
indexes of Text when purging tombstones(yorkie-team/yorkie#866).
  • Loading branch information
hackerwins authored Jul 4, 2024
1 parent 6dfd75d commit d0b3dc3
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 15 deletions.
17 changes: 13 additions & 4 deletions src/document/crdt/rga_tree_split.ts
Original file line number Diff line number Diff line change
Expand Up @@ -660,11 +660,17 @@ export class RGATreeSplit<T extends RGATreeSplitValue> implements GCParent {
}

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

/**
* `getTreeByID` returns the tree by ID for debugging purpose.
*/
public checkWeight(): boolean {
return this.treeByIndex.checkWeight();
public getTreeByID(): LLRBTree<RGATreeSplitNodeID, RGATreeSplitNode<T>> {
return this.treeByID;
}

/**
Expand Down Expand Up @@ -1012,6 +1018,9 @@ export class RGATreeSplit<T extends RGATreeSplitValue> implements GCParent {
* `purge` physically purges the given node from RGATreeSplit.
*/
public purge(node: RGATreeSplitNode<T>): void {
this.treeByIndex.delete(node);
this.treeByID.remove(node.getID());

const prev = node.getPrev();
const next = node.getNext();
const insPrev = node.getInsPrev();
Expand Down
20 changes: 16 additions & 4 deletions src/document/crdt/text.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,16 @@ import { CRDTElement } from '@yorkie-js-sdk/src/document/crdt/element';
import {
RGATreeSplit,
RGATreeSplitNode,
RGATreeSplitNodeID,
RGATreeSplitPosRange,
ValueChange,
} from '@yorkie-js-sdk/src/document/crdt/rga_tree_split';
import { escapeString } from '@yorkie-js-sdk/src/document/json/strings';
import { parseObjectValues } from '@yorkie-js-sdk/src/util/object';
import type * as Devtools from '@yorkie-js-sdk/src/devtools/types';
import { GCChild, GCPair } from '@yorkie-js-sdk/src/document/crdt/gc';
import { SplayTree } from '@yorkie-js-sdk/src/util/splay_tree';
import { LLRBTree } from '@yorkie-js-sdk/src/util/llrb_tree';

/**
* `TextChangeType` is the type of TextChange.
Expand Down Expand Up @@ -361,11 +364,20 @@ export class CRDTText<A extends Indexable = Indexable> extends CRDTElement {
}

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

/**
* `getTreeByID` returns the tree by ID for debugging.
*/
public checkWeight(): boolean {
return this.rgaTreeSplit.checkWeight();
public getTreeByID(): LLRBTree<
RGATreeSplitNodeID,
RGATreeSplitNode<CRDTTextValue>
> {
return this.rgaTreeSplit.getTreeByID();
}

/**
Expand Down
27 changes: 22 additions & 5 deletions src/document/json/text.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,22 @@ import {
} from '@yorkie-js-sdk/src/document/time/ticket';
import { ChangeContext } from '@yorkie-js-sdk/src/document/change/context';
import {
RGATreeSplitNode,
RGATreeSplitNodeID,
RGATreeSplitPos,
RGATreeSplitPosRange,
} from '@yorkie-js-sdk/src/document/crdt/rga_tree_split';
import { CRDTText, TextValueType } from '@yorkie-js-sdk/src/document/crdt/text';
import {
CRDTText,
CRDTTextValue,
TextValueType,
} from '@yorkie-js-sdk/src/document/crdt/text';
import { EditOperation } from '@yorkie-js-sdk/src/document/operation/edit_operation';
import { StyleOperation } from '@yorkie-js-sdk/src/document/operation/style_operation';
import { stringifyObjectValues } from '@yorkie-js-sdk/src/util/object';
import type * as Devtools from '@yorkie-js-sdk/src/devtools/types';
import { SplayTree } from '@yorkie-js-sdk/src/util/splay_tree';
import { LLRBTree } from '@yorkie-js-sdk/src/util/llrb_tree';

/**
* `TextPosStruct` represents the structure of RGATreeSplitPos.
Expand Down Expand Up @@ -256,11 +264,20 @@ export class Text<A extends Indexable = Indexable> {
}

/**
* `checkWeight` returns false when there is an incorrect weight node.
* for debugging purpose.
* `getTreeByIndex` returns IndexTree of the text for testing purpose.
*/
public getTreeByIndex(): SplayTree<CRDTTextValue> {
return this.text!.getTreeByIndex();
}

/**
* `getTreeByID` returns IDTree of the text for testing purpose.
*/
public checkWeight(): boolean {
return this.text!.checkWeight();
public getTreeByID(): LLRBTree<
RGATreeSplitNodeID,
RGATreeSplitNode<CRDTTextValue>
> {
return this.text!.getTreeByID();
}

/**
Expand Down
4 changes: 2 additions & 2 deletions test/integration/text_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -405,8 +405,8 @@ describe('Text', function () {
await c2.sync();
await c1.sync();

// assert.isOk(d1.getRoot().k1.checkWeight());
// assert.isOk(d2.getRoot().k1.checkWeight());
assert.isOk(d1.getRoot().k1.getTreeByIndex().checkWeight());
assert.isOk(d2.getRoot().k1.getTreeByIndex().checkWeight());
}, task.name);
});
});
Expand Down
15 changes: 15 additions & 0 deletions test/unit/document/document_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1238,6 +1238,21 @@ describe.sequential('Document', function () {
assert.equal(0, doc.getGarbageLen());
});

it('should purge node from indexes during GC', function () {
const doc = new Document<{ k1: Text }>('test-doc');
doc.update((root) => (root.k1 = new Text()));
assert.equal(doc.getRoot().k1.getTreeByID().size(), 1);

doc.update((root) => root.k1.edit(0, 0, 'ABC'));
assert.equal(doc.getRoot().k1.getTreeByID().size(), 2);

doc.update((root) => root.k1.edit(1, 3, ''));
assert.equal(doc.getRoot().k1.getTreeByID().size(), 3);

doc.garbageCollect(MaxTimeTicket);
assert.equal(doc.getRoot().k1.getTreeByID().size(), 2);
});

it('should handle escape string for strings containing single quotes', function () {
const doc = new Document<{ [key: string]: any }>('test-doc');
doc.update((root) => (root.str = `I'm yorkie`));
Expand Down

0 comments on commit d0b3dc3

Please sign in to comment.