add WeakRef tracking of imported Presences to liveslots #2667

warner · 2021-03-17T02:11:08Z

This updates the liveslots tables to use WeakRefs instead of strong references in slotToVal, which allows us to sense when userspace has stopped referencing an imported Presence. It adds a FinalizationRegistry to track when the vref is in a state where it's unreachable by userspace, hasn't been re-imported, and is ready to be submitted in a syscall.dropImports (but it does not yet call dropImports).

There's a description of the possible states and their transitions in the commit, which probably needs to be promoted to a theory-of-operation document somewhere.

Exports are kept alive by a strong reference in a Set named exported (which could maybe use a better name), because slotToVal can no longer do that job. In the future (#2664) the dispatch.dropExports call will remove these entries and allow non-externally-referenced Remotables to be dropped.

The biggest uncertainty I have is around Promises and device nodes, and whether we handle those correctly or not. It's meant to keep a strong reference to every exported Promise around until it gets resolved and retired, and not try to be clever about dropping unreachable-but-unresolved Promises (I don't think that's feasible anyways).

refs #2660 (but doesn't close it until we're emitting dropImports)
refs #2615

warner · 2021-03-17T18:38:49Z

This will probably need #2669 to land first, to hush the complaints about WeakRef/FinalizationRegistry in supervisor-subprocess-xsnap.js.

FUDCo

Mostly looks good, modulo a few pedantic comment bits and my confusion over when things should go into the finalization registry (the later of which I'm sure you'll either fix directly or more likely explain to me how I'm confused).

FUDCo · 2021-03-23T18:25:02Z

packages/SwingSet/src/kernel/liveSlots.js

+   * slotToVal is like a python WeakValueDict: a `Map` whose values are
+   * WeakRefs. If the entry is present but wr.deref()===undefined (the
+   * weakref is dead), treat that as if the entry was not present. The same
+   * slotToVal table is used for both imports and returning exports. The
+   * subset of those which need to be held strongly (exported objects and
+   * promises, imported promises) are kept alive by `exported`.
+   *
+   * valToSlot is a WeakMap (like WeakKeyDict), and is used for both imports


Not sure the references to Python are as helpful to JavaScript programmers as they might be to Python programmers, and I think most of the people reading this are likely to be JavaScript programmers.

FUDCo · 2021-03-23T18:29:35Z

packages/SwingSet/src/kernel/liveSlots.js

+   * Exports: pass-by-presence objects in the vat are exported as o+NN slots,
+   * as are the upcoming "virtual object" exports. Promises are exported as
+   * p+NN slots. We retain a strong reference to all exports via the
+   * `exported` Set until (TODO) the kernel tells us all external references
+   * have been dropped via dispatch.dropExports, or by some unilateral
+   * revoke-object operation executed by our user-level code.
+   *
+   * Imports: o-NN slots are represented as a Presence. p-NN slots are
+   * represented as an imported Promise, with the resolver held in a table to


Minor pedagogical nit: Correspondence between keys and values in the tables are described here using a "value from key" phrasing in the case of exports and using "key to value" phrasing in the case of imports. I think consistency would be clearer.

FUDCo · 2021-03-23T18:33:54Z

packages/SwingSet/src/kernel/liveSlots.js

+  const deadSet = new Set(); // vrefs that are finalized but not yet reported
+
+  /*
+    Imports have 5 states: UNKNOWN, REACHABLE, UNREACHABLE, COLLECTED,


Worth pointing out that this state machine is a conceptual model of what's going on, for purposes of explanation and understanding, rather than something that is actually represented as such in the code. In particular, some of these states are from a sort of God's eye view that doesn't actually exist in one place in reality.

FUDCo · 2021-03-23T18:59:21Z

packages/SwingSet/src/kernel/liveSlots.js

@@ -283,7 +359,9 @@ function build(
      }
      parseVatSlot(slot); // assertion
      valToSlot.set(val, slot);
-      slotToVal.set(slot, val);
+      slotToVal.set(slot, new WeakRef(val));


Should val be getting registered in the dropRegistry here? (Also at various other points where WeakRefs are created and inserted into slotToVal, of which there are several but only one of which registers.

Huh, good point. I think in this case we don't register it (because it's an export), but that warrants a thorough review, and it looks like I'm not removing things from the deadSet at the right times. I'll take another pass.

warner · 2021-03-24T05:07:40Z

I found a nice way to test that liveslots is doing the right thing with the deadSet. It's very satisfying to comment out functional code and confirm that the tests do indeed fail.

Liveslots now uses WeakRefs and a FinalizationRegistry to track the state of each import: UNKNOWN -> REACHABLE -> UNREACHABLE -> COLLECTED -> FINALIZED -> UNKNOWN. Reintroduction can move it from UNREACHABLE/COLLECTED/FINALIZED back to REACHABLE at any time. Liveslots maintains a local `deadSet` that contains all the vrefs which are in the FINALIZED state. They will remain in that state (and in `deadSet`) until a later change which uses `syscall.dropImports` to inform the kernel, and remove them from `deadSet`. Promises are retained until resolved+retired, even if userspace somehow drops all references to them. We might do better in the future, but the story is a lot more complicated than it is for Presences. Exported Remotables are still retained indefinitely. A later change (#2664) will wire `dropExports()` up to drop them. refs #2660

We only register finalizers for imported objects: not imported promises, and not exports of any flavor. We must remove imported objects from the deadSet when they are re-introduced. Update docs for clarity.

Liveslots is not yet calling syscall.dropImports, but by mocking WeakRef and FinalizationRegistry, we can test to make sure it updates the deadSet correctly.

warner · 2021-03-26T22:04:55Z

I'm going to cancel this PR, because in #2724 I discovered that it exposes non-determinism to vats, and consequently intermittent test failures (generally in the zoe swingset tests), which can be made more frequent by calling gc() before and after each crank.

We have part of a plan to address this, but it's going to involve a lot more work than I was hoping. I think dropExports may be split into two separate calls, or acquire a second argument, to indicate whether it's a "soft" drop (which drops the strong reference on the Remotable, but retains the outgoing WeakMap reference, and thus retains the object's identity), or a "hard" drop (which removes both).

warner added the SwingSet package: SwingSet label Mar 17, 2021

warner self-assigned this Mar 17, 2021

warner force-pushed the 2653-dummy-dropexports branch from 613c165 to 67dade8 Compare March 17, 2021 18:32

warner force-pushed the 2660-add-weakref branch from 203216d to d15c506 Compare March 17, 2021 18:36

warner marked this pull request as draft March 17, 2021 18:37

warner force-pushed the 2653-dummy-dropexports branch from 67dade8 to da1c505 Compare March 17, 2021 22:57

warner force-pushed the 2660-add-weakref branch from d15c506 to c9af540 Compare March 17, 2021 22:57

warner marked this pull request as ready for review March 18, 2021 22:30

warner requested a review from FUDCo March 19, 2021 06:53

warner force-pushed the 2660-add-weakref branch 3 times, most recently from 93e7e7f to bac7b20 Compare March 22, 2021 23:10

warner force-pushed the 2653-dummy-dropexports branch from b1080f0 to 9fabfdf Compare March 22, 2021 23:41

warner force-pushed the 2660-add-weakref branch from bac7b20 to 9ba29cb Compare March 22, 2021 23:41

Base automatically changed from 2653-dummy-dropexports to master March 22, 2021 23:57

warner force-pushed the 2660-add-weakref branch from 9ba29cb to aefc80d Compare March 22, 2021 23:59

FUDCo approved these changes Mar 23, 2021

View reviewed changes

warner force-pushed the 2660-add-weakref branch from 0a60719 to 8fe8077 Compare March 24, 2021 05:39

warner added 3 commits March 24, 2021 11:27

fix(swingset): update deadSet and droppedRegistry properly

3569a7e

We only register finalizers for imported objects: not imported promises, and not exports of any flavor. We must remove imported objects from the deadSet when they are re-introduced. Update docs for clarity.

test(swingset): test liveslots GC with mocks

2003156

Liveslots is not yet calling syscall.dropImports, but by mocking WeakRef and FinalizationRegistry, we can test to make sure it updates the deadSet correctly.

warner force-pushed the 2660-add-weakref branch from 8fe8077 to 2003156 Compare March 24, 2021 18:28

warner closed this Mar 26, 2021

warner mentioned this pull request Mar 31, 2021

refactor dispatch to be a function, not an object #2674

Closed

warner mentioned this pull request Apr 23, 2021

add WeakRef tracking of imported Presences to liveslots #2948

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add WeakRef tracking of imported Presences to liveslots #2667

add WeakRef tracking of imported Presences to liveslots #2667

warner commented Mar 17, 2021

warner commented Mar 17, 2021

FUDCo left a comment

FUDCo Mar 23, 2021

FUDCo Mar 23, 2021

FUDCo Mar 23, 2021

FUDCo Mar 23, 2021

warner Mar 23, 2021

warner commented Mar 24, 2021

warner commented Mar 26, 2021

add WeakRef tracking of imported Presences to liveslots #2667

add WeakRef tracking of imported Presences to liveslots #2667

Conversation

warner commented Mar 17, 2021

warner commented Mar 17, 2021

FUDCo left a comment

Choose a reason for hiding this comment

FUDCo Mar 23, 2021

Choose a reason for hiding this comment

FUDCo Mar 23, 2021

Choose a reason for hiding this comment

FUDCo Mar 23, 2021

Choose a reason for hiding this comment

FUDCo Mar 23, 2021

Choose a reason for hiding this comment

warner Mar 23, 2021

Choose a reason for hiding this comment

warner commented Mar 24, 2021

warner commented Mar 26, 2021