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

Price feed benchmarks #8603

Merged
merged 3 commits into from
Dec 8, 2023
Merged

Price feed benchmarks #8603

merged 3 commits into from
Dec 8, 2023

Conversation

FUDCo
Copy link
Contributor

@FUDCo FUDCo commented Dec 4, 2023

Implement benchmarks for price feeds with and without liquidation.

This includes adjustments to the boot tooling to accommodate its use for benchmarking, as well as improvements and enhancements to the bechmarkerator API that were required to make these benchmarks possible.

Closes #8496

@FUDCo FUDCo added enhancement New feature or request performance Performance related issues labels Dec 4, 2023
@FUDCo FUDCo requested review from turadg and warner December 4, 2023 04:04
@FUDCo FUDCo self-assigned this Dec 4, 2023
@FUDCo FUDCo changed the base branch from master to control-profiling December 4, 2023 04:11
@FUDCo FUDCo force-pushed the 8496-price-feed-benchmarks branch 2 times, most recently from 9110cd9 to a6d2f14 Compare December 4, 2023 08:42
packages/benchmark/doc/benchmarkerator.md Show resolved Hide resolved
@@ -90,6 +100,22 @@ import { makeWalletFactoryDriver } from '@agoric/boot/tools/drivers.ts';
* // benchmark context parameter of subsequent calls into the benchmark object.
* setup?: (context: BenchmarkContext) => Promise<Record<string, unknown> | undefined>,
*
* // Optional setup method for an individual round. This is executed as part
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 these comments won't appear in IDE or auto-gen output.

The type of the Benchmark object is better expressed in .ts. We could have a .d.ts for it (with a sibling .js dummy) or just move this file to .ts syntax. Are you okay with the latter? I could do it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been frustrated (in a way I think we've discussed previously) by the need to, in essence, write the declarations twice: once for the type checker, using one syntax, and once for the human reader, in a different syntax to provide documentation text that the JS type syntax did not permit. Since the benchmarks already have to be executed using tsx anyway, we're already in the TS world here, so I think converting this to TS may be the best course. If you're game for that I'm happy to have you do it, but I'd prefer to do it in a later PR as I've already been burned a couple times by TS conversions happening in the middle of trying to do something else unrelated.

Copy link
Member

Choose a reason for hiding this comment

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

Good call. A full conversion could wait for another PR. I just pushed a commit to solve this specific duplicate issue by extracting the types to a types.ts.

packages/benchmark/src/benchmarkerator.js Show resolved Hide resolved
packages/benchmark/src/benchmarkerator.js Show resolved Hide resolved
packages/benchmark/src/benchmarkerator.js Show resolved Hide resolved
packages/boot/tools/liquidation.ts Outdated Show resolved Hide resolved
@@ -45,6 +49,7 @@ import { makeWalletFactoryDriver } from '@agoric/boot/tools/drivers.ts';
* options: Record<string, string>,
* argv: string[],
* actors: Record<string, import('@agoric/boot/tools/drivers.ts').SmartWalletDriver>,
* tools: Record<string, unknown>,
Copy link
Member

Choose a reason for hiding this comment

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

these are known. Please type them. You could have a makeTools() function and make this have type ReturnType<typeof makeTools>

packages/boot/tools/liquidation.ts Show resolved Hide resolved
agoricNamesRemotes,
walletFactoryDriver,
governanceDriver,
like,
Copy link
Member

Choose a reason for hiding this comment

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

please maintain the t. since it's using the Ava API (even if benchmarkerator doesn't have full Ava).

Suggested change
like,
t,

It's type can be t: Pick<ExecutionContext, 'like'>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I need some additional TS syntax support on this one. tsc complains:
error TS2693: 'Pick' only refers to a type, but is being used as a value here.
It looks to me like a type annotation, but apparently I'm not holding it right.

Copy link
Member

Choose a reason for hiding this comment

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

I couldn't repro that but I did run into another kink, the API has a .skip property: https://github.com/avajs/ava/blob/af5684dff58a79da862fa57fad0e946985f730a2/types/assertions.d.cts#L186-L194

For now I just pushed a commit that suppresses the problem. (Along with some annotations for drivers)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weirdly, tsc is happy if I leave the type of the partial implementation of t undeclared.

Copy link
Member

Choose a reason for hiding this comment

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

it's only silent because there's no type annotation for t in the called function. Your force push removed a0dce27 and 9b5062f

I'm pushing another fix on top of your change.

@FUDCo FUDCo force-pushed the 8496-price-feed-benchmarks branch from 9b5062f to 04d2b00 Compare December 5, 2023 23:10
@warner
Copy link
Member

warner commented Dec 8, 2023

Just to confirm my understanding:

This price-feed benchmark code is taking advantage of unit-test code tooling, by calling into a priceFeedDriver, defined in packages/boot/tools/drivers.ts, which is basically defined as:

    async setPrice(price: number) {
      await Promise.all(
        oracleWallets.map(w =>
          w.executeOfferMaker(
            Offers.fluxAggregator.PushPrice,
            {
              offerId: `push-${price}-${Date.now()}`,
              roundId,
              unitPrice: BigInt(price * 1_000_000),
            },
            adminOfferId,
          ),
        ),
      );
      // prepare for next round
      oracleWallets.push(NonNullish(oracleWallets.shift()));
      roundId += 1n;
      // TODO confirm the new price is written to storage
    },

and w.executeOfferMaker boils down to

      const offerCapData = marshaller.toCapData(
        harden({
          method: 'executeOffer',
          offer,
        }),
      );
      return EV(walletPresence).handleBridgeAction(offerCapData, true);

and EV boils down to something like controller.queueToVatObject and some stuff to let it run.

So the round starts with the marshalling of the same message that vat-bridge would send to vat-walletFactory, and then injecting that message into the kernel.

Relative to the real chain, the benchmark is doing slightly more work (marshalling the wallet message) and slightly less work (not involving device-bridge or vat-bridge). I think that's fine: we can measure how much time we're spending in vat-bridge from our mainnet logs (I don't think it's much), and it would be too much hassle to involve device-bridge. It might be nice to exclude the extra marshalling call, but I don't think it will will significant.

One thing to note for the future.. having the benchmark share code with unit tests is convenient, but we need remain aware of diverging interests. For example, the TODO in setPrice suggests that at some point the unit test will acquire code to look at chain storage and assert that the new price has been written. That's not something we'd want the benchmark doing (it'd be work that the real chain operation does not do, and thus make our benchmark results artifically worse).

I'm hoping we can get to a point where benchmarks are less entwined with the unit test approach. Something wheree packages/boot/ exports some constructor function that builds the wallet spend message (ideally in the same form we drop into vat-bridge), and some other functions/configs that help us build a small chain (complete with zoe, vaults, wallets, etc), and then the benchmark's setup can build the chain, and the benchmark's round() can deliver the spend message to vat-board and wait for controller.run() to finish.

Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

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

Looks good

await priceFeedDrivers[collateralBrandKey].setPrice(9.99);

// check nothing liquidating yet
const liveSchedule = readLatest('published.auction.schedule');
Copy link
Member

Choose a reason for hiding this comment

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

This readLatest/assertion is overhead in the benchmarked code, right? Like, the benchmark results will be slightly worse than the real chain, because the real chain doesn't do these additional assertions?

Probably trivial, but I wanted to make sure I understand what is/is-not in the benchmarked path.

Copy link
Member

Choose a reason for hiding this comment

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

True. Since the vstorage is cached the difference will be small. Perhaps worth verifying.

Regardless, there can be a flag for whether to verify any values. Perhaps whether the final argument "t" is provided. That might be better than mocking it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, though readLatest is reading from the simulated vstorage, which amounts to a Map lookup, so the overhead is modest.

@FUDCo FUDCo force-pushed the control-profiling branch from 4cc5873 to b680821 Compare December 8, 2023 01:43
Base automatically changed from control-profiling to master December 8, 2023 02:35
This includes adjustments to the boot tooling to accommodate its use
for benchmarking, as well as improvements and enhancements to the
bechmarkerator API that were required to make these benchmarks possible.

Closes #8496
@FUDCo FUDCo force-pushed the 8496-price-feed-benchmarks branch from 23fab84 to e73ff32 Compare December 8, 2023 02:37
@FUDCo FUDCo added the automerge:rebase Automatically rebase updates, then merge label Dec 8, 2023
@FUDCo FUDCo force-pushed the 8496-price-feed-benchmarks branch from e73ff32 to 881bf9d Compare December 8, 2023 03:01
@mergify mergify bot merged commit bc4a41c into master Dec 8, 2023
67 checks passed
@mergify mergify bot deleted the 8496-price-feed-benchmarks branch December 8, 2023 03:32
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 enhancement New feature or request performance Performance related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement price feed benchmarks
3 participants