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

Performance: no lazy_static or const fn #160

Open
sorpaas opened this issue Jun 2, 2017 · 4 comments
Open

Performance: no lazy_static or const fn #160

sorpaas opened this issue Jun 2, 2017 · 4 comments

Comments

@sorpaas
Copy link
Contributor

sorpaas commented Jun 2, 2017

Right now as we build on stable Rust, const fn is not available. Thus struct like Gas are constructed on the fly (the config file only store a u64). Performance can be improved if we utilize lazy_static or const fn, if the latter ever stabilize (rust-lang/rust#24111).

@sorpaas
Copy link
Contributor Author

sorpaas commented Jun 7, 2017

@sorpaas sorpaas closed this as completed Jun 7, 2017
@sorpaas sorpaas reopened this Jun 8, 2017
@sorpaas sorpaas changed the title Problem: performance impacted by no lazy_static or const fn Performance: no lazy_static or const fn Jun 8, 2017
@sorpaas sorpaas closed this as completed Jun 13, 2017
@sorpaas sorpaas reopened this Jun 13, 2017
@sorpaas
Copy link
Contributor Author

sorpaas commented Sep 24, 2017

lazy_static deps is now used in the core sputnikvm crate.

@zjhmale
Copy link

zjhmale commented Oct 12, 2017

@sorpaas So all these precompiled gas functions need to be utilized with lazy_static right?

image

@sorpaas
Copy link
Contributor Author

sorpaas commented Oct 12, 2017

Actually originally this was related to all const variables here (https://github.com/ethereumproject/sputnikvm/blob/master/src/eval/cost.rs#L9). Because Gas needs to be constructed on the fly and it's not free, it might be a performance issue. (See all the Into::into() below.)

Still, however, I just realized that Gas implements Copy trait, so even if when it is lazy-static-ed it might still not help a lot. A constraint on the Ethereum blockchain is that Gas, although its theoretical max value is U256::max_value(), it in practice cannot exceed usize::max_value(). A better way to handle this is probably to make Gas a type parameter in all VM structs. After that, track the initial provided gas, if that value exceeds usize::max_value(), use usize as the type parameter, otherwise, fallback to the full U256 (the conditional check does not need to be in the sputnikvm library).

The type parameter of gas G usually only needs Add + Mul + Sub + From<usize>. In the calculation of memory cost, however, there needs to be a checked multiply which does not belong to a trait, so a custom trait might need to be created for that.

Oops sorry I probably should be more careful when labeling an issue as "good first issue".

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

No branches or pull requests

3 participants