-
Notifications
You must be signed in to change notification settings - Fork 121
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
Tweak/wasm costing #1978
base: develop
Are you sure you want to change the base?
Tweak/wasm costing #1978
Conversation
Docker tags |
Benchmark for ed53e7bClick to view benchmark
|
|
||
wasm_execution_units / 3000 |
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.
Something here doesn't seem right -
1 / 0.00028877714 = 3462 which rounded down to 3000
1 / 0.0003709 = 2696 which would round down to 2500, not 4000
That said, using 2500 would make wasm more expensive. Maybe a few more of us should run this to provide a larger sample size?
// Add 10,000 base units to make sure the we do not undercharge for WASM execution, | ||
// which might lead to system exploitation. | ||
// This is especially important in corner-cases such as `costing::spin_loop_v2` benchmark. | ||
let n = n + 10_000; |
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 feels to me slightly better to put this in consume_wasm_execution_units
rather than the wasmi
adaptor.
Also this would need to be dependent on the VmVersion
... and ideally statically so we don't introduce a branch in every call in this critical code path. I'm not quite sure how we'd do this, but there are a few options.
Perhaps ScryptoRuntime
takes a new generic parameter - we create a new one each invocation, so we can just switch on vm_api.get_scrypto_version()
and either create a ScryptoRuntime::<FeeBehaviour1, _>::new
or a ScryptoRuntime::<FeeBehaviour2, _>::new
(@iamyulong - similar in idea to #1919 but at the VM layer - if VmApi had an associated type which impls VersionedVmLogic
then we wouldn't need a match here)
Summary
Details
Testing
Update Recommendations
For dApp Developers
For Internal Integrators