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

Keep WeakRef to FinalizationRegistry target #7371

Closed
mhofman opened this issue Apr 10, 2023 · 0 comments · Fixed by #7552
Closed

Keep WeakRef to FinalizationRegistry target #7371

mhofman opened this issue Apr 10, 2023 · 0 comments · Fixed by #7552
Assignees
Labels
enhancement New feature or request liveslots requires vat-upgrade to deploy changes SwingSet package: SwingSet vaults_triage DO NOT USE xsnap the XS execution tool

Comments

@mhofman
Copy link
Member

mhofman commented Apr 10, 2023

What is the Problem Being Solved?

In Moddable-OpenSource/moddable@2523e3a Moddable is changing XS so that during organic GC, WeakRef are considered strong. This is in an attempt to hide organic gc from liveslots to work around #6784. However this only works if there is a WeakRef for anything observed by liveslots through FinalizationRegistry.

Apparently the VirtualObjectAwareWeakMap and VirtualObjectAwareWeakSet use a FinalizationRegistry to detect when the userland weak collection is finalized, but doesn't actually need a WeakRef to it.

Description of the Design

Add a WeakRef to these userland weak collections, possibly in a Set that is itself cleaned up when the FR callback triggers

@mhofman mhofman added enhancement New feature or request SwingSet package: SwingSet xsnap the XS execution tool liveslots requires vat-upgrade to deploy changes labels Apr 10, 2023
@ivanlei ivanlei added the vaults_triage DO NOT USE label Apr 27, 2023
warner added a commit that referenced this issue 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.

closes #7371
warner added a commit that referenced this issue 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
@mergify mergify bot closed this as completed in #7552 Apr 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request liveslots requires vat-upgrade to deploy changes SwingSet package: SwingSet vaults_triage DO NOT USE xsnap the XS execution tool
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants