Skip to content

Commit

Permalink
pre-review feedback updates
Browse files Browse the repository at this point in the history
Remove vrefs from their possibly-something set at the start of the
loop, not the end.

Add docs to emphasize that our RAM pillar is bounded by FINALIZED, not
COLLECTED, and why.
  • Loading branch information
warner committed Aug 27, 2024
1 parent 310c064 commit ca2d469
Showing 1 changed file with 20 additions and 5 deletions.
25 changes: 20 additions & 5 deletions packages/swingset-liveslots/src/boyd-gc.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,21 @@ import { parseVatSlot } from './parseVatSlots.js';
finalizer callback queued. So the callback must deduce the current state
and only perform cleanup (i.e. delete the slotToVal entry and add the
baseRef to possiblyDeadSet) in the COLLECTED state.
Our general rule is "trust the finalizer". The GC code below
considers a Presence to be reachable (the vref's "RAM pillar"
remains "up") until it moves to the FINALIZED state. We do this to
avoid race conditions between some other pillar dropping (and a
BOYD sampling the WeakRef) while it is in the COLLECTED state. If
we treated COLLECTED as unreachable,, then the subsequent
finalizer callback would examine the vref a second time,
potentially causing a vat-fatal double syscall.dropImports. This
rule may change if/when we use FinalizationRegistry better, by
explicitly de-registering the vref when we drop it, which JS
guarantees will remove and pending callback from the queue. This
may help us avoid probing the WeakRef during BOYD (instead relying
upon the fired/not-fired state of the FR), since that probing can
cause engines to retain objects longer than necessary.
*/

/*
Expand Down Expand Up @@ -334,6 +349,10 @@ export const makeBOYDKit = ({
// reachability, one at a time

for (const vrefOrBaseRef of [...possiblyDeadSet].sort()) {
// remove the vref now, but some deleteVirtualObject might
// shake it loose again for a future pass to investigate
possiblyDeadSet.delete(vrefOrBaseRef);

const parsed = parseVatSlot(vrefOrBaseRef);
assert.equal(parsed.type, 'object', vrefOrBaseRef);

Expand All @@ -358,10 +377,6 @@ export const makeBOYDKit = ({
exportsToRetire.add(facetVref);
}
gcAgain ||= !!res.gcAgain;

// remove the vref now, so we don't process it again in the
// next pass unless something else shakes it loose before then
possiblyDeadSet.delete(vrefOrBaseRef);
}

// Deleting virtual object state, or freeing weak-keyed
Expand All @@ -382,6 +397,7 @@ export const makeBOYDKit = ({
// Representative- type vrefs.

for (const vref of [...possiblyRetiredSet].sort()) {
possiblyRetiredSet.delete(vref);
const parsed = parseVatSlot(vref);
assert.equal(parsed.type, 'object', vref);
// not allocated by us? that makes it a Presence-type
Expand All @@ -401,7 +417,6 @@ export const makeBOYDKit = ({
importsToRetire.add(vref);
}
}
possiblyRetiredSet.delete(vref);
}

// note that retiring Presence-type vrefs cannot shake loose any
Expand Down

0 comments on commit ca2d469

Please sign in to comment.