Virtual object garbage collection

[FUDCo]

This implements the first stage of VOM GC: garbage collection of virtual objects per se.

This does not yet deal with virtual objects (or any other kind of objects) that are referenced from virtual object properties -- these are still pinned using the mechanisms that we had in place for that purpose prior to this. Nor does it implement GC of virtual object keyed entries in weak collections. These will come in a later PR.

Commits are organized topically for ease of review.

Closes #3456

[warner]

Nice work! The changes are a lot smaller than I expected.. that's awesome.

I've walked through everything and I agree that this is sound (it's not going to drop anything that it shouldn't drop), and the status quo of the RECOGNIZABLE state is maintained (i.e. it's just as conservative as the previous code). This change should drop (but not retire) virtual objects that are not being used by the kernel, and not represented by a Representative, and have never been used in virtualized data. Awesome.

I think there's a redundancy that should be cleaned up, because it's going to confuse us during the next phase. This PR uses both isVrefReachable and the reference counts to keep Virtual objects alive, and I think it should only use the reference counts. The resulting behavior is correct, but when we move to decrementing refcounts, this is going to look weird.

isVrefReachable is the predicate that the VOM provides to liveslots, to inhibit a vref drop when a Presence goes away. In the old code, the VOM used an add-only Set (reachableVrefs) to track every Presence-based vref that was ever used in virtualized data. This Set is populated by addReachablePresenceRef, which is called with every vref (of any sort) used in virtualized data. So addReachablePresenceRef is responsible for filtering out just the Presence-based ones that need to be pinned with reachableVrefs.

As a separate requirement, reachableRemotables is the Set of Remotable objects (not vrefs) that must be kept alive because of references from virtualized data. addReachableRemoteableRef is similiarly called for every vref (of any sort), and is responsible for filtering out just the Remotables.

addReachableRemoteableRef is called from exactly the same places as addReachablePresenceRef, for historical reasons (they got added at separate times, and I should have merged them at that time).

In this PR, you're changing addReachablePresenceRef to include VO-based vrefs to the reachableVrefs pinning set, and you're changing addReachableRemoteableRef to increment their refcounts. And in possibleVirtualObjectDeath, there are effectively four tests applied to the vref to see whether it can be dropped or not:

  function possibleVirtualObjectDeath(vobjID) {
    if (!isVrefReachable(vobjID) && !getValForSlot(vobjID)) {
      const [exported, refCount] = getRefCounts(vobjID);
      if (exported === 0 && refCount === 0) {
        syscall.vatstoreDelete(`vom.${vobjID}`);
        syscall.vatstoreDelete(`vom.${vobjID}.refCount`);
      }

• isVrefReachable: this is effectively testing the "pinned by virtualized data" predicate
• getValForSlot: this tests the "Representative is present" pillar
• exported === 0: this tests the "kernel export status is REACHABLE" pillar
• refCount === 0: this tests the "virtualized data status is REACHABLE" pillar

So here's my suggestion:

• merge addReachablePresenceRef and addReachableRemotableRef into a single addReachableRef
• make the merged addReachableRef look at the type of vref and do one of three things:
  • for Presence-related vrefs, add the vref to reachableVrefs
  • for Remotable-related vrefs, add the Remotable to reachableRemotables
  • for Representative (virtual object) -related vrefs, increment the refcount
• leave isVrefReachable and reachableVrefs for Presence-based vrefs
• change possibleVirtualObjectDeath to get the refcounts, then use a three-clause test:

 const [exported, refCount] = getRefCounts(vobjID);
 if (!getValForSlot(vobjID) && exported === 0 && refCount === 0) {
   // delete..

I'll add some other suggestions inline.

[warner]

Code looks good but let's make those cleanups for the sake of our sanity in the next phase. I'm confused by the tests (probably by the VOM's caching) but I'll keep studying them.

[warner]

I find this test pretty confusing. If I read it correctly, by the time finish() finishes, the virtual objects originally associated with the Representatives in things[] are left in the following states

number	exported	has Representative	in virtualized data	notes
1	exported+dropped	no	no	Rep dropped before kernel drops
2	exported+dropped	yes	no
3	never	no	no
4	exported+retained	no	no
5	never	no	no
6	never	no	no
7	never	yes	no	no
8	never	yes	no	no
9	never	yes	no	no

The test is asserting:

• v1.vs.vom.o+1/2 (that's thing2, right?) has a 0/0 refcount: good, it's kept alive by the Representative pillar and nothing else
• ../3 (thing3) has a 1/0 refcount .. wait, thing4 should have 1/0 (kept alive by the kernel-export pillar, but no Representative), am I off-by-one already? The vobjID are assigned upon object creation, not export, yeah?
• ../4 through ../7 are deleted: I'm off again, it looks like thing7 should be retained by a Representative
• ../8 and ../9 have data but no refcounts, which seems slightly weird, but I guess you're avoiding the DB expense for virtual objects that are retained by only a Representative and not by either a kernel-export or virtualized-data?

If we can enumerate the cases we care about, it might be easier to follow this test (and to enhance it in the future) to make them all explicit. Create a batch of Representatives, forget a few, export a few, store a few in virtualized data. Wait for the peer to drop some of the exported ones. Forget a few more (to test both orderings: Representative collected before kernel drops, kernel drops before Representative collected). Then let the test runner look at the DB and make sure everything matches the expectations.

[FUDCo]

These cases will remain confusing until we have implemented all three legs of the tripod. Remember that at this point we've only got 2, because using something in a property value pins it forever.

[FUDCo]

Let me see if I can make this clearer (I'll turn this into a comment in the test if so):

First of all, nothing in this test exercises a case where any virtual object refers to any other virtual object, so the fourth column of your table is nugatory. Also, I'm not sure what the batch of "no"s in the lower portion of the table in the Notes column even means. With respect to any given case, we really only have two variables to fiddle with: whether a reference to the VO is exported, and whether a reference to the VO is retained in memory in the vat.

For the first variable, there are three possible states: not exported, exported and then retained externally, exported and then dropped externally.

For the second variable, there are two possible states: retained internally, and dropped internally.

That would seem like 6 cases, but that's not quite true, since a reference that is dropped internally before being exported can't ever be exported. (Also if a VO has both an exported reference and an internal reference and both are dropped, we still have to worry about what order they're dropped in, but it's the other test that worries about the order of droppage issue).

number	exported?	export dropped?	local dropped?	deleted?
1	yes	yes	yes	yes (dropped both locally and by export)
2	yes	yes	no	no (retained in things)
3	yes	no	yes	no (retained by export)
4	no	n/a	yes	yes (dropped before use)
5,6,7	no	n/a	yes	yes (dropped at end)
8,9	no	n/a	no	no (retained unused)

Things 5, 6, and 7 are essentially the same as 4, but they were handled in a loop at the end, to make sure that that worked.
Things 8 and 9 are both the same. They were never used and so were retained where they were originally stashed on creation.

[warner]

Ooookay, I think I see how that table might come out of the logic in getThing, but I don't think I'd be able to figure that out by reading the test (and obviously I've already gotten it wrong once). Thank's for adding the table to the test case, that will help me a lot.

[warner]

We should check with @katelynsills to see if ERTP wants to put Payments in a WeakSet or as the key of a WeakMap (I seem to remember a de-duplicating WeakSet in reallocate that should probably have been a Set instead). If so, we might want to figure out how to retire those objects sooner rather than later.

[katelynsills]

We should check with @katelynsills to see if ERTP wants to put Payments in a WeakSet or as the key of a WeakMap (I seem to remember a de-duplicating WeakSet in reallocate that should probably have been a Set instead). If so, we might want to figure out how to retire those objects sooner rather than later.

The paymentLedger is a weakStore from '@agoric/store' with payments as keys. Separately, there is anti-aliasing which uses a native Set. There was no WeakSet for anti-aliasing, but rather a makeWeakStore (the vat global, not the import from @agoric/store) which was used only because it was absolutely necessary at the time. This was because only the vat global makeWeakStore supported virtual object identities. After native Sets supported virtual objects, I changed the code to use a native Set.

[warner]

The paymentLedger is a weakStore from '@agoric/store' with payments as keys.

And that weakStore (really a WeakMap with a different API) lives in the same vat as the Payments themselves, right? So we'll only ever be using Representatives in it, not Presences.

Let's see, our current VirtualObjectAwareWeakMap will recognize the non-Presence vref being used as a key, and will add the vref string to a (strong) Map object. We don't yet have a mechanism to trigger when the virtual object is deleted (to walk through all the VirtualObjectAwareWeakMaps) and remove it. So we'll keep the Map entry forever (in RAM). The value is (I think) is an Amount, which references a Brand, which is probably also in the same vat (so it'll be a Remotable, not a Presence). So we'll consume RAM for the Map entry and the Amount for each Payment that was ever used. Hrmph.

Separately, there is anti-aliasing which uses a native Set. There was no WeakSet for anti-aliasing, but rather a makeWeakStore (the vat global, not the import from @agoric/store) which was used only because it was absolutely necessary at the time. This was because only the vat global makeWeakStore supported virtual object identities. After native Sets supported virtual objects, I changed the code to use a native Set.

Got it. And is that anti-aliasing taking place in the Zoe vat, which might not be the one where the Payment was created? So it might be dealing with Presences. But in either case, using a native Set is great, that doesn't interact with our virtual object manager at all, and it will properly drop everything when the Set itself is deleted.

[katelynsills]

And that weakStore (really a WeakMap with a different API) lives in the same vat as the Payments themselves, right? So we'll only ever be using Representatives in it, not Presences.

Yes! This is part of the ERTP code which is invoked when makeIssuerKit is invoked. So it will always be in the same vat as the Payments themselves.

Let's see, our current VirtualObjectAwareWeakMap will recognize the non-Presence vref being used as a key, and will add the vref string to a (strong) Map object. We don't yet have a mechanism to trigger when the virtual object is deleted (to walk through all the VirtualObjectAwareWeakMaps) and remove it. So we'll keep the Map entry forever (in RAM). The value is (I think) is an Amount, which references a Brand, which is probably also in the same vat (so it'll be a Remotable, not a Presence). So we'll consume RAM for the Map entry and the Amount for each Payment that was ever used. Hrmph.

Yup, the Brand will also be in the same vat. It's trivial to change the paymentLedger to use a Value instead of an Amount, if that would be helpful. We have an issue for it, but it hasn't been prioritized.

And is that anti-aliasing taking place in the Zoe vat, which might not be the one where the Payment was created? So it might be dealing with Presences.

Nope, this is still part of core ERTP (only occurs in E(issuer).combine(payments)), so it will be the same vat in which the Payment is created.

[warner]

not for today, but a future refactoring to make: since this is now the third time vref is getting parsed, we should probably rearrange this to have a top-level loop on vrefs, parse it once inside the loop, then have all three checks (type, allocatedByVat, virtual/not-virtual) after that parse

[warner]

Ok, let's land this, but let's make sure to fix that vrefIsReachable thing soon.