Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

WASM gas schedule #6638

Merged
merged 11 commits into from Oct 9, 2017
Merged

WASM gas schedule #6638

merged 11 commits into from Oct 9, 2017

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Oct 4, 2017 •
edited

this actually closes the WASM MVP #6056, since proper gas metering is all that left there

WASM itself can be parameterized by much broader range of opcode types, but seems like they all have roughly the same cpu costs

@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Oct 4, 2017
@NikVolf NikVolf changed the title Wasm gas schedule WASM gas schedule Oct 4, 2017
@NikVolf NikVolf added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 4, 2017
@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Oct 4, 2017
arkpar
arkpar reviewed Oct 5, 2017
View reviewed changes
ethcore/vm/src/schedule.rs Outdated
/// Wasm cost table
pub struct WasmCosts {
/// Arena allocator cost, per byte
pub alloc: usize,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why usize? usize should only be used for representing memory area sizes. Hence the name. u64 should prevent any unexpected issues on 32-bit platforms.

Copy link
Contributor Author

@NikVolf NikVolf Oct 5, 2017 •
edited

Choose a reason for hiding this comment

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

And why all ethereum Schedule members are usize?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No idea, but I think it needs to be fixed.

Copy link
Contributor Author

@NikVolf NikVolf Oct 5, 2017 •
edited

Choose a reason for hiding this comment

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

https://github.com/paritytech/parity/blob/c7ea25227a01aa9d22964f84489d7f16a224ce5c/ethcore/evm/src/evm.rs#L59

oh this is because of that, gas counter for evm uses different size across platforms

i don't use it, so i might stick to u64

arkpar
arkpar reviewed Oct 5, 2017
View reviewed changes
ethcore/wasm/src/runtime.rs Outdated
@@ -102,6 +103,13 @@ pub struct RuntimeContext {
pub value: U256,
}

macro_rules! charge {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not a simple function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because of double borrowing of self

Copy link
Collaborator

Choose a reason for hiding this comment

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

ugh, ok

Copy link
Contributor

Choose a reason for hiding this comment

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

seems like a fn charge<F>(&mut self, f: F) where F: FnOnce(&Schedule) -> u64 would work in all cases here

@arkpar arkpar added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 5, 2017
@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Oct 5, 2017
@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 5, 2017
rphmeier
rphmeier reviewed Oct 5, 2017
View reviewed changes
ethcore/wasm/src/runtime.rs Outdated
@@ -369,13 +412,17 @@ impl<'a, 'b> Runtime<'a, 'b> {
-> Result<Option<interpreter::RuntimeValue>, InterpreterError>
{
let amount = context.value_stack.pop_as::<i32>()? as u32;

charge!(self, self.ext.schedule().wasm.alloc * amount);
Copy link
Contributor

Choose a reason for hiding this comment

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

depending on the alloc cost, this seems like it could easily overflow a u32 when allocating just a few MB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's super easy operation with arena allocator, so it should not be priced more than memory opcodes
and memory limit for wasm is just 16mb

i will change to u64 anyway though to be uniform

rphmeier
rphmeier reviewed Oct 5, 2017
View reviewed changes
ethcore/wasm/src/runtime.rs Outdated
@@ -211,6 +249,9 @@ impl<'a, 'b> Runtime<'a, 'b> {

let code = self.memory.get(code_ptr, code_len as usize)?;

charge!(self, self.ext.schedule().create_gas as u32);
charge!(self, (self.ext.schedule().create_data_gas * code.len()) as u32);
Copy link
Contributor

Choose a reason for hiding this comment

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

again, using u32 for gas here seems a little flaky. In the EVM code we use u64 in general.

@rphmeier rphmeier added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Oct 5, 2017
@NikVolf NikVolf added the A0-pleasereview 🤓 Pull request needs code review. label Oct 5, 2017
@NikVolf NikVolf removed the A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. label Oct 5, 2017
@NikVolf
Copy link
Contributor Author

NikVolf commented Oct 5, 2017

@rphmeier @arkpar
updated!

@NikVolf NikVolf mentioned this pull request Oct 7, 2017
Closed
5 tasks
debris
debris approved these changes Oct 9, 2017
View reviewed changes
Copy link
Collaborator

@debris debris 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 not fully familiar with the context of this pr, but it looks good to me. Just one minor (irrelevant) grumble.

ethcore/wasm/src/lib.rs
);

let data_section_length = contract_module.data_section()
.map(|section| section.entries().iter().fold(0, |sum, entry| sum + entry.value().len()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

using

map(|entry| entry.value().len()).sum()

would be more idiomatic

@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 9, 2017
@gavofyork gavofyork merged commit 1601030 into master Oct 9, 2017
@gavofyork gavofyork deleted the wasm-schedule branch October 9, 2017 11:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@arkpar arkpar arkpar left review comments

@rphmeier rphmeier rphmeier left review comments

@debris debris debris approved these changes

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@NikVolf @debris @arkpar @rphmeier @gavofyork