-
Notifications
You must be signed in to change notification settings - Fork 177
Fix weakmap gc #1016
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 weakmap gc #1016
Conversation
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.
Can you walk me through how this fixes the bug? mr->value
is visited with or without your change, right?
api-test.c
Outdated
@@ -269,6 +269,32 @@ static void two_byte_string(void) | |||
JS_FreeRuntime(rt); | |||
} | |||
|
|||
static void weak_map_gc_check(void) | |||
{ | |||
const char *code = |
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.
const char *code = | |
static const char code[] = |
api-test.c
Outdated
JSValue ret = JS_Eval(ctx, code, strlen(code), "<input>", JS_EVAL_TYPE_GLOBAL); | ||
assert(!JS_IsException(ret)); | ||
JS_RunGC(rt); | ||
JSMemoryUsage memory_usage; | ||
JS_ComputeMemoryUsage(rt, &memory_usage); | ||
assert(memory_usage.memory_used_count < 10000); | ||
assert(memory_usage.memory_used_size < 1024 * 1024); |
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.
This can be made more precise by running the script once, recording the used memory, then running it a few more times, checking each time used memory stays the same.
quickjs.c
Outdated
if (s) { | ||
list_for_each(el, &s->records) { | ||
mr = list_entry(el, JSMapRecord, link); | ||
if (!s->is_weak) | ||
if (!s->is_weak) { | ||
JS_MarkValue(rt, mr->key, mark_func); | ||
JS_MarkValue(rt, mr->value, mark_func); | ||
JS_MarkValue(rt, mr->value, mark_func); | ||
} | ||
} | ||
} | ||
} |
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.
If you're going to do this, you can change the outer if (s) {
to if (s && !s->is_weak) {
but that turns the function into a no-op so you might as well remove it as the mark function for JS_CLASS_WEAKMAP and JS_CLASS_WEAKSET.
Thanks, I've updated the code as you suggested. Without this change: With this change:
|
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 with one final suggestion.
@@ -49058,8 +49078,7 @@ static void js_map_mark(JSRuntime *rt, JSValueConst val, | |||
if (s) { |
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.
if (s) { | |
if (s) { | |
assert(!s->is_weak); |
There seems to be a new bug in the |
I'm sorry, There are no bugs. |
Thanks, merged. |
The values in weak map should be mark as reachable only when it's key is reachable.
Fix #1015