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: initial xsnap vat metering for compute, stack, allocation #2988

Merged
merged 11 commits into from
Apr 28, 2021

Conversation

dckc
Copy link
Member

@dckc dckc commented Apr 27, 2021

fixes #2972

note #2980, #2979 are postponed.

@dckc dckc marked this pull request as draft April 27, 2021 23:32
@dckc
Copy link
Member Author

dckc commented Apr 27, 2021

I'm switching to draft until more tests pass. I'd like to squash a couple fixups...

@dckc dckc force-pushed the 2972-xs-test-metering branch 2 times, most recently from 48d36f7 to 1866870 Compare April 28, 2021 00:37
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

Just one important suggestion, and looks good.

@dckc dckc marked this pull request as ready for review April 28, 2021 01:28
@dckc
Copy link
Member Author

dckc commented Apr 28, 2021

@warner I'm standing by for your review. Let me know if you'd rather I just went ahead based on @kriskowal 's.

packages/xsnap/src/api.js Outdated Show resolved Hide resolved
packages/xsnap/src/xsnap.c Outdated Show resolved Hide resolved
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.

Everything looks good to me (minor suggestions made) except the question of the api.js import. If we can find an answer quickly, I'd like to incorporate it. If we can't, I'm ok with landing it as-is and fixing it properly next week.

@warner
Copy link
Member

warner commented Apr 28, 2021

@dckc when you looked at the slog output, you saw the new allocation meter value appear in the delivery-results event, right? That means it's being plumbed all the way back, great. If not, we might need to fix something in supervisor-helper.js to pass it through.

@dckc
Copy link
Member Author

dckc commented Apr 28, 2021

@dckc when you looked at the slog output, you saw the new allocation meter value appear in the delivery-results event, right?

Yes; this is documented in the .dr part of the (perhaps somewhat cryptic) comment:

	// SLOGFILE=out.slog agoric start local-chain
	// jq -s '.|.[]|.dr[2].allocate' < out.slog|grep -v null|sort -u | sort -nr
	int MB = 1024 * 1024;
	int measured_max = 30 * MB;
	the->allocationLimit = 10 * measured_max;

@dckc dckc merged commit 62f2eef into master Apr 28, 2021
@dckc dckc deleted the 2972-xs-test-metering branch April 28, 2021 19:49
@warner
Copy link
Member

warner commented Apr 29, 2021

@dckc thanks for landing this. A brief nit: the commits in this branch aren't really independent, so I think it'd have been better to land them with a merge commit, rather than a fast-forward (e.g. see the previously-landed PR, #2982, which used 7be8d71 to merge 6 related commits). My goal is for the left-side path of a git branch diagram (the first parent of each commit) to always pass all tests, and a merge commit is how we can retain this property in the face of groups of commits that need to be landed all-or-nothing. Thanks!

@dckc
Copy link
Member Author

dckc commented Apr 29, 2021

Oh! I forgot about that distinction between a merge and a rebase. Thanks for clarifying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add allocation metering to XS
4 participants