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

perf(ethcore): micro-opt#10405

Merged
debris merged 1 commit into
masterfrom
ethcore-micro-opt
Mar 6, 2019
Merged

perf(ethcore): micro-opt#10405
debris merged 1 commit into
masterfrom
ethcore-micro-opt

Conversation

@niklasad1
Copy link
Copy Markdown
Collaborator

@niklasad1 niklasad1 commented Feb 22, 2019 •
edited
Loading

Mostly fixes that changes eagerly eval to lazy eval shouldn't impact much

niklasad1
niklasad1 commented Feb 23, 2019
View reviewed changes
Comment thread ethcore/src/state/mod.rs
Failed(ExecutionError),
/// The transaction successfully completd with the given proof.
Complete(Executed),
Complete(Box<Executed>),
Copy link
Copy Markdown
Collaborator Author

@niklasad1 niklasad1 Feb 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Executed is quite big (368 bytes) and thus the enum will be size of biggest enum value.

For example if a function returns ProvedExecution the compiler must preserve stack to be at least 368 because it can't know which branch to take (technically the caller preserve must preserve this).

However, I'm not sure of the performance implications because I haven't benched it but I guess 5-40% (might even be worse when a lot calls are chained, i.e. passed on into to several functions by value)

Seems to only be used by light::on_demand::request and engine::validator_set::safe_contract

niklasad1
niklasad1 commented Feb 23, 2019
View reviewed changes
Comment thread ethcore/src/error.rs
TemporarilyInvalid(OutOfBounds<SystemTime>),
/// Log bloom header field is invalid.
InvalidLogBloom(Mismatch<Bloom>),
InvalidLogBloom(Box<Mismatch<Bloom>>),
Copy link
Copy Markdown
Collaborator Author

@niklasad1 niklasad1 Feb 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Bloom: 256 bytes

@niklasad1
perf(ethcore): micro-opt
7c0b645 
Mostly fixes that changes `eagerly eval` to `lazy eval`
@folsen
Copy link
Copy Markdown
Contributor

folsen commented Feb 25, 2019

@niklasad1 is it possible to add some microbenchmarks to this as well to see what the change is? I feel like we could use more benchmarks across the board

@niklasad1
Copy link
Copy Markdown
Collaborator Author

niklasad1 commented Feb 25, 2019

@folsen yeah I think so. I can try to add that but I think the only noticeable difference is the changed enum

@soc1c soc1c added this to the 2.5 milestone Feb 25, 2019
@soc1c soc1c added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Feb 25, 2019
ascjones
ascjones approved these changes Feb 26, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM

@ascjones ascjones added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 26, 2019
@debris debris merged commit 91933d8 into master Mar 6, 2019
@debris debris deleted the ethcore-micro-opt branch March 6, 2019 14:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@ascjones ascjones ascjones approved these changes

@debris debris debris approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.

Projects

None yet

Milestone

2.5

Development

Successfully merging this pull request may close these issues.

5 participants

@niklasad1 @folsen @ascjones @debris @soc1c