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

chore: let makeRunUtils caller provide run policy #10348

Merged
merged 5 commits into from
Dec 2, 2024
Merged

Conversation

dckc
Copy link
Member

@dckc dckc commented Oct 28, 2024

Description

Support computron measurements in boostrap tests for performance testing by letting makeRunUtils caller provide a way to make a run policy.

Security Considerations

A swingset controller relies on the run policy to be well-behaved. But the caller of makeRunUtils can only shoot themselves in the foot.

Scaling Considerations

helps measure scaling issues

Documentation Considerations

JSDocs with static types provide adequate docs for an internal tool such as this.

Testing Considerations

Integration with a couple bootstrap tests is included.
See also #10254 .

Upgrade Considerations

n/a

Copy link

cloudflare-workers-and-pages bot commented Oct 28, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 19ae5da
Status: ✅  Deploy successful!
Preview URL: https://37d410c7.agoric-sdk.pages.dev
Branch Preview URL: https://dc-perf-meter.agoric-sdk.pages.dev

View logs

@dckc dckc marked this pull request as draft October 28, 2024 17:43
@dckc
Copy link
Member Author

dckc commented Oct 28, 2024

hang on... I should integrate this into an existing bootstrap test...

@dckc
Copy link
Member Author

dckc commented Oct 28, 2024

darn... I'm having trouble getting test/bootstrapTests/price-feed-replace.test.ts to run on xsnap.

I should have tested that it works before I put so much work into doing a computron measurement.

unsettled value for kp3542
----- FlxAgg.7  17 new quote { quoteAmount: { brand: Object [Alleged: quote brand] {}, value: [ { amountIn: { brand: [Object], value: 1_000_000n }, amountOut: { brand: [Object], value: 15_990_000n }, timer: Object [Alleged: timerService] {}, timestamp: { absValue: 121n, timerBrand: [Object] } } ] }, quotePayment: Object [Alleged: quote payment] {} }
  ✘ [fail]: setupVaults; run updatePriceFeeds proposals Rejected promise returned by test
    ℹ setPrice computrons 65536531n
    ℹ building @agoric/builders/scripts/inter-protocol/updatePriceFeeds.js
    ℹ REJECTED from ava test.serial("setupVaults; run updatePriceFeeds proposals"): (Error#1)
    ℹ Error#1: unsettled value for kp3542
    ℹ     at makeError (file:///home/connolly/projects/agoric-sdk/node_modules/ses/src/error/assert.js:350:61)
          at fail (file:///home/connolly/projects/agoric-sdk/node_modules/ses/src/error/assert.js:482:20)
          at Fail (file:///home/connolly/projects/agoric-sdk/node_modules/ses/src/error/assert.js:492:39)
          at queueAndRun (file:///home/connolly/projects/agoric-sdk/packages/SwingSet/tools/run-utils.js:49:19)
          at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
          at async proposeParams (file:///home/connolly/projects/agoric-sdk/packages/boot/tools/drivers.ts:340:17)
          at async Object.changeParams (file:///home/connolly/projects/agoric-sdk/packages/boot/tools/drivers.ts:409:7)
          at async updateVaultManagerParams (file:///home/connolly/projects/agoric-sdk/packages/boot/test/tools/changeVaultParams.ts:22:3)
          at async file:///home/connolly/projects/agoric-sdk/packages/boot/test/bootstrapTests/price-feed-replace.test.ts:94:3

@dckc
Copy link
Member Author

dckc commented Oct 29, 2024

ah. I had a bug in turning off the policy, so when I started contracts, I tripped the 65Mc limit.

@dckc
Copy link
Member Author

dckc commented Oct 29, 2024

It's now integrated into a couple of existing bootstrap tests, though only when SWINGSET_WORKER_TYPE=xsnap:

$ cd agoric-sdk/packages/boot
$ SWINGSET_WORKER_TYPE=xsnap yarn test test/bootstrapTests/price-feed-replace.test.ts
...
  ✔ setupVaults; run updatePriceFeeds proposals (1m 27.6s)
    ℹ setPrice computrons 65536531n
...

$ SWINGSET_WORKER_TYPE=xsnap yarn test test/bootstrapTests/orchestration.test.ts
...
  ✔ stakeAtom - smart wallet (13.8s)
    ℹ makeAccount computrons 15231491n
...

@dckc dckc marked this pull request as ready for review October 29, 2024 19:09
@dckc
Copy link
Member Author

dckc commented Oct 30, 2024

re docs: how does benchmarkerator relate?

@dckc
Copy link
Member Author

dckc commented Nov 22, 2024

@gibson042 merge conflicts crept in.

I presume you don't mind that I rebased to address them.

Copy link
Member

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

Nice! Nearly every comment I have is a naming suggestion, and none of them are blockers.

* @param {boolean} [ignoreBlockLimit]
* @returns {import('./launch-chain.js').ChainRunPolicy}
*/
export function computronCounter(
Copy link
Member

Choose a reason for hiding this comment

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

I'm thrilled to see this pulled out of launch-chain.js, but we actually already have packages/SwingSet/src/lib/runPolicies.js. I think merging it in would be out-of-scope for this PR because it's a cosmic-swingset ChainRunPolicy rather than merely a SwingSet RunPolicy, although in the fullness of time that's probably exactly what should happen because the extra methods of the former aren't really blockchain-specific.

At any rate, focusing on the present, I'd like to see this function renamed for accuracy, e.g.:

Suggested change
export function computronCounter(
export function makeExclusiveCleanupRunPolicy(

(or similar).

and its containing file renamed to something like chain-run-polic{y,ies}.js.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for all the feedback, but due to how much stuff I'm juggling, I'm inclined to leave the swingset refactoring to folks like yourself that spend more time in the swingset code.

Comment on lines +34 to +41
{ beansPerUnit, vatCleanupBudget },
ignoreBlockLimit = false,
) {
const {
[BeansPerBlockComputeLimit]: blockComputeLimit,
[BeansPerVatCreation]: vatCreation,
[BeansPerXsnapComputron]: xsnapComputron,
} = beansPerUnit;
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: for better comprehensibility, especially at call sites, consider splitting the overly-generic beansPerUnit parameter to be less directly coupled to cosmic-swingset chain parameters:

Suggested change
{ beansPerUnit, vatCleanupBudget },
ignoreBlockLimit = false,
) {
const {
[BeansPerBlockComputeLimit]: blockComputeLimit,
[BeansPerVatCreation]: vatCreation,
[BeansPerXsnapComputron]: xsnapComputron,
} = beansPerUnit;
{ computronScale, computeLimit, vatCreationCost, vatCleanupBudget },
) {
// Prefer just the new names, but to clarify their mapping:
// const blockComputeLimit = computeLimit;
// const vatCreation = vatCreationCost;
// const xsnapComputron = computronScale;
const ignoreBlockLimit = computeLimit === Infinity;

and then call sites look something like:

  async function bootstrapBlock(blockHeight, blockTime, params) {
    
    // This is before the initial block, we need to finish processing the
    // entire bootstrap before opening for business.
    const runPolicyConfig = makeRunPolicyConfig(params, true);
    const runPolicy = makeExclusiveCleanupRunPolicy(runPolicyConfig);
    
  }
  async function endBlock(blockHeight, blockTime, params) {
    
    // If we have work to complete this block, it needs to run to completion.
    // It will also run to completion any work that swingset still had pending.
    const neverStop = runThisBlock.size() > 0;

    // Process the work for this block using a dedicated Cranker with a stateful
    // run policy.
    const runPolicyConfig = makeRunPolicyConfig(params, neverStop);
    const runPolicy = makeExclusiveCleanupRunPolicy(runPolicyConfig);
    
  }

with

/**
 * Derive SwingSet run policy configuration from cosmic-swingset parameters
 * (maximum computation per block, computron scale factor, etc.).
 *
 * @param {object} params
 * @param {BeansPerUnit} params.beansPerUnit
 * @param {import('@agoric/swingset-vat').CleanupBudget} [params.vatCleanupBudget]
 * @param {boolean} [ignoreComputeLimit]
 */
function makeRunPolicyConfig(params, ignoreComputeLimit = false) {
  const { beansPerUnit, vatCleanupBudget } = params;
  const {
    [BeansPerBlockComputeLimit]: requestedComputeLimit,
    [BeansPerVatCreation]: vatCreationCost,
    [BeansPerXsnapComputron]: computronScale,
  } = beansPerUnit;
  const computeLimit = ignoreComputeLimit ? Infinity : requestedComputeLimit;
  return { computronScale, computeLimit, vatCreationCost, vatCleanupBudget };
}

Comment on lines +10 to +14
/** @typedef {{ provideRunPolicy: () => RunPolicy | undefined }} RunPolicyMaker */

/**
* @param {import('../src/controller/controller.js').SwingsetController} controller
* @param {RunPolicyMaker} [perfTool]
Copy link
Member

Choose a reason for hiding this comment

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

Attempted future-proofing: perfTool: RunPolicyMakerharness: RunHarness (and likewise in packages/boot/tools/supports.ts and the other files)

Suggested change
/** @typedef {{ provideRunPolicy: () => RunPolicy | undefined }} RunPolicyMaker */
/**
* @param {import('../src/controller/controller.js').SwingsetController} controller
* @param {RunPolicyMaker} [perfTool]
/** @typedef {{ provideRunPolicy: () => RunPolicy | undefined }} RunHarness */
/**
* @param {import('../src/controller/controller.js').SwingsetController} controller
* @param {RunHarness} [harness] provides run policies for the controller

Comment on lines +693 to +699
/** @param {boolean} x */
usePolicy: x => {
counting = x;
if (!counting) {
policy = undefined;
}
},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/** @param {boolean} x */
usePolicy: x => {
counting = x;
if (!counting) {
policy = undefined;
}
},
/** @param {boolean} enabled */
usePolicy: enabled => {
counting = enabled;
if (!counting) {
policy = undefined;
}
},

policy = undefined;
}
},
totalCount: () => (policy?.totalBeans() || 0n) / c2b,
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion:

Suggested change
totalCount: () => (policy?.totalBeans() || 0n) / c2b,
totalComputronCount: () => (policy?.totalBeans() || 0n) / c2b,

@@ -660,3 +669,45 @@ export const makeSwingsetTestKit = async (
};
};
export type SwingsetTestKit = Awaited<ReturnType<typeof makeSwingsetTestKit>>;

export const makePolicyProvider = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export const makePolicyProvider = () => {
export const makeRunPolicyProvider = () => {

Comment on lines +53 to +56
const cleanupPossible =
Object.values(remainingCleanups).length > 0
? Object.values(remainingCleanups).some(n => n > 0)
: defaultCleanupBudget > 0;
Copy link
Member

Choose a reason for hiding this comment

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

Please feel free to clean up this function a bit.

Suggested change
const cleanupPossible =
Object.values(remainingCleanups).length > 0
? Object.values(remainingCleanups).some(n => n > 0)
: defaultCleanupBudget > 0;
const cleanupPossible = Object.values(remainingCleanups).some(n => n > 0);

@dckc dckc added the automerge:rebase Automatically rebase updates, then merge label Nov 27, 2024
in a couple tests:

$ cd agoric-sdk/packages/boot
$ SWINGSET_WORKER_TYPE=xsnap yarn test test/bootstrapTests/price-feed-replace.test.ts
...
  ✔ setupVaults; run updatePriceFeeds proposals (1m 27.6s)
    ℹ setPrice computrons 65536531n

$ SWINGSET_WORKER_TYPE=xsnap yarn test test/bootstrapTests/orchestration.test.ts
...
  ✔ stakeAtom - smart wallet (13.8s)
    ℹ makeAccount computrons 15231491n
@mergify mergify bot merged commit 4a76042 into master Dec 2, 2024
81 checks passed
@mergify mergify bot deleted the dc-perf-meter branch December 2, 2024 18:17
gibson042 added a commit that referenced this pull request Dec 2, 2024
gibson042 added a commit that referenced this pull request Dec 2, 2024
gibson042 added a commit that referenced this pull request Dec 2, 2024
Rename types/functions/parameters as suggested at:
* #10348 (comment)
* #10348 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants