-
Notifications
You must be signed in to change notification settings - Fork 237
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
SwingSet tools for load testing contracts #11158
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to defer to other reviewers.
@@ -171,6 +173,8 @@ export const getNodeTestVaultsConfig = async ({ | |||
); | |||
} | |||
|
|||
Object.assign(config, configOverrides); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This relies on loadSwingsetConfigFile
not hardening its result, right? That seems odd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @dckc. And since I still have a goal to either replace this or reïmplement it on top of makeCosmicSwingsetTestKit
, I'd like any changes to be consistent with that.
const configFromFile: SwingSetConfig & { coreProposals?: any[] } = NonNullish(
await loadSwingsetConfigFile(configPath),
);
const config: SwingSetConfig & { coreProposals?: any[] } = {
...configFromFile,
// exclude Pegasus from core proposals because it relies on IBC to Golang
// that isn't running
coreProposals: configFromFile.coreProposals?.filter(
spec => spec !== '@agoric/pegasus/scripts/init-core.js',
),
...configOverrides,
defaultManagerType,
bundleCachePath: bundleDir,
};
await fsAmbientPromises.mkdir(bundleDir, { recursive: true });
// make an almost-certainly-unique file name with a fixed-length prefix
const configFilenameParts = …
@@ -338,6 +338,10 @@ export async function makeSwingsetController( | |||
return kernel.reapAllVats(previousVatPos); | |||
}, | |||
|
|||
async snapshotAllVats() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it important to skip a test for this new feature?
kernelKeeper.vatIsAlive(vatID) || Fail`${q(vatID)}: not alive`; | ||
const vatKeeper = kernelKeeper.provideVatKeeper(vatID); | ||
const vatOptions = vatKeeper.getOptions(); | ||
|
||
if (!vatOptions.useTranscript) { | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this change well enough to review it without a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second @dckc's request for test coverage of the new maybeSaveSnapshot
parameter.
Deploying agoric-sdk with
|
Latest commit: |
097ce77
|
Status: | ✅ Deploy successful! |
Preview URL: | https://1e6cadde.agoric-sdk.pages.dev |
Branch Preview URL: | https://mhofman-boot-load-test-tools.agoric-sdk.pages.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good, but could use a few tweaks.
@@ -171,6 +173,8 @@ export const getNodeTestVaultsConfig = async ({ | |||
); | |||
} | |||
|
|||
Object.assign(config, configOverrides); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @dckc. And since I still have a goal to either replace this or reïmplement it on top of makeCosmicSwingsetTestKit
, I'd like any changes to be consistent with that.
const configFromFile: SwingSetConfig & { coreProposals?: any[] } = NonNullish(
await loadSwingsetConfigFile(configPath),
);
const config: SwingSetConfig & { coreProposals?: any[] } = {
...configFromFile,
// exclude Pegasus from core proposals because it relies on IBC to Golang
// that isn't running
coreProposals: configFromFile.coreProposals?.filter(
spec => spec !== '@agoric/pegasus/scripts/init-core.js',
),
...configOverrides,
defaultManagerType,
bundleCachePath: bundleDir,
};
await fsAmbientPromises.mkdir(bundleDir, { recursive: true });
// make an almost-certainly-unique file name with a fixed-length prefix
const configFilenameParts = …
@@ -141,13 +141,15 @@ export const keyArrayEqual = ( | |||
* @param options.configPath - Path to the base config file | |||
* @param options.defaultManagerType - SwingSet manager type to use | |||
* @param options.discriminator - Optional string to include in the config filename | |||
* @param options.configOverrides - Other SwingSet options to set in the config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param options.configOverrides - Other SwingSet options to set in the config | |
* @param options.configOverrides - Other SwingSet options to set in the config | |
(may be overridden by more specific options such as `bundleDir` and | |
`defaultManagerType`) |
@@ -417,6 +421,7 @@ type AckBehaviorType = (typeof AckBehavior)[keyof typeof AckBehavior]; | |||
* @param options.defaultManagerType - SwingSet manager type to use | |||
* @param options.harness - Optional run harness | |||
* @param options.resolveBase - Base URL or path for resolving module paths | |||
* @param options.configOverrides - Other SwingSet options to set in the config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param options.configOverrides - Other SwingSet options to set in the config | |
* @param options.configOverrides - Other SwingSet options to set in the config | |
(may be overridden by more specific options such as `bundleDir` and | |
`defaultManagerType`) |
|
||
function reapAllVats(previousVatPos = {}) { | ||
/** @type {Record<string, number>} */ | ||
const currentVatPos = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a slight preference for these vat transcript positions to be represented as a Map instance rather than an object (because arbitrarily-keyed plain objects are unnecessary hazards, and Map better communicates intent anyway).
* is satisified | ||
* | ||
* @param {VatID} vatID | ||
* @param {number} [forceThreshold] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* is satisified | |
* | |
* @param {VatID} vatID | |
* @param {number} [forceThreshold] | |
* is satisfied or a lower explicit delivery count has been reached. | |
* | |
* @param {VatID} vatID | |
* @param {number} [minDeliveryCount] |
); | ||
|
||
for (let i = 0; i < 3; i += 1) { | ||
const lastExported = exportedPromises.slice(-1)[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const lastExported = exportedPromises.slice(-1)[0]; | |
const lastExported = exportedPromises.at(-1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typing of Array<T>.at(pos: number): T | undefined
which is extremely annoying in these cases.
async getExport() { | ||
return Far('Bootstrap exported', {}); | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages/vats/tools/bootstrap-chain-reflective.js and packages/SwingSet/tools/vat-puppet.js strive to eliminate the need for vat code dedicated to any particular tests. Consider replacing packages/SwingSet/test/reap-all/bootstrap-reap-all.js with the former and packages/SwingSet/test/reap-all/vat-dumbo.js with the latter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vat-puppet.js
doesn't support defining a kind or another virtual storage mechanism allowing to make one object hold onto another. I might be able to use makeRemotable
for the base object in the chain, but that's about it.
This seems not worth the effort.
} | ||
await c.run(); | ||
|
||
// Drop our interest in the intermediary promises without gaining an interest in their resolution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Drop our interest in the intermediary promises without gaining an interest in their resolution | |
// Drop our interest in the intermediary promises without gaining an interest | |
// in their settlement |
t.deepEqual(postReapReapPos, postMessagesReapPos); | ||
checkQueues([]); | ||
|
||
// Drop our interest in the last promise of the chain, dropping the refcount onto its resolution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Drop our interest in the last promise of the chain, dropping the refcount onto its resolution | |
// Drop our interest in the last promise of the chain without gaining an | |
// interest in its settlement |
for (const vat of ['v11', 'v10', 'v9', 'v1']) { | ||
const newReapPos = c.reapAllVats(prevReapPos); | ||
checkQueues([vat]); | ||
const { [vat]: prevVatPos, ...prevOtherPos } = prevReapPos; | ||
const { [vat]: newVatPos, ...newOtherPos } = newReapPos; | ||
t.log(`vat ${vat} prev reap pos ${prevVatPos}, new reap pos ${newVatPos}`); | ||
t.deepEqual(newOtherPos, prevOtherPos); | ||
prevReapPos = newReapPos; | ||
await c.run(); | ||
checkQueues([]); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (const vat of ['v11', 'v10', 'v9', 'v1']) { | |
const newReapPos = c.reapAllVats(prevReapPos); | |
checkQueues([vat]); | |
const { [vat]: prevVatPos, ...prevOtherPos } = prevReapPos; | |
const { [vat]: newVatPos, ...newOtherPos } = newReapPos; | |
t.log(`vat ${vat} prev reap pos ${prevVatPos}, new reap pos ${newVatPos}`); | |
t.deepEqual(newOtherPos, prevOtherPos); | |
prevReapPos = newReapPos; | |
await c.run(); | |
checkQueues([]); | |
} | |
for (const vatID of ['v11', 'v10', 'v9', 'v1']) { | |
const { [vatID]: prevVatPos, ...prevOtherPos } = prevReapPos; | |
const newReapPos = c.reapAllVats(prevReapPos); | |
checkQueues([vatID]); | |
const { [vatID]: newVatPos, ...newOtherPos } = newReapPos; | |
t.log(`vat ${vatID} reap pos prev ${prevVatPos}, new ${newVatPos}`); | |
t.deepEqual(newOtherPos, prevOtherPos); | |
prevReapPos = newReapPos; | |
await c.run(); | |
checkQueues([]); | |
} |
// prettier-ignore | ||
const allVats = | ||
harden(['v1', 'v6', 'v7', 'v8', 'v5', 'v2', 'v4', 'v9', 'v10', 'v11']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// prettier-ignore | |
const allVats = | |
harden(['v1', 'v6', 'v7', 'v8', 'v5', 'v2', 'v4', 'v9', 'v10', 'v11']); | |
const { activeVats } = c.getStatus(); | |
t.log(activeVats.map(({ id, options }) => `${id}: ${options.name}`)); | |
// As noted in kernel.js, the comms vat is not reapable. | |
const reapable = activeVats.flatMap(({ id, options }) => | |
options.name === 'comms' ? [] : [id], | |
); | |
// We care about the dynamic vats. | |
t.true(reapable.length >= 7); // bootstrap, staticDumbo{1,2,3}, dyn{1,2,3} | |
const [dyn1, dyn2, dyn3] = reapable.slice(-3); |
// Because of our chained references, our GC shakes lose an object for | ||
// each reap round. | ||
let prevReapPos = postMessagesReapPos; | ||
for (const vat of ['v11', 'v10', 'v9', 'v1']) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accompanying the above suggestion:
for (const vat of ['v11', 'v10', 'v9', 'v1']) { | |
for (const vat of [dyn3, dyn2, dyn1, c.vatNameToID('bootstrap')]) { |
// Create a chain of references | ||
|
||
/** @type {string[]} */ | ||
const exportedPromises = []; | ||
exportedPromises.push( | ||
/** @type {string} */ (c.queueToVatRoot('bootstrap', 'getExport')), | ||
); | ||
|
||
for (let i = 0; i < 3; i += 1) { | ||
const lastExported = exportedPromises.slice(-1)[0]; | ||
exportedPromises.push( | ||
/** @type {string} */ ( | ||
c.queueToVatObject(dynamicRoots[i], 'makeHolder', [kslot(lastExported)]) | ||
), | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typing of
Array<T>.at(pos: number): T | undefined
which is extremely annoying in these cases.
That seems to be an issue anyway, so we might as well use type casting:
// Create a chain of references | |
/** @type {string[]} */ | |
const exportedPromises = []; | |
exportedPromises.push( | |
/** @type {string} */ (c.queueToVatRoot('bootstrap', 'getExport')), | |
); | |
for (let i = 0; i < 3; i += 1) { | |
const lastExported = exportedPromises.slice(-1)[0]; | |
exportedPromises.push( | |
/** @type {string} */ ( | |
c.queueToVatObject(dynamicRoots[i], 'makeHolder', [kslot(lastExported)]) | |
), | |
); | |
} | |
// Create a chain of references | |
const exportedPromises = [ | |
/** @type {string} */ (c.queueToVatRoot('bootstrap', 'getExport')), | |
]; | |
for (let i = 0; i < 3; i += 1) { | |
const lastExported = /** @type {string} */ (exportedPromises.at(-1)); | |
exportedPromises.push( | |
/** @type {string} */ ( | |
c.queueToVatObject(dynamicRoots[i], 'makeHolder', [kslot(lastExported)]) | |
), | |
); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use noUncheckedIndexedAccess
so not a problem in practice. I just wish JSDoc had non-null type assertions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw I'd be happy to convert the SwingSet tests to .ts
refs: #10955
Best reviewed commit-by-commit
Description
While reproducing the heap growth investigation for FastUSDC and smart wallet, I found myself wanting more comparable GC and snapshot points.
These change make it possible for the host, in this case a load test like for FastUSDC, to perform GC and snapshotting of vats at a known point. It also performs these only for vats that have seen any activity during the load test.
There is a non trivial change to
maybeSaveSnapshot
which I claim is equivalent to before, just avoids paging in vats unnecessarily when snapshotting is manually triggered.Security Considerations
None. This introduces a new host capability that isn't used in production hosts.
Scaling Considerations
None
Documentation Considerations
Hopefully we can polish the FastUSDC load test as a reference example.
Testing Considerations
Manually tested in using mhofman/fu-impact
Upgrade Considerations
None, testing only