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.

Sign up for GitHub

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(swingset): gc-actions: new algorithm, update test #3296

Merged
merged 2 commits into from
Jun 12, 2021
+108 −21
Merged

fix(swingset): gc-actions: new algorithm, update test #3296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6c85e21
fix(swingset): gc-actions: new algorithm, update test
warner Jun 11, 2021
fe5a721
chore(swingset): add docs to processNextGCAction and filterAction
warner Jun 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 66 additions & 19 deletions packages/SwingSet/src/kernel/gc-actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,31 +15,79 @@ function parseAction(s) {
export function processNextGCAction(kernelKeeper) {
const allActionsSet = kernelKeeper.getGCActions();

function getRefCount(kref) {
// When we check for a re-exported kref, if it's entirely missing, that
// qualifies (to us) as a zero refcount.
const owner = kernelKeeper.ownerOfKernelObject(kref);
if (owner) {
return kernelKeeper.getObjectRefCount(kref);
// GC actions are each one of 'dropExport', 'retireExport', or
// 'retireImport', aimed at a specific vat and affecting a specific kref.
// They are added to the durable "GC Actions" set (stored in kernelDB) when
// `processRefcounts` notices a refcount sitting at zero, which means some
// vat needs to be told that an object can be freed. Before each crank, the
// kernel calls processNextGCAction to see if there are any GC actions that
// should be taken. All such GC actions are executed before any regular vat
// delivery gets to run.

// However, things might have changed between the time the action was
// pushed into the durable set and the time the kernel is ready to execute
// it. For example, the kref might have been re-exported: we were all set
// to tell the exporting vat that their object isn't recognizable any more
// (with a `dispatch.retireExport`), but then they sent a brand new copy to
// some importer. We must negate the `retireExport` action, because it's no
// longer the right thing to do. Alternatively, the exporting vat might
// have deleted the object itself (`syscall.retireExport`) before the
// kernel got a chance to deliver the `dispatch.retireExport`, which means
// we must bypass the action as redundant (since it's an error to delete
// the same c-list entry twice).

// This `filterAction` function looks at each queued GC Action and decides
// whether the current state of the c-lsits and reference counts warrants
// permits the action to run, or if it should be negated/bypassed.

function filterAction(vatKeeper, action, type, kref) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably could use a comment explaining why the actions are being filtered, since the verb "filter" is pretty generic, i.e., what the respective meanings of the pre-filtration and post-filtration collections are supposed to be. I think what's happening is it's throwing away the actions that it's not actually going to perform this time through, but I'm not 100% sure my understanding is correct.

const hasCList = vatKeeper.hasCListEntry(kref);
const isReachable = hasCList ? vatKeeper.getReachableFlag(kref) : undefined;
const exists = kernelKeeper.kernelObjectExists(kref);
const { reachable, recognizable } = exists
? kernelKeeper.getObjectRefCount(kref)
: {};

if (type === 'dropExport') {
if (!exists) return false; // already, shouldn't happen
if (reachable) return false; // negated
if (!hasCList) return false; // already, shouldn't happen
if (!isReachable) return false; // already, shouldn't happen
}
if (type === 'retireExport') {
if (!exists) return false; // already
if (reachable || recognizable) return false; // negated
if (!hasCList) return false; // already
}
return { reachable: 0, recognizable: 0 };
if (type === 'retireImport') {
if (!hasCList) return false; // already
}
return true;
}

function filterActions(groupedActions) {
// We process actions in groups (sorted first by vat, then by type), to
// make it deterministic, and to ensure that `dropExport` happens before
// `retireExport`. This examines one group at a time, filtering everything
// in that group, and returning the survivors of the first group that
// wasn't filtered out entirely. Our available dispatch functions take
// multiple krefs (`dispatch.dropExports`, rather than
// `dispatch.dropExport`), so the set of surviving krefs can all be
// delivered to a vat in a single crank.

// Some day we may consolidate the three GC delivery methods into a single
// one, in which case we'll batch together an entire vat's worth of
// actions, instead of the narrower (vat+type) group. The filtering rules
// may need to change to support that, to ensure that `dropExport` and
// `retireExport` can both be delivered.

function filterActions(vatID, groupedActions) {
const vatKeeper = kernelKeeper.provideVatKeeper(vatID);
const krefs = [];
const actions = [];
for (const action of groupedActions) {
const { type, kref } = parseAction(action);
const { reachable, recognizable } = getRefCount(kref);
// negate actions on re-exported krefs, and don't treat as work to do
if (reachable || (type === 'retireExport' && recognizable)) {
allActionsSet.delete(action);
} else {
if (filterAction(vatKeeper, action, type, kref)) {
krefs.push(kref);
actions.push(action);
}
}
for (const action of actions) {
allActionsSet.delete(action);
}
return krefs;
Expand All @@ -57,7 +105,6 @@ export function processNextGCAction(kernelKeeper) {
}
forVat.get(type).push(action);
}
// console.log(`grouped:`, grouped);

const vatIDs = Array.from(grouped.keys());
vatIDs.sort();
Expand All @@ -67,7 +114,7 @@ export function processNextGCAction(kernelKeeper) {
for (const type of typePriority) {
if (forVat.has(type)) {
const actions = forVat.get(type);
const krefs = filterActions(actions);
const krefs = filterActions(vatID, actions);
if (krefs.length) {
// at last, we act
krefs.sort();
Expand Down
44 changes: 42 additions & 2 deletions packages/SwingSet/test/test-gc-actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ test('gc actions', t => {
actions = a;
newActions = Array.from(a);
}
const clistState = { v1: { ko1: {}, ko2: {} }, v2: { ko2: {} } };

const kernelKeeper = {
getGCActions() {
return new Set(actions);
Expand All @@ -20,13 +22,23 @@ test('gc actions', t => {
newActions = Array.from(a);
newActions.sort();
},
ownerOfKernelObject(kref) {
return rc[kref] ? 'vatX' : undefined;
kernelObjectExists(kref) {
return !!rc[kref];
},
getObjectRefCount(kref) {
const [reachable, recognizable] = rc[kref];
return { reachable, recognizable };
},
provideVatKeeper(vatID) {
return {
hasCListEntry(kref) {
return !!clistState[vatID][kref].exists;
},
getReachableFlag(kref) {
return !!clistState[vatID][kref].isReachable;
},
};
},
};
function process() {
return processNextGCAction(kernelKeeper);
Expand All @@ -46,8 +58,10 @@ test('gc actions', t => {
// fully dropped. the dropExport takes priority
setActions(['v1 dropExport ko1', 'v1 retireExport ko1']);
rc = { ko1: [0, 0] };
clistState.v1.ko1 = { exists: true, isReachable: true };
msg = process();
t.deepEqual(msg, make('dropExports', 'v1', 'ko1'));
clistState.v1.ko1 = { exists: true, isReachable: false };
t.deepEqual(newActions, ['v1 retireExport ko1']);
// then the retireExport
setActions(['v1 retireExport ko1']);
Expand All @@ -59,18 +73,21 @@ test('gc actions', t => {
// fully dropped, then fully re-reachable before dropExports: both negated
setActions(['v1 dropExport ko1', 'v1 retireExport ko1']);
rc = { ko1: [1, 1] }; // re-exported, still reachable+recognizable
clistState.v1.ko1 = { exists: true, isReachable: true };
msg = process();
t.deepEqual(msg, undefined);
t.deepEqual(newActions, []);

// fully dropped, dropExport happens, then fully re-reachable: retire negated
setActions(['v1 dropExport ko1', 'v1 retireExport ko1']);
rc = { ko1: [0, 0] };
clistState.v1.ko1 = { exists: true, isReachable: true };
msg = process();
t.deepEqual(msg, make('dropExports', 'v1', 'ko1'));
t.deepEqual(newActions, ['v1 retireExport ko1']);
setActions(['v1 retireExport ko1']);
rc = { ko1: [1, 1] };
clistState.v1.ko1 = { exists: true, isReachable: false };
msg = process();
t.deepEqual(msg, undefined);
t.deepEqual(newActions, []);
Expand All @@ -79,9 +96,11 @@ test('gc actions', t => {
rc = { ko1: [0, 0] };
setActions(['v1 dropExport ko1', 'v1 retireExport ko1']);
rc = { ko1: [1, 1] };
clistState.v1.ko1 = { exists: true, isReachable: true };
rc = { ko1: [0, 1] };
msg = process();
t.deepEqual(msg, make('dropExports', 'v1', 'ko1'));
clistState.v1.ko1 = { exists: true, isReachable: false };
t.deepEqual(newActions, ['v1 retireExport ko1']);
// the retire is left pending because we ignore lower-prority types
setActions(['v1 retireExport ko1']);
Expand All @@ -93,11 +112,13 @@ test('gc actions', t => {
// fully dropped, dropExports happens, re-reachable, partial drop: retire
// negated
setActions(['v1 dropExport ko1', 'v1 retireExport ko1']);
clistState.v1.ko1 = { exists: true, isReachable: true };
rc = { ko1: [0, 0] };
msg = process();
t.deepEqual(msg, make('dropExports', 'v1', 'ko1'));
t.deepEqual(newActions, ['v1 retireExport ko1']);
setActions(['v1 retireExport ko1']);
clistState.v1.ko1 = { exists: true, isReachable: true };
rc = { ko1: [0, 1] };
msg = process();
t.deepEqual(msg, undefined);
Expand All @@ -106,46 +127,65 @@ test('gc actions', t => {
// partially dropped: recognizable but not reachable
setActions(['v1 dropExport ko1']);
rc = { ko1: [0, 1] }; // recognizable, not reachable
clistState.v1.ko1 = { exists: true, isReachable: true };
msg = process();
t.deepEqual(msg, make('dropExports', 'v1', 'ko1'));
t.deepEqual(newActions, []);

// partially dropped, re-reachable: negate dropExports
setActions(['v1 dropExport ko1']);
rc = { ko1: [1, 1] };
clistState.v1.ko1 = { exists: true, isReachable: true };
msg = process();
t.deepEqual(msg, undefined);
t.deepEqual(newActions, []);

// priority order: retireImports is last
setActions(['v1 dropExport ko1', 'v1 retireImport ko2']);
rc = { ko1: [0, 0], ko2: [0, 0] };
clistState.v1.ko1 = { exists: true, isReachable: true };
clistState.v1.ko2 = { exists: true, isReachable: false };
msg = process();
t.deepEqual(msg, make('dropExports', 'v1', 'ko1'));
t.deepEqual(newActions, ['v1 retireImport ko2']);

setActions(['v1 retireExport ko1', 'v1 retireImport ko2']);
rc = { ko1: [0, 0], ko2: [0, 0] };
clistState.v1.ko1 = { exists: true, isReachable: true };
clistState.v1.ko2 = { exists: true, isReachable: false };
msg = process();
t.deepEqual(msg, make('retireExports', 'v1', 'ko1'));
t.deepEqual(newActions, ['v1 retireImport ko2']);

setActions(['v1 retireImport ko2']);
rc = { ko1: [0, 0], ko2: [0, 0] };
clistState.v1.ko2 = { exists: true, isReachable: false };
msg = process();
t.deepEqual(msg, make('retireImports', 'v1', 'ko2'));
t.deepEqual(newActions, []);

// retireImport but was already done
setActions(['v1 retireImport ko2']);
rc = { ko1: [0, 0], ko2: [0, 0] };
clistState.v1.ko2 = { exists: false, isReachable: false };
msg = process();
t.deepEqual(msg, undefined);
t.deepEqual(newActions, []);

// multiple vats: process in sorted order
setActions(['v1 dropExport ko1', 'v2 dropExport ko2']);
rc = { ko1: [0, 0], ko2: [0, 0] };
clistState.v1.ko1 = { exists: true, isReachable: true };
clistState.v2.ko2 = { exists: true, isReachable: true };
msg = process();
t.deepEqual(msg, make('dropExports', 'v1', 'ko1'));
t.deepEqual(newActions, ['v2 dropExport ko2']);

// multiple vats: vatID is major sort order, type is minor
setActions(['v1 retireExport ko1', 'v2 dropExport ko2']);
rc = { ko1: [0, 0], ko2: [0, 0] };
clistState.v1.ko1 = { exists: true, isReachable: false };
clistState.v2.ko2 = { exists: true, isReachable: true };
msg = process();
t.deepEqual(msg, make('retireExports', 'v1', 'ko1'));
t.deepEqual(newActions, ['v2 dropExport ko2']);
Expand Down