-
Notifications
You must be signed in to change notification settings - Fork 98
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
Fix getGarbageLen
to retrun correct size
#714
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #714 +/- ##
=======================================
Coverage 81.75% 81.76%
=======================================
Files 57 57
Lines 4161 4163 +2
Branches 805 805
=======================================
+ Hits 3402 3404 +2
Misses 502 502
Partials 257 257 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution.
It seems like an intuitive solution. Because of Set, we use space for GC targets. but I'm curious if there are any ideas for further improvement.
CC) @justiceHui
Improved time complexity for Discussed this topic with @justiceHui . |
@hackerwins @justiceHui I expect the following code will pass the tests successfully. However, although Additionally, when running the test without reversing the childList, const benchmarkGCLen = (size: number) => {
const doc = new Document<{ a: any }>('test-doc');
doc.update((root) => {
let temp = root;
for (let i = 0; i < size; i++) {
temp.a = {};
temp = temp.a;
}
});
doc.update((root) => {
let temp = root;
const childList = [];
for (let i = 0; i < size; i++) {
temp.a = {};
childList.push(temp);
temp = temp.a;
}
for (const child of childList.reverse()) {
delete child.a;
}
});
const expect = 2 * size;
assert.equal(expect, doc.getGarbageLen());
assert.equal(expect, doc.garbageCollect(MaxTimeTicket));
}; |
This reverts commit 610516f.
@hackerwins As discussed during today's daily sync, I have restored the commit to the state where duplicate counting is eliminated using a Set. Regarding performance optimization, we agreed to proceed after @chacha912 submit the related issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! I left a brief comment.
And thanks for reporting the bug in the GC logic. I will create a separate issue for this bug.
I modified the benchmark code with @devleejb test for the worst-case scenario of In this scenario, const benchmarkSkewTreeGC = (size: number) => {
const doc = new Document<{ a: any }>('test-doc');
doc.update((root) => {
let temp = root;
const childList = [];
for (let i = 0; i < size; i++) {
temp.a = {};
childList.push(temp);
temp = temp.a;
}
for (const child of childList.reverse()) {
delete child.a;
}
});
assert.equal(size, doc.getGarbageLen());
assert.equal(size, doc.garbageCollect(MaxTimeTicket));
}; ![]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
What this PR does / why we need it?
getGarbageLen
function returns the correct length, I used a Set to eliminate duplicate countinggetGarbageLen
Any background context you want to provide?
Refer to issue yorkie/#688 for details.
What are the relevant tickets?
Fixes # yorkie-team/yorkie#688
Checklist