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

fix(liveslots): retain WeakRefs to voAware collections #7552

Merged
merged 4 commits into from
Apr 29, 2023

Conversation

warner
Copy link
Member

@warner warner commented Apr 29, 2023

A lingering source of GC sensitivity is when a FinalizationRegistry is
watching an object that userspace might drop, and that FR callback
might perform syscalls, or simply consume more computrons when GC
happens than when it does not. While we expect all instances of a
consensus kernel to perform GC at the same time, any tolerance we can
maintain against divergent GC schedules will help us with
upgrade (replaying a transcript with a slightly different version of
XS).

To help this, in XS, WeakRefs are treated as strong references during
"organic" GC, and are only actually weak during the forced GC we do
inside bringOutYourDead. We'd like this improvement for objects
tracked by FinalizationRegistries too. Liveslots has two FRs,
vreffedObjectRegistry (whose entries all have WeakRefs in slotToVal,
so they're covered), and droppedCollectionRegistry (in the VRM).

The VRM's droppedCollectionRegistry tracks the VO-aware replacements
for WeakMap/Set that we impose on userspace, and these do not have
vref identities, so they will never appear in slotToVal or valToSlot,
so we need to create new WeakRefs to trigger the organic-GC-retention
behavior.

This commit adds a Map named droppedCollectionWeakRefs to the VRM,
which maps from a new 'holder' object to a special WeakRef for
each (imposed) VO-aware WeakMap/Set that userspace creates. This entry
is held until the FR callback is run, which removes it. The holder
object is tracked by the context/heldValue value that the FR passes to
its callback.

The net result is that userspace dropping their VO-aware collection
objects should not result in the FinalizationRegistry callback running
until a bringOutYourDead delivery, which happens on a fixed
(within-consensus) schedule.

This PR also performs refactoring and test tooling improvements to help test the new feature.

closes #7371

@warner warner added SwingSet package: SwingSet liveslots requires vat-upgrade to deploy changes labels Apr 29, 2023
@warner warner requested a review from mhofman April 29, 2023 06:51
@warner warner self-assigned this Apr 29, 2023
@warner
Copy link
Member Author

warner commented Apr 29, 2023

Most of the commits are refactorings and test preparation: only the last commit changes real behavior.

Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

After giving this some thoughts, we don't need the side maps at all. Just put the WeakRef in the value held by the FinalizationRegistry

Comment on lines 51 to 54
const holder = {};
const wr = new WeakRef(target);
droppedCollectionWeakRefs.set(holder, wr);
droppedCollectionRegistry.register(target, { descriptor, holder });
Copy link
Member

Choose a reason for hiding this comment

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

We don't need the droppedCollectionWeakRefs indirection, the heldValue is held strongly.

Suggested change
const holder = {};
const wr = new WeakRef(target);
droppedCollectionWeakRefs.set(holder, wr);
droppedCollectionRegistry.register(target, { descriptor, holder });
const wr = new WeakRef(target);
droppedCollectionRegistry.register(target, { descriptor, wr });

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, will do, but I think that will change my testing plan.

I can check that a mock WeakRef was created around the VOAware collection. I can instrument my mock FR to let me inspect the heldValue and confirm that it is present in there. But I won't be able to test past that point. I guess that's probably good enough.

@warner warner requested a review from mhofman April 29, 2023 18:09
Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -636,7 +650,8 @@ export function makeVirtualReferenceManager(
}
}

function finalizeDroppedCollection(descriptor) {
function finalizeDroppedCollection({ descriptor }) {
// the 'wr' WeakRef will be dropped about now
Copy link
Member

Choose a reason for hiding this comment

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

So little tangent on what optimizations engines are allowed or not. Technically, since wr is never observed by the program, the JS engine is allowed to not keep it in the heldValue, which means that the WeakRef would actually not exist in the first place.

If we wanted to be as close to the spec rules as possible, we would need to actually try to deref the weakref and assert it's empty (or even better, log the target if not empty, causing an actual observation). Obviously no engine goes that far in its optimizations, certainly not XS, and the destructuring dropping the wr is in fact completely sufficient.

Copy link
Member Author

Choose a reason for hiding this comment

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

wow, the engine is allowed to inspect the callback's code to determine that? hardcore.

Copy link
Member

Choose a reason for hiding this comment

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

The engine is allowed to do anything as long as it's not observable to the program. If a property of an object is never used, it doesn't have to store it ;) But in practice engines are not omniscient enough.

@warner warner added automerge:rebase Automatically rebase updates, then merge and removed automerge:rebase Automatically rebase updates, then merge labels Apr 29, 2023
A lingering source of GC sensitivity is when a FinalizationRegistry is
watching an object that userspace might drop, and that FR callback
might perform syscalls, or simply consume more computrons when GC
happens than when it does not. While we expect all instances of a
consensus kernel to perform GC at the same time, any tolerance we can
maintain against divergent GC schedules will help us with
upgrade (replaying a transcript with a slightly different version of
XS).

To help this, in XS, WeakRefs are treated as strong references during
"organic" GC, and are only actually weak during the forced GC we do
inside bringOutYourDead. We'd like this improvement for objects
tracked by FinalizationRegistries too. Liveslots has two FRs,
`vreffedObjectRegistry` (whose entries all have WeakRefs in slotToVal,
so they're covered), and `droppedCollectionRegistry` (in the VRM).

The VRM's `droppedCollectionRegistry` tracks the VO-aware replacements
for WeakMap/Set that we impose on userspace, and these do not have
vref identities, so they will never appear in slotToVal or valToSlot,
so we need to create new WeakRefs to trigger the organic-GC-retention
behavior.

These WeakRefs are stored in the FR's "context"/heldValue data, which
will be passed to the callback when the VO-aware collection is
dropped, collected, and finalized. The WeakRef will be kept alive
through that entry until the finalizer runs.

The net result is that userspace dropping their VO-aware collection
objects should not result in the FinalizationRegistry callback running
until a bringOutYourDead delivery, which happens on a fixed
(within-consensus) schedule.

closes #7371
@warner warner added the automerge:rebase Automatically rebase updates, then merge label Apr 29, 2023
@mergify mergify bot merged commit 6e658d9 into master Apr 29, 2023
@mergify mergify bot deleted the 7371-weakref-to-FR-target branch April 29, 2023 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge liveslots requires vat-upgrade to deploy changes SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keep WeakRef to FinalizationRegistry target
2 participants