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

Remove pass-by-reference return data value from executive #9211

Merged
merged 10 commits into from
Aug 13, 2018

Conversation

sorpaas
Copy link
Collaborator

@sorpaas sorpaas commented Jul 24, 2018

rel #6744

Return data is always available from FinalizationResult, so using pass-by-reference in Executive parameters doesn't give us any performance benefits. This reference also creates some trouble for resumable Executive.

This PR changes it so that output is read from FinalizationResult::return_data. An unsafe in evm crate was also able to be removed due to this change.

@sorpaas sorpaas added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jul 24, 2018
@sorpaas sorpaas added this to the 2.1 milestone Jul 24, 2018
@sorpaas sorpaas mentioned this pull request Jul 24, 2018
@sorpaas sorpaas added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 24, 2018
@sorpaas
Copy link
Collaborator Author

sorpaas commented Jul 24, 2018

Previously we trace call output via cmp::min(parent_output.len(), actual_output.len()), but because we don't know anything about parent_output in executive right now, this needs to be changed to just actual_output.len(). One test is affected. Don't know whether there're better ways to handle this.

@sorpaas sorpaas added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jul 24, 2018
return match call_result {
MessageCallResult::Success(gas_left, data) => {
let len = cmp::min(output.len(), data.len());
(&mut output[..len]).copy_from_slice(&data[..len]);

This comment was marked as resolved.

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.

it took my some time to go through the code, but it lgtm!

@@ -431,7 +438,6 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
Err(evm_err)
} else {
self.state.discard_checkpoint();
output.write(0, &builtin_out_buffer);
Copy link
Collaborator

Choose a reason for hiding this comment

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

-1 copy 👍

@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 24, 2018
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

LGTM, although it's the first time I'm looking at this code. I don't have any suggestion regarding the change in trace output.

@sorpaas sorpaas merged commit ff716e7 into master Aug 13, 2018
@sorpaas sorpaas deleted the sp-return-data branch August 13, 2018 21:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants