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(swingset): abandon/delete most non-durables during stopVat() #5056

Merged
merged 1 commit into from
Apr 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
24 changes: 24 additions & 0 deletions packages/SwingSet/src/liveslots/collectionManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ export function makeCollectionManager(
serialize,
unserialize,
) {
// TODO(#5058): we hold a list of all collections (both virtual and
// durable) in RAM, so we can delete the virtual ones during
// stopVat(), and tolerate subsequent GC-triggered duplication
// deletion without crashing. This needs to move to the DB to avoid
// the RAM consumption of a large number of collections.
const allCollectionObjIDs = new Set();
const storeKindIDToName = new Map();

const storeKindInfo = {
Expand Down Expand Up @@ -566,9 +572,13 @@ export function makeCollectionManager(
}

function deleteCollection(vobjID) {
if (!allCollectionObjIDs.has(vobjID)) {
return false; // already deleted
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 what situation could cause us to get to here, but if we do I'm betting it's probably a bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

This could be triggered by the violence I'm doing to the refcounts: the "delete all collections" pass deletes collection-1, then a subsequent BOYD drops the last (real) reference to it, so the refmgr tries to delete it again, causing a crash when it can't find the export status.

This will be a lot better with the scheme we cooked up (#5090).

}
const { id, subid } = parseVatSlot(vobjID);
const kindName = storeKindIDToName.get(`${id}`);
const collection = summonCollectionInternal(false, 'GC', subid, kindName);
allCollectionObjIDs.delete(vobjID);

const doMoreGC = collection.clearInternal(true);
let priorKey = '';
Expand All @@ -583,6 +593,18 @@ export function makeCollectionManager(
return doMoreGC;
}

function deleteAllVirtualCollections() {
const vobjIDs = Array.from(allCollectionObjIDs).sort();
for (const vobjID of vobjIDs) {
const { id } = parseVatSlot(vobjID);
const kindName = storeKindIDToName.get(`${id}`);
const { durable } = storeKindInfo[kindName];
if (!durable) {
deleteCollection(vobjID);
}
}
}

function makeCollection(label, kindName, keySchema, valueSchema) {
assert.typeof(label, 'string');
assert(storeKindInfo[kindName]);
Expand All @@ -606,6 +628,7 @@ export function makeCollectionManager(
JSON.stringify(serialize(harden(schemata))),
);
syscall.vatstoreSet(prefixc(collectionID, '|label'), label);
allCollectionObjIDs.add(vobjID);

return [
vobjID,
Expand Down Expand Up @@ -843,6 +866,7 @@ export function makeCollectionManager(

return harden({
initializeStoreKindInfo,
deleteAllVirtualCollections,
makeScalarBigMapStore,
makeScalarBigWeakMapStore,
makeScalarBigSetStore,
Expand Down
66 changes: 35 additions & 31 deletions packages/SwingSet/src/liveslots/liveslots.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { insistMessage } from '../lib/message.js';
import { makeVirtualReferenceManager } from './virtualReferences.js';
import { makeVirtualObjectManager } from './virtualObjectManager.js';
import { makeCollectionManager } from './collectionManager.js';
import { releaseOldState } from './stop-vat.js';

const DEFAULT_VIRTUAL_OBJECT_CACHE_SIZE = 3; // XXX ridiculously small value to force churn for testing

Expand Down Expand Up @@ -1265,32 +1266,6 @@ function build(
vom.insistAllDurableKindsReconnected();
}

/**
* @returns { Promise<void> }
*/
async function stopVat() {
assert(didStartVat);
assert(!didStopVat);
didStopVat = true;

// Pretend that userspace rejected all non-durable promises. We
// basically do the same thing that `thenReject(p,
// vpid)(rejection)` would have done, but we skip ahead to the
// syscall.resolve part. The real `thenReject` also does
// pRec.reject(), which would give control to userspace (who might
// have re-imported the promise and attached a .then to it), and
// stopVat() must not allow userspace to gain agency.

const rejectCapData = m.serialize('vat upgraded');
const vpids = Array.from(deciderVPIDs.keys()).sort();
const rejections = vpids.map(vpid => [vpid, true, rejectCapData]);
if (rejections.length) {
syscall.resolve(rejections);
}
// eslint-disable-next-line no-use-before-define
await bringOutYourDead();
}

/**
* @param { VatDeliveryObject } delivery
* @returns { void | Promise<void> }
Expand Down Expand Up @@ -1330,10 +1305,6 @@ function build(
result = startVat(vpCapData);
break;
}
case 'stopVat': {
result = stopVat();
break;
}
default:
assert.fail(X`unknown delivery type ${type}`);
}
Expand All @@ -1354,6 +1325,37 @@ function build(
return undefined;
}

/**
* @returns { Promise<void> }
*/
async function stopVat() {
assert(didStartVat);
assert(!didStopVat);
didStopVat = true;

try {
await releaseOldState({
m,
deciderVPIDs,
syscall,
exportedRemotables,
addToPossiblyDeadSet,
slotToVal,
valToSlot,
dropExports,
retireExports,
vrm,
collectionManager,
bringOutYourDead,
vreffedObjectRegistry,
});
} catch (e) {
console.log(`-- error during stopVat()`);
console.log(e);
throw e;
}
}

/**
* Do things that should be done (such as flushing caches to disk) after a
* dispatch has completed and user code has relinquished agency.
Expand Down Expand Up @@ -1402,9 +1404,11 @@ function build(
*/
async function dispatch(delivery) {
// We must short-circuit dispatch to bringOutYourDead here because it has to
// be async
// be async, same for stopVat
if (delivery[0] === 'bringOutYourDead') {
return meterControl.runWithoutMeteringAsync(bringOutYourDead);
} else if (delivery[0] === 'stopVat') {
return meterControl.runWithoutMeteringAsync(stopVat);
} else {
// Start user code running, record any internal liveslots errors. We do
// *not* directly wait for the userspace function to complete, nor for
Expand Down
Loading