Skip to content
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

add WeakRef tracking of imported Presences to liveslots #2667

Closed
wants to merge 3 commits into from

Conversation

warner
Copy link
Member

@warner warner commented Mar 17, 2021

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 warner added the SwingSet package: SwingSet label Mar 17, 2021
@warner warner self-assigned this Mar 17, 2021
@warner warner force-pushed the 2653-dummy-dropexports branch from 613c165 to 67dade8 Compare March 17, 2021 18:32
@warner warner force-pushed the 2660-add-weakref branch from 203216d to d15c506 Compare March 17, 2021 18:36
@warner warner marked this pull request as draft March 17, 2021 18:37
@warner
Copy link
Member Author

warner commented Mar 17, 2021

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

@warner warner force-pushed the 2653-dummy-dropexports branch from 67dade8 to da1c505 Compare March 17, 2021 22:57
@warner warner force-pushed the 2660-add-weakref branch from d15c506 to c9af540 Compare March 17, 2021 22:57
@warner warner marked this pull request as ready for review March 18, 2021 22:30
@warner warner requested a review from FUDCo March 19, 2021 06:53
@warner warner force-pushed the 2660-add-weakref branch 3 times, most recently from 93e7e7f to bac7b20 Compare March 22, 2021 23:10
@warner warner force-pushed the 2653-dummy-dropexports branch from b1080f0 to 9fabfdf Compare March 22, 2021 23:41
@warner 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 warner force-pushed the 2660-add-weakref branch from 9ba29cb to aefc80d Compare March 22, 2021 23:59
Copy link
Contributor

@FUDCo FUDCo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment on lines 83 to 90
* 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 67 to 75
* 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const deadSet = new Set(); // vrefs that are finalized but not yet reported

/*
Imports have 5 states: UNKNOWN, REACHABLE, UNREACHABLE, COLLECTED,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

warner commented Mar 24, 2021

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.

@warner 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
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 warner force-pushed the 2660-add-weakref branch from 8fe8077 to 2003156 Compare March 24, 2021 18:28
@warner
Copy link
Member Author

warner commented Mar 26, 2021

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants