-
Notifications
You must be signed in to change notification settings - Fork 233
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
Reference count virtual object property references for GC #3732
Conversation
I think this also will close #3149 |
I'm still picking through the case analysis, but this seems to be a pretty solid implementation of the original approach. However, #3649 (comment) and #3649 (comment) , and our conversation about GC-provoked metering divergence whenever the refcounting code looks at the The specific sequence I'm concerned about would be:
So that's a recipe for not only metering variance that depends on the GC state, but also syscall variance. I think the fix is to be more conservative with deletion. You made an excellent start by introducing My original This would parallel the way the kernel and comms GC works. The kernel (in So the task (maybe as part of this PR, maybe as a follow-on PR) is to change liveslots and VOM.js:
In general, the interface between If the VOM drops a pillar itself (i.e. deletion of virtualized data), the vref is added to |
Related to #3649 (comment), I'm not sure I follow the current usage of WeakRefs. IMO those should be In my mind, most code would consider the Representative to be available, until declared dead by So my question is, what code outside of BOYD and the "get representative" need to know about the collected state of a representative, and why? |
a0b13ee
to
3b896a1
Compare
@mhofman recently taught me something about FinalizationRegistry that I didn't know: callbacks are cancelled if you unregister the object. So if object A is registered, then collected (nominally queueing a finalizer callback), then We need to defend against the "re-introduction case" (A is imported and registered, A is dropped, A is collected, A gets reimported before the finalizer gets to run, now if the finalizer runs it might delete the live I think @mhofman is probably right and we can simplify the GC implementation. I need to think through the approach, though. We still have the same REACHABLE -> UNREACHABLE -> COLLECTED -> FINALIZED -> GONE sequence, but if a vref is reintroduced while in the COLLECTED state (i.e. after polling the WeakRef would return But, let's see, there's still a gap between the finalizer running and I think we could do that, but.. how should that interact with e.g. a virtual object which just lost one of the other two pillars? The (abstract) virtual object is kept alive by any of three pillars: an in-memory Implementation wise, the VOM will notice when e.g. the refcount pillar is gone (the refcount drops to zero), and that needs to move the VO state one step closer to being deleted. At that point we need to know the status of the So instead of polling the WeakRef at that point, I guess we could afford to have an in-memory Set of all the vrefs for which we know Representatives exist (probably Presences too). We'd add a vref to the set upon creation of the Presence/Representative, and we'd remove them when the finalizer fires, and we'd rely upon The thing I can't get my head around is the polarity of that set. If we're polling the WeakRef, then I can see the right architecture:
But if we track a Set of deleted So without deeper analysis, it feels to me like the easiest thing to do is to poll the WeakRef during |
What you describe here is what is actually implemented modulo the unlanded PRs (i.e., it's what we'll have once these PRs are in). The dance is otherwise very delicate, and I'm nervous about error cases slipping through if we try to be too clever. The issue, to me, is that the finalizer is triggered by the GC of the in-memory object, but the finalizer itself operates on the vref, which might, at the time it runs, now refer to a different in-memory object -- this is one reason for the "possibly" part of the |
Yeah, the unregister token should always be the (string) vref. We'd unregister it when deserialization needs to re-introduce the And the finalizer callback is still probably best place to delete the now-useless
If we switch to relying upon unregistration nullifying pending finalizer callbacks, I think the main thing that would change is step 2. That WeakRef could only be alive if the vref was reintroduced, and the reintroduction code would unregister the finalizer, so the callback would never run. Therefore the callback should never observe the WeakRef to be empty, and we could skip step two, assume the WeakRef is empty, and unconditionally delete But, 1: that doesn't help us make metering more consistent (polling the WeakRef makes code GC-sensitive, but we're already dependent upon having finalizers only run inside the unmetered box, because the computrons used by the callback would also be GC-sensitive), and 2: it's a cheap thing to check. So, if we can feel confident in getting |
Unregister tokens are required to be objects, so this won't work. |
Oh. hm. |
Ah yeah, that's partially my fault as I found a bug in v8 with That said the answer to your pickle is probably also in that thread: use the Regarding the broader issue, I'm still having a hard time following, since I'm not familiar enough with the actual implemenation yet. Do I understand correctly that Would the following work (pseudo code, not using existing code): const vrefToWeakRef = new Map();
const registry = new FinalizationRegistry((vref) => {
vrefToWeakRef.set(vref, null);
});
const getRepresentative = (vref) => {
let rep;
let weakRef = vrefToWeakRef.get(vref);
if (weakRef) {
rep = weakRef.deref();
if (rep) {
return rep;
} else {
registry.unregister(weakRef);
vrefToWeakRef.set(vref, null);
}
}
rep = makeRepresentative(vref);
weakRef = new WeakRef(rep);
registry.register(rep, vref, weakRef);
vrefToWeakRef.set(vref, weakRef);
};
const checkHasPillar = (vref) => {
if (!vrefToWeakRef.has(vref) && refcount === 0 && !hasExport) {
prune(vref);
}
}
const bringOutYourDead = () => {
for (const [vref, weakRef] of vrefToWeakRef.entries()) {
if (!weakRef) {
vrefToWeakRef.delete(vref);
checkHasPillar(vref);
}
}
}; That way:
|
Interesting..
Correct, because our resource requirement is that RAM usage can be O(N) in the number of live
Roughly, yes. The const registry = new FinalizationRegistry((vref) => {
vrefToWeakRef.set(vref, null);
}); because that callback should never get run after a re-introduction. This loop: const bringOutYourDead = () => {
for (const [vref, weakRef] of vrefToWeakRef.entries()) { is too expensive, we use a set of possibly-killed vrefs instead of walking the entire table. Let's see, with this approach, the REACHABLE/UNREACHABLE/COLLECTED/FINALIZED state transition digram looks basically like:
To make it work properly, we'd need to integrate some pieces from the existing code, like:
But yeah, I think that handles the reintroduction part properly. |
Yeah I knew I was gonna get dinged on that, but figured it'd be easy to reintroduce a separate "deadSet", which has the same content as
To make sure:
Basically everywhere |
Btw, we should test that the engines we actually run under are not broken re this. Here is what I wrote a couple years ago (before the switch to individual callbacks and registry name change): https://github.com/mhofman/tc39-weakrefs-shim/blob/3cd6ff1910bc884937396ab12d59e1d4b55e2932/src/tests/FinalizationGroup.shared.ts#L534-L572 Idea is to register 2 cells in registry for the same object, with the unregister token of the other as Edit: It's an easy one to mess up, SpiderMonkey did before I reported it to them: https://bugzilla.mozilla.org/show_bug.cgi?id=1600488 |
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.
Ok, looks pretty good but a couple of changes to make.
I reviewed this by locally rebasing 3547-vom-weak-key-gc
onto current trunk, then rebasing this PR's vom-vprop-ref-counting
on top of that, then reading the result. You'll need to do something similar. You might consider making the recommended changes on this branch, then land this PR onto its parent branch (closing this PR), then rebasing the parent branch to current trunk, then landing the other PR.
assert(type === 'object', `unprepared to track ${type}`); | ||
if (virtual) { | ||
// Representative: send nothing, but perform refcount checking | ||
if (slotToVal.get(vref)) { |
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.
I think this slotToVal
is redundant: vrefs only get here if they were in deadSet
, which starts the loop empty and only grows with things that passed the if !getValForSlot
check earlier. Also/although the difference between slotToVal.get
(which returns true
if there's an empty WeakRef) and getValForSlot
(which returns false
) makes that not so clear cut. But I think that's not a difference this code is supposed to care about.
We do need to make sure the dead WeakRef is cleaned up somewhere. I see a relevant slotToVal.delete
in retireOneExport
, but I'm not sure we're doing the cleanup for imported Presences. I bet that should be added to the finalizer callback.
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.
I don't think that's right. The !getValForSlot
check several lines above tests that there is no in-memory object (i.e., it's been GC'd and finalized), so when we reach this point the test of slotToVal
is telling us that even though the WeakRef is cleared, the WeakRef itself is still there, i.e., there's still a mapping from the vref that might need to be cleaned up, which is what this code does. It is important that the finalizer callback only add to possiblyDeadSet
and not do any kind of cleanup, else we might introduce visible nondeterminism.
} | ||
} | ||
} | ||
} while (possiblyDeadSet.size > 0 || possiblyRetiredSet.size > 0 || doMore); |
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.
I don't know that it's worth addressing now, but as a performance improvement, I think we don't need to repeat the gcAndFinalize
unless doMore
tells us that an in-memory reference (e.g. a key of VOM.remotableRefCounts
) was dropped. If possiblyDeadSet
/possiblyRetiredSet
have grown, we need to rescan them, but we have no reason to think that the RAM object graph has changed, so we don't need to spend the time doing another full GC sweep.
To implement that, I guess we'd add a flag like gcNeedsCollection
, set it to true
upon entry to scanForDeadObjects
, and at the top of the loop do
if (gcNeedsCollection) {
await gcAndFinalize();
gcNeedsCollection = false;
}
and if maybeFree
is true, set both doMore
and gcNeedsCollection
.
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.
Seems plausible. Sounds like something that could be easy to mess up, so not for this round of changes, but we should make a note...
slotToVal.delete(vref); | ||
// console.log(`-- adding ${vref} to deadSet`); | ||
// eslint-disable-next-line no-use-before-define | ||
addToPossiblyDeadSet(vref); |
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 needs to take responsibility for deleting the dead WeakRef, so something like:
if (wr) {
if (!wr.deref()) {
slotToVal.delete(vref);
addToPossiblyDeadSet(vref);
}
} else {
addToPossiblyDeadSet(vref);
}
except maybe less ugly.
And we should do a pass after this lands to incorporate @mhofman 's recommendations on unregistering during reintroduction, which should reduce the number of cases this callback observes.
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.
No, per my comment above, I think that would totally break it.
de1c6e1
to
b7d34e5
Compare
I appear to have introduced some kind of weird Git singularity. |
c620de9
to
c4af855
Compare
b7d34e5
to
d24b550
Compare
d24b550
to
9679c47
Compare
c4af855
to
3680745
Compare
9679c47
to
bc16f4c
Compare
* rename unretiredKernelRecognizableRemotables to kernelRecognizableRemotables , since they're all unretired * retireOneExport: delete from kernelRecognizableRemotables even if slotToVal was empty. I don't think this could happen, but they're logically distinct checks. * rearrange scanForDeadObjects slightly for clarity * update comment on possibleVirtualObjectDeath * comment on why we add globals to vatPowers * don't completely unpack gcTools: one-off uses can dereference directly, to let the unpack fit in a single line refs #3732
Ok, #3968 has my (small) recommended changes. To remind myself, there are still two items I want to investigate after this lands:
|
r+ with the supplied PR landed |
* chore(swingset): small tweaks to GC * rename unretiredKernelRecognizableRemotables to kernelRecognizableRemotables , since they're all unretired * retireOneExport: delete from kernelRecognizableRemotables even if slotToVal was empty. I don't think this could happen, but they're logically distinct checks. * rearrange scanForDeadObjects slightly for clarity * update comment on possibleVirtualObjectDeath * comment on why we add globals to vatPowers * don't completely unpack gcTools: one-off uses can dereference directly, to let the unpack fit in a single line refs #3732 * fix: revert retireOneExport handling of kernelRecognizableRemotables I'm still uncertain that this is the right way to go, but we're going to leave this as-is and revisit it later.
* chore(swingset): small tweaks to GC * rename unretiredKernelRecognizableRemotables to kernelRecognizableRemotables , since they're all unretired * retireOneExport: delete from kernelRecognizableRemotables even if slotToVal was empty. I don't think this could happen, but they're logically distinct checks. * rearrange scanForDeadObjects slightly for clarity * update comment on possibleVirtualObjectDeath * comment on why we add globals to vatPowers * don't completely unpack gcTools: one-off uses can dereference directly, to let the unpack fit in a single line refs #3732 * fix: revert retireOneExport handling of kernelRecognizableRemotables I'm still uncertain that this is the right way to go, but we're going to leave this as-is and revisit it later.
* chore(swingset): small tweaks to GC * rename unretiredKernelRecognizableRemotables to kernelRecognizableRemotables , since they're all unretired * retireOneExport: delete from kernelRecognizableRemotables even if slotToVal was empty. I don't think this could happen, but they're logically distinct checks. * rearrange scanForDeadObjects slightly for clarity * update comment on possibleVirtualObjectDeath * comment on why we add globals to vatPowers * don't completely unpack gcTools: one-off uses can dereference directly, to let the unpack fit in a single line refs #3732 * fix: revert retireOneExport handling of kernelRecognizableRemotables I'm still uncertain that this is the right way to go, but we're going to leave this as-is and revisit it later.
* chore(swingset): small tweaks to GC * rename unretiredKernelRecognizableRemotables to kernelRecognizableRemotables , since they're all unretired * retireOneExport: delete from kernelRecognizableRemotables even if slotToVal was empty. I don't think this could happen, but they're logically distinct checks. * rearrange scanForDeadObjects slightly for clarity * update comment on possibleVirtualObjectDeath * comment on why we add globals to vatPowers * don't completely unpack gcTools: one-off uses can dereference directly, to let the unpack fit in a single line refs #3732 * fix: revert retireOneExport handling of kernelRecognizableRemotables I'm still uncertain that this is the right way to go, but we're going to leave this as-is and revisit it later.
Previously we treated any reference to an object in the value of a virtual object property as a permanent reference to that object, preventing its garbage collection forever. With this set of changes, these references are now counted, and any unreferenced objects become eligible for garbage collection, regardless of whether the reference is to a virtual object, a presence or a local (remotable) object.