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

feat: use xsnap worker CPU meter and start reporting consumption #2384

Merged
merged 5 commits into from
Feb 11, 2021

Conversation

michaelfig
Copy link
Member

Refs #2322

We implement the same basic policy as in the local worker: refill the meter with a constant value (1e7 computrons) before every crank and terminate the vat if it consumes more than that in a crank.

There is still a long way to go, but at least now we detect and terminate an xsnap vat upon infinite loops.

@michaelfig michaelfig added the metering charging for execution (was: package: tame-metering and transform-metering) label Feb 10, 2021
@michaelfig michaelfig requested a review from dckc February 10, 2021 15:27
@michaelfig michaelfig self-assigned this Feb 10, 2021
dckc
dckc previously approved these changes Feb 10, 2021
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

I'm hesitant to move forward with -DmxMetering=1 while Moddable considers it experimental. I'd like to hear from @dtribble on that, but presuming you're already in communication with him about it, I approve.

I'd really rather not have any in the type annotations, but I don't suppose that's critical.

@michaelfig michaelfig force-pushed the mfig-2322-initial-xs-metering branch from 8b64ebf to 0b177df Compare February 10, 2021 20:45
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

I think I should withdraw my approval until we have clarity on @warner 's question:

shouldn't the meterStats be part of the OK that comes back from the worker?

@michaelfig michaelfig requested review from warner and dckc February 10, 2021 21:10
@michaelfig michaelfig dismissed dckc’s stale review February 10, 2021 21:11

Need another pass due to important changes.

@michaelfig michaelfig force-pushed the mfig-2322-initial-xs-metering branch from 8b581db to b418db5 Compare February 10, 2021 23:55
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

I was about to approve this, but then ava hangs when I yarn test because some xsnap process is hanging around.

@dckc
Copy link
Member

dckc commented Feb 11, 2021

... ava hangs when I yarn test because some xsnap process is hanging around.

diagnosis: makefiles/lin/xsnap.mk is missing -DmxMetering=1

How did this get thru ci?

eek! packages/xsnap doesn't seem to get its tests run in ci: https://github.com/Agoric/agoric-sdk/pull/2384/checks?check_run_id=1875722804

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

makefiles/lin/xsnap.mk is missing -DmxMetering=1

and the test-quick ci job is missing packages/xsnap altogether!

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.

Tentative approval, please check the comments and see if the recommended changes are ones you're comfortable with. The XSNAP_VERSION thing is the one I'm most concerned about. The message-result protocol is the one I thought the most about but it might not be worth doing anything about.

@michaelfig
Copy link
Member Author

... ava hangs when I yarn test because some xsnap process is hanging around.

diagnosis: makefiles/lin/xsnap.mk is missing -DmxMetering=1

I've added preflight checks to xsnap.c to reject metering unless -DmxMetering.

How did this get thru ci?

eek! packages/xsnap doesn't seem to get its tests run in ci: https://github.com/Agoric/agoric-sdk/pull/2384/checks?check_run_id=1875722804

Added!

@michaelfig michaelfig enabled auto-merge February 11, 2021 03:32
@michaelfig michaelfig merged commit 43344ad into master Feb 11, 2021
@michaelfig michaelfig deleted the mfig-2322-initial-xs-metering branch February 11, 2021 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metering charging for execution (was: package: tame-metering and transform-metering)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants