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

Duplicate deregister in Nested Object GC #715

Open
chacha912 opened this issue Jan 4, 2024 · 0 comments
Open

Duplicate deregister in Nested Object GC #715

chacha912 opened this issue Jan 4, 2024 · 0 comments
Labels
bug 🐞 Something isn't working good first issue 🐤 Good for newcomers sdk ⚒️

Comments

@chacha912
Copy link
Contributor

What happened:

When performing garbage collection (GC) on nested objects, the deregister process leads to redundant operations on child nodes. This issue arises because the deregister iterates over children, while getDescendants also performs a similar traversal, resulting in unnecessary duplication.

/**
* `deregisterElement` deregister the given element and its descendants from hash table.
*/
public deregisterElement(element: CRDTElement): number {
let count = 0;
const deregisterElementInternal = (elem: CRDTElement) => {
const createdAt = elem.getCreatedAt().toIDString();
this.elementPairMapByCreatedAt.delete(createdAt);
this.removedElementSetByCreatedAt.delete(createdAt);
count++;
if (elem instanceof CRDTContainer) {
elem.getDescendants((e) => {
deregisterElementInternal(e);
return false;
});
}
};
deregisterElementInternal(element);
return count;
}

/**
* `getDescendants` returns the descendants of this object by traversing.
*/
public getDescendants(
callback: (elem: CRDTElement, parent: CRDTContainer) => boolean,
): void {
for (const node of this.memberNodes) {
const element = node.getValue();
if (callback(element, this)) {
return;
}
if (element instanceof CRDTContainer) {
element.getDescendants(callback);
}
}
}

What you expected to happen:

Garbage collection should efficiently iterate over children only once, deregistering them, and returning the accurate count of garbage-collected nodes.

How to reproduce it (as minimally and precisely as possible):

Execute the following test code:

it.only('test', async function ({ task }) {
  type TestDoc = { shape?: { point?: { x?: number; y?: number } } };
  const docKey = toDocKey(`${task.name}-${new Date().getTime()}`);
  const doc = new yorkie.Document<TestDoc>(docKey);

  doc.update((root) => {
    root.shape = { point: { x: 0, y: 0 } };
    delete root.shape;
  });
  assert.equal(doc.getGarbageLen(), 4); // shape, point, x, y
  assert.equal(doc.garbageCollect(MaxTimeTicket), 4); // The number of GC nodes must also be 4. (It's 6 for now.)
});

Anything else we need to know?:

This issue was reported in #714 .
The deregisterElements logic has been modified in #691 .

Environment:

  • Operating system:
  • Browser and version:
  • Yorkie version (use yorkie version): v0.4.11
  • Yorkie JS SDK version: v0.4.11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working good first issue 🐤 Good for newcomers sdk ⚒️
Projects
None yet
Development

No branches or pull requests

2 participants