remove install-metering-and-ses from apps and most unit tests #3393

warner · 2021-06-22T02:29:36Z

At this point, any dynamic vats that need to be metered should be run under xsnap, not local, which means we no longer need to use @agoric/install-metering-and-ses to wrap the globals with metered versions. Those wrappers cause a significant slowdown in all Node.js-side code (like the kernel), even when metering is not enabled, so it's nice to remove them.

This changes spawner and zoe tests which need to exercise metering to use an xsnap worker. All instances of import '@agoric/install-metering-and-ses' have been changed to import '@agoric/install-ses'. This reduces the runtime of those tests by 4x-6x (in one case, Zoe's test-crashingContract.js, reducing it from 340s to 60s).

Most importantly, this changes our two main applications (cosmic-swingset and the solo node) in the same way. We don't have measurements on the performance changes, but we expect them to be noticeable.

refs #3373

Probably doesn't close it yet, there are still some swingset internal tests that exercise that metering code which needs to be changed.

refs #3373

The tests that actually need to exercise metering now use `xs-worker` instead of enabling global metering. This speeds those tests up by 4x-6x. refs #3373

All metered vats should now be using xsnap, so we no longer need to meter 'local' vats, and can remove the performance-costly global metering wrapper. refs #3373

warner · 2021-06-22T02:43:29Z

I ran my testnet-load-generator recipe to test this (agoric start local-chain and agoric start local-solo). I didn't see any complaints by the chain node, but the solo node said:

agoric: deploy: running /home/warner/stuff/agoric/agoric-sdk/packages/dapp-svelte-wallet/api/deploy.js
2021-06-22T02:36:27.314Z SwingSet: kernel: note: makeGetMeter() cannot enable global metering, app must import @agoric/install-metering-and-ses

which makes me think the wallet deploy is using the spawner (which does not make a new dynamic vat? I forget) and the within-vat metering code (the other thing we want to rip out). I suspect that means whatever code is being loaded is subject to the metering transform but not running with meter-transformed globals, so it's even less meter-confined than before. @michaelfig can you imagine this being a problem for the next week? I'm guessing it's ok.

The loadgen sequence seemed to run without problems. I haven't yet investigated how performance might have changed.

katelynsills

LGTM!

michaelfig · 2021-06-22T04:25:53Z

the wallet deploy is using the spawner (which does not make a new dynamic vat? I forget)

The spawner does make a new dynamic vat.

The problem is apparently that the defaultManagerType in packages/solo/* is 'local'. Can you please change this to 'xs-worker'? Then I suspect there will be no more warnings, and all our code will use xsnap unless overridden with SWINGSET_WORKER_TYPE.

$ rg "'local'" ../solo/src/
../solo/src/init-basedir.js
24:    defaultManagerType = env.SWINGSET_WORKER_TYPE || 'local',

../solo/src/main.js
81:          defaultManagerType: process.env.SWINGSET_WORKER_TYPE || 'local',

../solo/src/vat-http.js [irrelevant]

michaelfig

Looks good, just needs to update packages/solo to use 'xs-worker'.

This changes the solo node to use xs-worker (xsnap) vats instead of local ones, which has a secondary effect of silencing a warning from the spawner as it attempts to create a metered vat without the Node.js globals having been wrapped for metering. refs #3393

warner · 2021-06-22T04:46:00Z

@michaelfig I'm cool with switching solo to xs-worker.. I think we hesitated on doing that for the previous phase, but I'm feeling good about it now.

I made the two changes you recommended, but I'm still seeing that warning message on the client:

Open CapTP connection to ws://127.0.0.1:8000/private/captp...oooooo2021-06-22T04:43:25.300Z chain-cosmos-sdk: could not find peer mailbox - agoric1ne5c4yqe07hgtg6fccpxraxdm7ws2hs48kug6r: rpc error: code = Unknown desc = could not get peer mailbox: unknown request
2021-06-22T04:43:25.344Z chain-cosmos-sdk: gas estimate: 48763
oo
agoric: deploy: running /home/warner/stuff/agoric/agoric-sdk/packages/dapp-svelte-wallet/api/deploy.js
2021-06-22T04:43:27.937Z SwingSet: kernel: note: makeGetMeter() cannot enable global metering, app must import @agoric/install-metering-and-ses
2021-06-22T04:43:30.045Z chain-cosmos-sdk: gas estimate: 44547

I'm wondering if something in agoric deploy is trying to meter the deploy script it's loading? Probably not a big deal, but it might point to a place where we're actually expecting metering under Node.js and probably have to make a decision to let that go.

But, I'll land this once CI passes.

michaelfig · 2021-06-22T04:59:51Z

I'm still seeing that warning message on the client

Maybe:

$ rg '"local"' ../solo
../solo/solo-config.json
3:  "defaultManagerType": "local",

???

The config file needed updating too.

warner · 2021-06-22T05:06:45Z

That did the trick, thanks!

Will land as soon as CI finishes.

This changes the solo node to use xs-worker (xsnap) vats instead of local ones, which has a secondary effect of silencing a warning from the spawner as it attempts to create a metered vat without the Node.js globals having been wrapped for metering. refs #3393

warner added 4 commits June 21, 2021 19:20

chore(spawner): stop using install-metering-and-ses

d77c5fe

refs #3373

chore(zoe): stop using install-metering-and-ses

8c66401

The tests that actually need to exercise metering now use `xs-worker` instead of enabling global metering. This speeds those tests up by 4x-6x. refs #3373

fix(solo): stop using install-metering-and-ses

b21fb61

All metered vats should now be using xsnap, so we no longer need to meter 'local' vats, and can remove the performance-costly global metering wrapper. refs #3373

fix(cosmic-swingset): stop using install-metering-and-ses

3317007

All metered vats should now be using xsnap, so we no longer need to meter 'local' vats, and can remove the performance-costly global metering wrapper. refs #3373

warner requested review from katelynsills and michaelfig June 22, 2021 02:43

warner self-assigned this Jun 22, 2021

warner added Zoe package: Zoe spawner Spawner and contractHost cosmic-swingset package: cosmic-swingset labels Jun 22, 2021

warner added this to the Testnet: Stress Test Phase milestone Jun 22, 2021

katelynsills approved these changes Jun 22, 2021

View reviewed changes

michaelfig reviewed Jun 22, 2021

View reviewed changes

fix(solo): finish changing default vatManager to 'xs-worker'

ea19295

The config file needed updating too.

warner mentioned this pull request Jun 22, 2021

still doing metering transform on node in solo, cosmic-swingset, tests #3373

Closed

warner enabled auto-merge June 22, 2021 05:29

warner merged commit f3f299b into master Jun 22, 2021

warner deleted the 3373-remove-global-metering branch June 22, 2021 05:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove install-metering-and-ses from apps and most unit tests #3393

remove install-metering-and-ses from apps and most unit tests #3393

warner commented Jun 22, 2021

warner commented Jun 22, 2021

katelynsills left a comment

michaelfig commented Jun 22, 2021 •

edited

Loading

michaelfig left a comment

warner commented Jun 22, 2021 •

edited

Loading

michaelfig commented Jun 22, 2021

warner commented Jun 22, 2021

remove install-metering-and-ses from apps and most unit tests #3393

remove install-metering-and-ses from apps and most unit tests #3393

Conversation

warner commented Jun 22, 2021

warner commented Jun 22, 2021

katelynsills left a comment

Choose a reason for hiding this comment

michaelfig commented Jun 22, 2021 • edited Loading

michaelfig left a comment

Choose a reason for hiding this comment

warner commented Jun 22, 2021 • edited Loading

michaelfig commented Jun 22, 2021

warner commented Jun 22, 2021

michaelfig commented Jun 22, 2021 •

edited

Loading

warner commented Jun 22, 2021 •

edited

Loading