Skip to content

Commit

Permalink
fix(swingset): define 'meterControl' to disable metering
Browse files Browse the repository at this point in the history
The `meterControl` tool will give liveslots a way to temporarily disable
metering, so it can perform GC-sensitive operations without affecting the
vat's metering results.

Each supervisor provides their own version: all are dummy facades except for
XS.

refs #3458
  • Loading branch information
warner committed Aug 13, 2021
1 parent d63810f commit ef9e21d
Show file tree
Hide file tree
Showing 11 changed files with 201 additions and 2 deletions.
28 changes: 27 additions & 1 deletion packages/SwingSet/docs/liveslots.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,37 @@ Some useful things cannot be serialized: they will trigger an error.
Uncertain:

* Maps: This might actually serialize as pass-by-presence, since it has no non-function properties (in fact it has no own properties at all, they all live on `Map.prototype`, whose properties are all functions). The receiving side gets a Presence, not a Map, but invoking e.g. `E(p).get(123)` will return a promise that will be fulfilled with the results of `m.get(123)` on the sending side.
* WeakMaps: same, except the values being passed into `get()` would be coming from the deserializer, and so they might not be that useful (especially since Liveslots does not implement distributed GC yet).
* WeakMaps: same, except the values being passed into `get()` would be coming from the deserializer, and so they might not be that useful.

## How things get serialized

* pass-by-presence objects: `{@qclass: "slot", index: slotIndex}`
* local Promises: passed as a promise
* promise returned by `E(p).foo()`: passes as a promise, with pipelining enabled
* Function: rejected (todo: wrap)

## Garbage Collection vs Metering

When a swingset kernel is part of a consensus machine, the visible state must be a deterministic function of userspace activity. Every member kernel must perform the same set of operations.

However we are not yet confident that the timing of garbage collection will remain identical between kernels that experience different patterns of snapshot+restart. In particular, up until recently, the amount of "headroom" in the XS memory allocator was reset upon snapshot reload: the new XS engine only allocates as much RAM as the snapshot needs, whereas before the snapshot was taken, the RAM footprint could have been larger (e.g. if a large number of objects we allocated and then released), leading to more "headroom". Automatic GC is triggered by an attempt to allocate space which cannot be satisfied by this headroom, so it will happen more frequently in the post-reload engine than before the snapshot. See issues #3428, #3458, and #3577 for details.

We rely upon the engine to only invoke finalizer callback at explicitly-deterministic times, but we tolerate (guard against) objects becoming collected spontaneously, which will e.g. cause a WeakRef to become "dead" (`wr.deref() === undefined`) at a random point in the middle of a turn. Any code which calls `wr.deref`, or is conditionally executed/skipped according to the results, is "GC-sensitive". This includes `convertSlotToVal`, and therefore `m.unserialize`.

We cannot allow metering results to diverge between validators, because:

* 1: it might make the difference between the crank completing successfully, and the vat being terminated for a per-crank metering fault
* 2: it will change the large-scale Meter value, which is reported to userspace
* 3: it might cause the runPolicy to finish the block earlier on one validator than on others

all of which would cause a consensus failure.

To prevent this, we run most of the "inbound" side of liveslots without metering. This includes the first turn of all `dispatch.*` methods, which runs entirely within liveslots:

* `dispatch.deliver` performs argument deserialization in the first turn, then executes user code in the second and subsequent turns
* `dispatch.notify` does the same
* the GC deliveries (`dispatch.dropExport`, etc) only use one turn

We also disable metering when deserializing the return value from a (synchronous) device call, and when retiring a promise ID (which touches `slotToVal`).

Finally, we disable metering for all turns of the post-crank GC `finish()` call. This excludes all invocations of the finalizer callbacks, as well as all the `processDeadSet` code which is highly sensitive to the results.
55 changes: 55 additions & 0 deletions packages/SwingSet/src/kernel/dummyMeterControl.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// @ts-check
import { assert } from '@agoric/assert';

export function makeDummyMeterControl() {
let meteringDisabled = 0;

function isMeteringDisabled() {
return !!meteringDisabled;
}

function assertIsMetered(msg) {
assert(!meteringDisabled, msg);
}

function assertNotMetered(msg) {
assert(!!meteringDisabled, msg);
}

function runWithoutMetering(thunk) {
meteringDisabled += 1;
try {
return thunk();
} finally {
meteringDisabled -= 1;
}
}

async function runWithoutMeteringAsync(thunk) {
meteringDisabled += 1;
try {
return await thunk();
} finally {
meteringDisabled -= 1;
}
}

// return a version of f that runs outside metering
function unmetered(f) {
function wrapped(...args) {
return runWithoutMetering(() => f(...args));
}
return harden(wrapped);
}

/** @type { MeterControl } */
const meterControl = {
isMeteringDisabled,
assertIsMetered,
assertNotMetered,
runWithoutMetering,
runWithoutMeteringAsync,
unmetered,
};
return harden(meterControl);
}
2 changes: 2 additions & 0 deletions packages/SwingSet/src/kernel/kernel.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { insistMessage, insistVatDeliveryResult } from '../message.js';
import { insistDeviceID, insistVatID } from './id.js';
import { makeKernelSyscallHandler, doSend } from './kernelSyscall.js';
import { makeSlogger, makeDummySlogger } from './slogger.js';
import { makeDummyMeterControl } from './dummyMeterControl.js';
import { getKpidsToRetire } from './cleanup.js';
import { processNextGCAction } from './gc-actions.js';

Expand Down Expand Up @@ -812,6 +813,7 @@ export default function buildKernel(
FinalizationRegistry,
waitUntilQuiescent,
gcAndFinalize,
meterControl: makeDummyMeterControl(),
});
const vatManagerFactory = makeVatManagerFactory({
allVatPowers,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import engineGC from '../../engine-gc.js';
import { WeakRef, FinalizationRegistry } from '../../weakref.js';
import { makeGcAndFinalize } from '../../gc-and-finalize.js';
import { waitUntilQuiescent } from '../../waitUntilQuiescent.js';
import { makeDummyMeterControl } from '../dummyMeterControl.js';
import { makeLiveSlots } from '../liveSlots.js';
import {
makeSupervisorDispatch,
Expand Down Expand Up @@ -70,11 +71,13 @@ parentPort.on('message', ([type, ...margs]) => {
makeMarshal,
testLog,
};

const gcTools = harden({
WeakRef,
FinalizationRegistry,
waitUntilQuiescent,
gcAndFinalize: makeGcAndFinalize(engineGC),
meterControl: makeDummyMeterControl(),
});
const ls = makeLiveSlots(
syscall,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { makeMarshal } from '@agoric/marshal';
import engineGC from '../../engine-gc.js';
import { WeakRef, FinalizationRegistry } from '../../weakref.js';
import { makeGcAndFinalize } from '../../gc-and-finalize.js';
import { makeDummyMeterControl } from '../dummyMeterControl.js';
import {
arrayEncoderStream,
arrayDecoderStream,
Expand Down Expand Up @@ -86,11 +87,13 @@ fromParent.on('data', ([type, ...margs]) => {
makeMarshal,
testLog,
};

const gcTools = harden({
WeakRef,
FinalizationRegistry,
waitUntilQuiescent,
gcAndFinalize: makeGcAndFinalize(engineGC),
meterControl: makeDummyMeterControl(),
});
const ls = makeLiveSlots(
syscall,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,66 @@ function workerLog(first, ...args) {

workerLog(`supervisor started`);

function makeMeterControl() {
let meteringDisabled = 0;

function isMeteringDisabled() {
return !!meteringDisabled;
}

function assertIsMetered(msg) {
assert(!meteringDisabled, msg);
}
function assertNotMetered(msg) {
assert(!!meteringDisabled, msg);
}

function runWithoutMetering(thunk) {
const limit = globalThis.currentMeterLimit();
const before = globalThis.resetMeter(0, 0);
meteringDisabled += 1;
try {
return thunk();
} finally {
globalThis.resetMeter(limit, before);
meteringDisabled -= 1;
}
}

async function runWithoutMeteringAsync(thunk) {
const limit = globalThis.currentMeterLimit();
const before = globalThis.resetMeter(0, 0);
meteringDisabled += 1;
try {
return await thunk();
} finally {
globalThis.resetMeter(limit, before);
meteringDisabled -= 1;
}
}

// return a version of f that runs outside metering
function unmetered(f) {
function wrapped(...args) {
return runWithoutMetering(() => f(...args));
}
return harden(wrapped);
}

/** @type { MeterControl } */
const meterControl = {
isMeteringDisabled,
assertIsMetered,
assertNotMetered,
runWithoutMetering,
runWithoutMeteringAsync,
unmetered,
};
return harden(meterControl);
}

const meterControl = makeMeterControl();

/**
* Wrap byte-level protocols with tagged array codec.
*
Expand Down Expand Up @@ -180,6 +240,7 @@ function makeWorker(port) {
// FIXME(mfig): Here is where GC-per-crank is silently disabled.
// We need to do a better analysis of the tradeoffs.
gcAndFinalize: makeGcAndFinalize(gcEveryCrank && globalThis.gc),
meterControl,
});

const ls = makeLiveSlots(
Expand Down
17 changes: 17 additions & 0 deletions packages/SwingSet/src/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -201,3 +201,20 @@
* crankFailed: (details: {}) => PolicyOutput,
* } } RunPolicy
*/

/**
* The MeterControl object gives liveslots a mechanism to disable metering for certain GC-sensitive
* regions of code. Only the XS worker can actually do metering, but we track the enabled/disabled
* status on all workers, so that the assertions can be exercised more thoroughly (via non-XS unit
* tests). MeterControl.isMeteringDisabled()===false does not mean metering is happening, it just
* means that MeterControl isn't disabling it.
*
* @typedef {Object} MeterControl
* @property {() => boolean} isMeteringDisabled Ask whether metering is currently disabled.
* @property {*} assertIsMetered
* @property {*} assertNotMetered
* @property {*} runWithoutMetering Run a callback outside metering
* @property {*} runWithoutMeteringAsync Run an async callback outside metering
* @property {*} unmetered Wrap a callback with runWithoutMetering
*
*/
2 changes: 2 additions & 0 deletions packages/SwingSet/test/liveslots-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import engineGC from '../src/engine-gc.js';
import { WeakRef, FinalizationRegistry } from '../src/weakref.js';
import { waitUntilQuiescent } from '../src/waitUntilQuiescent.js';
import { makeGcAndFinalize } from '../src/gc-and-finalize.js';
import { makeDummyMeterControl } from '../src/kernel/dummyMeterControl.js';
import { makeLiveSlots } from '../src/kernel/liveSlots.js';

export function buildSyscall() {
Expand Down Expand Up @@ -46,6 +47,7 @@ export function makeDispatch(
FinalizationRegistry,
waitUntilQuiescent,
gcAndFinalize: makeGcAndFinalize(engineGC),
meterControl: makeDummyMeterControl(),
});
const { setBuildRootObject, dispatch } = makeLiveSlots(
syscall,
Expand Down
2 changes: 2 additions & 0 deletions packages/SwingSet/test/test-liveslots.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { assert, details as X } from '@agoric/assert';
import engineGC from '../src/engine-gc.js';
import { waitUntilQuiescent } from '../src/waitUntilQuiescent.js';
import { makeGcAndFinalize } from '../src/gc-and-finalize.js';
import { makeDummyMeterControl } from '../src/kernel/dummyMeterControl.js';
import { makeLiveSlots } from '../src/kernel/liveSlots.js';
import { buildSyscall, makeDispatch } from './liveslots-helpers.js';
import {
Expand Down Expand Up @@ -906,6 +907,7 @@ function makeMockGC() {
getAllFRs,
waitUntilQuiescent,
gcAndFinalize: mockGCAndFinalize,
meterControl: makeDummyMeterControl(),
});
}

Expand Down
7 changes: 6 additions & 1 deletion packages/SwingSet/test/test-marshal.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,16 @@ import { Far } from '@agoric/marshal';
import { makePromiseKit } from '@agoric/promise-kit';

import { WeakRef, FinalizationRegistry } from '../src/weakref.js';
import { makeDummyMeterControl } from '../src/kernel/dummyMeterControl.js';
import { makeMarshaller } from '../src/kernel/liveSlots.js';

import { buildVatController } from '../src/index.js';

const gcTools = harden({ WeakRef, FinalizationRegistry });
const gcTools = harden({
WeakRef,
FinalizationRegistry,
meterControl: makeDummyMeterControl(),
});

async function prep() {
const config = {};
Expand Down
23 changes: 23 additions & 0 deletions packages/SwingSet/test/test-metering-control.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// eslint-disable-next-line import/order
import { test } from '../tools/prepare-test-env-ava.js';

import { makeDummyMeterControl } from '../src/kernel/dummyMeterControl.js';

test('dummy meter control', async t => {
const mc = makeDummyMeterControl();
t.false(mc.isMeteringDisabled());
t.throws(mc.assertNotMetered);
mc.runWithoutMetering(mc.assertNotMetered);
let x = 0;
mc.runWithoutMetering(() => (x = 1));
t.is(x, 1);
await mc.runWithoutMeteringAsync(() => (x = 2));
t.is(x, 2);
function set(y) {
x = y;
return 'yes';
}
const unmeteredSet = mc.unmetered(set);
t.is(unmeteredSet(3), 'yes');
t.is(x, 3);
});

0 comments on commit ef9e21d

Please sign in to comment.