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

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

Closed
dckc opened this issue Jun 21, 2021 · 5 comments
Closed

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

dckc opened this issue Jun 21, 2021 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@dckc
Copy link
Member

dckc commented Jun 21, 2021

Description

Now that metering is taking shape on XS, is there any value in metering on the node side?

In local integration testing (with agoric start) I saw:

2021-06-20T15:22:10.176Z ...

  at assertLooksLikeValue (packages/ERTP/src/amountMath.js:134:16)
  at meteredConstructor.make (packages/ERTP/src/amountMath.js:198:11)
  at Alleged: purse.actions.send (packages/dapp-svelte-wallet/api/src/lib-wallet.js:278:58)

meteredConstructor.make is almost certainly an artifact of a metering transform.

Searching for import '@agoric/install-metering-and-ses'; turns up:

  • packages/solo/src/entrypoint.js
  • packages/cosmic-swingset/src/entrypoint.js

plus in tests:

  • packages/zoe/test/swingsetTests/zoe-metering/test-zoe-metering.js
  • packages/SwingSet/test/vat-admin/test-replay.js
  • packages/SwingSet/test/metering/test-dynamic-vat-metered.js
  • packages/zoe/test/swingsetTests/brokenContracts/test-crashingContract.js
  • packages/zoe/test/swingsetTests/zoe/test-zoe.js

This search used github's code search; I've seen it miss things.

To Reproduce

IOU?

cc @katelynsills

@dckc dckc added the bug Something isn't working label Jun 21, 2021
@katelynsills
Copy link
Contributor

It'd be great to be able to remove this. I tried removing and makeGetMeter in packages/SwingSet/src/kernel/metering.js seemed to require @agoric/install-metering-and-ses

@dckc dckc added this to the Testnet: Stress Test Phase milestone Jun 21, 2021
@dckc
Copy link
Member Author

dckc commented Jun 21, 2021

I'd like us to make an explicit decision about whether to get this into the near-term release.

@warner
Copy link
Member

warner commented Jun 22, 2021

I'm experimenting now with adding config.defaultManagerType = 'xs-worker' in the dozen-ish unit tests that use swingset and exercise what happens when a vat exceeds its meter (and replacing the import '@agoric/install-metering-and-ses' with import '@agoric/install-ses'). It reduced the runtime by 4x in one case (each change appeared to contribute 2x).

@warner
Copy link
Member

warner commented Jun 22, 2021

Zoe's test-crashingConstract.js goes from 340s to 60s.

warner added a commit that referenced this issue Jun 22, 2021
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
warner added a commit that referenced this issue Jun 22, 2021
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 added a commit that referenced this issue Jun 22, 2021
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
Copy link
Member

warner commented Jun 22, 2021

PR #3393 will remove install-metering-and-ses from all non-SwingSet packages, including our two main applications (cosmic-swingset and ag-solo). That removes the tame-metering wrappers which "tame" the globals (adding code to increment a meter, if getMeter is available, when global objects like Array are invoked).

We still enforce transform-metering on dynamic vats run on the local worker, however without wrapped globals, this wouldn't be sound (confined code could assemble some builtins to consume more CPU than their meter allowance should permit). Also, the transformation is kinda slow. Our applications are not using xs-worker vats, not local, but any 3rd-party applications, and some remaining swingset tests, still might use local dynamic vats. A cleanup task is to track those down and fix them, then probably disable metering === true on local vats entirely (and remove the transformation code).

@warner warner closed this as completed in f3f299b Jun 22, 2021
kriskowal pushed a commit that referenced this issue Jun 22, 2021
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
kriskowal pushed a commit that referenced this issue Jun 22, 2021
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
kriskowal pushed a commit that referenced this issue Jun 22, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants