-
Notifications
You must be signed in to change notification settings - Fork 206
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
Conversation
I'm switching to draft until more tests pass. I'd like to squash a couple fixups... |
48d36f7
to
1866870
Compare
There was a problem hiding this 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.
packages/SwingSet/src/kernel/vatManager/manager-subprocess-xsnap.js
Outdated
Show resolved
Hide resolved
- export api constants so consumers can avoid node.js modules - export METER_TYPE
- avoid tail call optimization in stack overflow
1d8ff5f
to
76cf306
Compare
@warner I'm standing by for your review. Let me know if you'd rather I just went ahead based on @kriskowal 's. |
There was a problem hiding this 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.
@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 |
Yes; this is documented in the // 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 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! |
Oh! I forgot about that distinction between a merge and a rebase. Thanks for clarifying. |
fixes #2972
note #2980, #2979 are postponed.