Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
wip on metering host fns #100
wip on metering host fns #100
Changes from all commits
c3c5055
389898e
bf3b660
346abcd
19f57b9
0131101
b8184cd
17884c6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
How do these points get returned to the pool? It seems like not keeping part of the capacity requested to be claimed would mean that returning capacity on completion could get out of sync?
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.
It looks like that's not the idea. You allocate a certain amount of capacity and when it's gone it's gone until something calls
set
to top it back up?I still wonder if accurately tracking the value to account for overrun and making the 'set' an 'increment by' makes sense?
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.
@ThetaSinner well changing the middleware from decreasing points to increasing would mean rewriting the middleware from the ground up
we're using the default setup from wasmer, and what i'm doing here is exposing it so that it can be accessed by host functions as well as the default wasm op counter
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.
I don't mean dropping the increasing part, the increasing part is fine. I'm looking at
set_remaining_points
and wondering if that should be a paired up increment rather than a set?If this is just exposing something wasmer already does then fine
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.
@ThetaSinner it's a direct port of https://github.com/wasmerio/wasmer/blob/master/lib/middlewares/src/metering.rs#L335 but not requiring the instance itself to be in the environment, all we need is the global
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.
Understood thanks!
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Minor, optional:
assert_eq!
here?Then separate assertions on the next line so it's clear what failed if something goes wrong?