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

Allocate ID numbers from counters that are kept in persistent storage #4898

Merged
merged 1 commit into from
Mar 24, 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
6 changes: 2 additions & 4 deletions packages/SwingSet/src/liveslots/collectionManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export function makeCollectionManager(
syscall,
vrm,
allocateExportID,
allocateCollectionID,
convertValToSlot,
convertSlotToVal,
registerValue,
Expand Down Expand Up @@ -546,8 +547,6 @@ export function makeCollectionManager(
return doMoreGC;
}

let nextCollectionID = 1;

function makeCollection(label, kindName, keySchema, valueSchema) {
assert.typeof(label, 'string');
assert(storeKindInfo[kindName]);
Expand All @@ -557,8 +556,7 @@ export function makeCollectionManager(
assertPattern(valueSchema);
schemata.push(valueSchema);
}
const collectionID = nextCollectionID;
nextCollectionID += 1;
const collectionID = allocateCollectionID();
const kindID = obtainStoreKindID(kindName);
const vobjID = `o+${kindID}/${collectionID}`;

Expand Down
79 changes: 69 additions & 10 deletions packages/SwingSet/src/liveslots/liveslots.js
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,6 @@ function build(
const disavowalError = harden(Error(`this Presence has been disavowed`));

const importedPromisesByPromiseID = new Map(); // vpid -> { resolve, reject }
let nextExportID = 1;
let nextPromiseID = 5;

function makeImportedPresence(slot, iface = `Alleged: presence ${slot}`) {
// Called by convertSlotToVal for type=object (an `o-NN` reference). We
Expand Down Expand Up @@ -438,6 +436,46 @@ function build(
return Remotable(iface);
}

let idCounters;
let idCountersAreDirty = false;

function initializeIDCounters() {
if (!idCounters) {
const raw = syscall.vatstoreGet('idCounters');
if (raw) {
idCounters = JSON.parse(raw);
} else {
idCounters = {};
Copy link
Member

Choose a reason for hiding this comment

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

I think this case needs a idCountersAreDirty= so we'll do a write during startVat, instead of waiting until the first delivery that allocates something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The base state always the same ({}) so we don't have to write it until there's an actual change.

}
}
}

function allocateNextID(name, initialValue = 1) {
if (!idCounters) {
// Normally `initializeIDCounters` would be called from startVat, but some
// tests bypass that so this is a backstop. Note that the invocation from
// startVat is there to make vatStore access patterns a bit more
// consistent from one vat to another, principally as a confusion
// reduction measure in service of debugging; it is not a correctness
// issue.
initializeIDCounters();
}
if (!idCounters[name]) {
idCounters[name] = initialValue;
}
const result = idCounters[name];
idCounters[name] += 1;
idCountersAreDirty = true;
return result;
}

function flushIDCounters() {
if (idCountersAreDirty) {
syscall.vatstoreSet('idCounters', JSON.stringify(idCounters));
idCountersAreDirty = false;
}
}

// TODO: fix awkward non-orthogonality: allocateExportID() returns a number,
// allocatePromiseID() returns a slot, exportPromise() uses the slot from
// allocatePromiseID(), exportPassByPresence() generates a slot itself using
Expand All @@ -446,14 +484,22 @@ function build(
// use a slot from the corresponding allocateX

function allocateExportID() {
const exportID = nextExportID;
nextExportID += 1;
return exportID;
// We start exportIDs with 1 because 'o+0' is always automatically
// pre-assigned to the root object.
return allocateNextID('exportID', 1);
Copy link
Member

Choose a reason for hiding this comment

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

We didn't have one before, but let's add a note that we use "1" because it's bigger than the "o+0" that's hard-coded for the root object.

The "5" for promises is arbitrary and could be reduced to 0 without problems, but the inital exportID must never be less than 1.

}

function allocateCollectionID() {
return allocateNextID('collectionID', 1);
}

function allocatePromiseID() {
const promiseID = nextPromiseID;
nextPromiseID += 1;
// The starting point for numbering promiseIDs is pretty arbitrary. We start
// from 5 as a very minor aid to debugging: It helps, when puzzling over
// trace logs and the like, for the numbers in the various species of IDs to
// be a little out of sync and thus a little less similar to each other when
// jumbled together.
const promiseID = allocateNextID('promiseID', 5);
return makeVatSlot('promise', true, promiseID);
}

Expand Down Expand Up @@ -539,6 +585,7 @@ function build(
syscall,
vrm,
allocateExportID,
allocateCollectionID,
// eslint-disable-next-line no-use-before-define
convertValToSlot,
unmeteredConvertSlotToVal,
Expand Down Expand Up @@ -1164,6 +1211,7 @@ function build(
});
}

initializeIDCounters();
const vatParameters = m.unserialize(vatParametersCapData);
baggage = collectionManager.provideBaggage();

Expand Down Expand Up @@ -1266,6 +1314,14 @@ function build(
return undefined;
}

/**
* Do things that should be done (such as flushing caches to disk) after a
* dispatch has completed and user code has relinquished agency.
*/
function afterDispatchActions() {
flushIDCounters();
}

/**
* This 'dispatch' function is the entry point for the vat as a whole: the
* vat-worker supervisor gives us VatDeliveryObjects (like
Expand Down Expand Up @@ -1315,11 +1371,14 @@ function build(
// any promise it returns to fire.
const p = Promise.resolve(delivery).then(unmeteredDispatch);

// Instead, we wait for userspace to become idle by draining the
// promise queue. We return 'p' so that any bugs in liveslots that
// Instead, we wait for userspace to become idle by draining the promise
// queue. We clean up and then return 'p' so any bugs in liveslots that
// cause an error during unmeteredDispatch will be reported to the
// supervisor (but only after userspace is idle).
return gcTools.waitUntilQuiescent().then(() => p);
return gcTools.waitUntilQuiescent().then(() => {
afterDispatchActions();
return p;
});
}
}
harden(dispatch);
Expand Down
8 changes: 6 additions & 2 deletions packages/SwingSet/test/liveslots-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,11 @@ export function matchVatstoreDelete(key) {
}

export function matchVatstoreSet(key, value) {
return { type: 'vatstoreSet', key, value };
if (value !== undefined) {
return { type: 'vatstoreSet', key, value };
} else {
return { type: 'vatstoreSet', key };
}
}

export function matchRetireExports(...slots) {
Expand All @@ -256,7 +260,7 @@ export function matchRetireImports(...slots) {
}

export function validate(v, match) {
v.t.deepEqual(v.log.shift(), match);
v.t.like(v.log.shift(), match);
}

export function validateDone(v) {
Expand Down
11 changes: 11 additions & 0 deletions packages/SwingSet/test/stores/test-storeGC.js
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ function validateUpdate(v, key, before, after) {
function validateMakeAndHold(v, rp) {
validateCreateStore(v, mainHeldIdx);
validateReturned(v, rp);
validate(v, matchVatstoreSet('idCounters'));
validateDone(v);
}

Expand Down Expand Up @@ -442,6 +443,9 @@ function validateImportAndHold(v, rp, idx) {
validate(v, matchVatstoreGet(`vc.${idx}.|label`, `store #${idx}`));
}
validateReturned(v, rp);
if (idx === NONE) {
validate(v, matchVatstoreSet('idCounters'));
}
validateDone(v);
}

Expand All @@ -461,6 +465,7 @@ function validateCreateHolder(v, idx) {
}

function validateInit(v) {
validate(v, matchVatstoreGet('idCounters', NONE));
validate(v, matchVatstoreGet('baggageID', NONE));
validate(v, matchVatstoreGet('storeKindIDTable', NONE));
validate(
Expand Down Expand Up @@ -858,6 +863,7 @@ function validatePrepareStore3(
validate(v, matchVatstoreSet(`vc.${base + 2}.|entryCount`, '1'));

validateReturned(v, rp);
validate(v, matchVatstoreSet('idCounters'));
if (!nonVirtual) {
validateRefCountCheck(v, contentRef, '3');
if (checkES) {
Expand Down Expand Up @@ -971,6 +977,7 @@ test.serial('store refcount management 3', async t => {
validate(v, matchVatstoreGet(`vc.${base + 2}.|entryCount`, '0'));
validate(v, matchVatstoreSet(`vc.${base + 2}.|entryCount`, '1'));
validateReturned(v, rp);
validate(v, matchVatstoreSet('idCounters'));
validateStatusCheck(v, mapRef(mainHeldIdx), '1', NONE);
validateStatusCheck(v, mapRef(base), '1', NONE);
validateStatusCheck(v, mapRef(base + 1), '1', NONE);
Expand Down Expand Up @@ -1078,6 +1085,7 @@ test.serial('remotable refcount management 1', async t => {
let rp = await dispatchMessage('makeAndHoldRemotable');
validateInit(v);
validateReturned(v, rp);
validate(v, matchVatstoreSet('idCounters'));
validateDone(v);

rp = await dispatchMessage('prepareStore3');
Expand Down Expand Up @@ -1109,6 +1117,7 @@ test.serial('remotable refcount management 2', async t => {
let rp = await dispatchMessage('makeAndHoldRemotable');
validateInit(v);
validateReturned(v, rp);
validate(v, matchVatstoreSet('idCounters'));
validateDone(v);

rp = await dispatchMessage('prepareStore3');
Expand Down Expand Up @@ -1163,6 +1172,7 @@ test.serial('verify store weak key GC', async t => {
validate(v, matchVatstoreGet(`vc.${setID}.|${mapRef(keyID)}`, '1'));
validate(v, matchVatstoreSet(`vc.${setID}.${ordinalKey}`, nullValString));
validateReturned(v, rp);
validate(v, matchVatstoreSet('idCounters'));
validateDone(v);

t.is(testHooks.countCollectionsForWeakKey(mapRef(keyID)), 2);
Expand Down Expand Up @@ -1246,6 +1256,7 @@ test.serial('verify presence weak key GC', async t => {
validate(v, matchVatstoreGet(`vc.${setID}.|${presenceRef}`, '1'));
validate(v, matchVatstoreSet(`vc.${setID}.${ordinalKey}`, nullValString));
validateReturned(v, rp);
validate(v, matchVatstoreSet('idCounters'));

t.is(testHooks.countCollectionsForWeakKey(presenceRef), 2);
t.is(testHooks.storeSizeInternal(mapRef(mapID)), 1);
Expand Down
2 changes: 2 additions & 0 deletions packages/SwingSet/test/test-baggage.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ function validateCreateBaggage(v, idx) {
}

function validateSetup(v) {
validate(v, matchVatstoreGet('idCounters', NONE));
validate(v, matchVatstoreGet('baggageID', NONE));
validate(v, matchVatstoreGet('storeKindIDTable', NONE));
validate(
Expand Down Expand Up @@ -80,5 +81,6 @@ test.serial('exercise baggage', async t => {
validate(v, matchVatstoreGet('vc.1.|entryCount', '1'));
validate(v, matchVatstoreSet('vc.1.|entryCount', '2'));
validateReturned(v, rp);
validate(v, matchVatstoreSet('idCounters'));
validateDone(v);
});
Loading