Implement FinalizationRegistry #168
Conversation
saghul
force-pushed
the
finrec
branch
3 times, most recently
from
83492c8 to
1aa580b
Compare
|
@bnoordhuis This is ready now, PTAL! |
quickjs.c
Outdated
| list_for_each_safe(el, el1, &frd->entries) { | ||
| JSFinRecEntry *fre = list_entry(el, JSFinRecEntry, link); | ||
| delete_finrec_weakref(rt, fre); | ||
| } |
There was a problem hiding this comment.
Is this an accidentally-quadratic (well, O(n*m)) thing? Perhaps it's better to use a doubly linked list / circular queue so removal is O(1) and this loop stays O(n)?
I suppose the counterargument is that it makes every JSObject one almost-always-unused pointer bigger. And you know, that's actually a pretty good counterargument.
As an aside: you could use plain list_for_each here since the list isn't mutated. Not that I object to using the _safe variant, just mentioning it.
There was a problem hiding this comment.
Is this an accidentally-quadratic (well, O(n*m)) thing? Perhaps it's better to use a doubly linked list / circular queue so removal is O(1) and this loop stays O(n)?
I suppose the counterargument is that it makes every JSObject one almost-always-unused pointer bigger. And you know, that's actually a pretty good counterargument.
So here is the problem: you have a finalization registry where you did this:
finReg.register(target);
finReg.register(target, undefined, target);
The 2nd line is dumb, but allowed. The script reaches the end and the runtime is being destroyed. The finalization registry will go first.
Now as we destroy it, target will have a refcount of 1, but 2 weak ref entries.
Because we added the one with the token last it will be the first one to go. As its refcount reaches 0 it will be freed and we'll be left hanging on to freed memory as we continue iterating that list.
Since weakrefs don't seem to be very used I think keeping it simple, as in as small overhead as possible is a good compromise. A similar pattern is used in reset_weak_ref I reckon for a similar reason.
Thanks for the review!
As an aside: you could use plain list_for_each here since the list isn't mutated. Not that I object to using the _safe variant, just mentioning it.
Yeah, I kinda left it that way because it doesn't hurt...
No description provided.