Implement Document.GarbageCollect in JS SDK
#101
Conversation
|
hmm, the test failed. Let me check the test cases! |
|
Thanks for the great work PR. I tested based on the yorkie-team/yorkie#100 issue. It seems that the same issue occurs as a result of testing. // document_test.ts
it('garbage collection test3', function () {
const doc = Document.create('test-col', 'test-doc');
assert.equal('{}', doc.toSortedJSON());
doc.update((root) => {
root['list'] = [1, 2, 3];
}, 'set list [1, 2, 3]');
assert.equal('{"list":[1,2,3]}', doc.toSortedJSON());
doc.update((root) => {
delete root['list'][1];
}, 'deletes index 1');
assert.equal('{"list":[1,3]}', doc.toSortedJSON());
assert.equal(1, doc.getGarbageLen());
assert.equal(1, doc.garbageCollect(MaxTimeTicket));
assert.equal(0, doc.getGarbageLen());
(doc.getRootForDebug().getObject().get('list') as JSONArray).printAnnotatedString();
(doc.getCloneRootForDebug().getObject().get('list') as JSONArray).printAnnotatedString();
// for test
assert.equal(0, 1);
});// document.ts
export class Document implements Observable<DocEvent> {
...
public getRootForDebug() {
return this.root;
}
public getCloneRootForDebug() {
return this.clone;
}
}// array.ts
export class JSONArray extends JSONContainer {
...
public printAnnotatedString() {
logger.debug(this.elements.getAnnotatedString();
}
}If you add test codes and temporary codes and run the test, you can see the following results. ...
LOG: 'YORKIE D: [1:000000000000000000000000:2:1][1:000000000000000000000000:4:3]'
Chrome Headless 86.0.4240.193 (Mac OS 10.15.7): Executed 11 of 42 SUCCESS (0 secs / 0.217 secs)
LOG: 'YORKIE D: [1:000000000000000000000000:2:1]{1:000000000000000000000000:3:2}[1:000000000000000000000000:4:3]'
Chrome Headless 86.0.4240.193 (Mac OS 10.15.7): Executed 11 of 42 SUCCESS (0 secs / 0.217 secs)
...If you look at the debug log, you can see that there are still nodes in the cloned root. If this happens, we believe that garbage collection at the language level will not be possible, resulting in a memory leak. I think we must solve this problem first. |
|
@dc7303 I think that's a good idea! Let me check the issue again! |
|
@dc7303 @hackerwins I'm thinking about adding a test for comparing cloned root and root in the gc tests. What's your take on it? |
|
@mojosoeun I think it's a good idea :) |
1. add a test for comparing cloned root and root 2. add a garbage collection test in yorkie_test 3. add a garbage collection with detached document test
There was a problem hiding this comment.
Thank you. I like the overall modification and approach. 👍 Leave a few comments.
about dist/yorkie.js
We deleted the dist folder from the PR below. Is there any reason to add dist?
098090f
1. delete dist folder 2. add a new line for comment in array.ts 3. change return type from IterableIterator<JSONElement> to void for getDescendants function
|
@hackerwins Thank you for reviewing my code! When it comes to the |
|
@dc7303 Thank you for reviewing my code! 🙏 when it comes to |
There was a problem hiding this comment.
Thank you for reflecting on the feedback. I left one more comment.
When it comes to comments, we can write to them next time(#98 is an issue that is intended to create a reference document for the user).
remove redundant code in array, object, root's getDescendants function
delete the given node from the heap
use findIndex instead of for loop
This reverts commit 6f06eea.
1. implement deleting a node from binary heap 2. add test cases for release function
There was a problem hiding this comment.
LGTM 👍
Thank you. While reviewing this PR, I learned how to delete nodes from the heap.
And we are adding the GC function to the Text. Can you review it together?
yorkie-team/yorkie#104
Codecov Report
@@ Coverage Diff @@
## master #101 +/- ##
==========================================
+ Coverage 72.02% 73.33% +1.30%
==========================================
Files 48 48
Lines 2481 2550 +69
Branches 396 409 +13
==========================================
+ Hits 1787 1870 +83
+ Misses 537 511 -26
- Partials 157 169 +12
Continue to review full report at Codecov.
|
* Implement `Document.GarbageCollect` in JS SDK * Run garbage collection for the clone * Add garbage collection tests * Add a test for comparing cloned root and root * Add a garbage collection test in yorkie_test * Add a garbage collection with detached document test * Add heap release function in-place way Co-authored-by: hackerwins <[email protected]>
What does this PR do?
Implement
Document.GarbageCollectin JS SDKHow should this be manually tested?
Any background context you want to provide?
I implemented this feature based on yorkie-team/yorkie#3. I tried to figure out how can I solve it. And after running
npm run build:proto, lots of files in api folder were updated automatically 🤔. I'm not sure I did it correctly, so please feel free to feedback about my code :)What are the relevant tickets?
Fixes #38
Checklist