Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Rework memory grow/check helpers - pass gas counter by value #598

Merged
merged 3 commits into from
Apr 5, 2023

Conversation

chfast
Copy link
Member

@chfast chfast commented Mar 27, 2023

This is preparation to remove gas from ExecutionState and passing gas by value everywhere.

@codecov
Copy link

codecov bot commented Mar 27, 2023

Codecov Report

Merging #598 (390fea4) into master (476abd7) will decrease coverage by 34.02%.
The diff coverage is 100.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##           master     #598       +/-   ##
===========================================
- Coverage   97.11%   63.09%   -34.02%     
===========================================
  Files          74       36       -38     
  Lines        7445     2092     -5353     
===========================================
- Hits         7230     1320     -5910     
- Misses        215      772      +557     
Flag Coverage Δ
blockchaintests ?
statetests 63.09% <100.00%> (+0.03%) ⬆️
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/evmone/instructions.hpp 88.63% <100.00%> (-11.37%) ⬇️
lib/evmone/instructions_calls.cpp 100.00% <100.00%> (ø)

... and 69 files with indirect coverage changes

@chfast chfast force-pushed the memory_grow branch 2 times, most recently from 7caf00e to d6d8c21 Compare March 27, 2023 19:19
@chfast chfast requested review from axic, gumb0, rodiazet and yperbasis March 27, 2023 22:32
// Check memory requirements for "copy" instructions.
inline bool check_memory(ExecutionState& state, const uint256& offset, const uint256& size) noexcept
/// Check memory requirements for "copy" instructions.
inline bool check_memory(
Copy link
Member

Choose a reason for hiding this comment

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

Not sure the asymmetry between grow_memory and check_memory of how gas_left is used is good. Should probably keep one option, pass-by-reference or return, for both.

Copy link
Member Author

Choose a reason for hiding this comment

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

The check_memory is inline so it does not matter. The grow_memory is noinline so calling convention will force int64_t& gas to be passed via memory.

Thecheck_memory can be changed too but it has much more users and the changed version wasn't looking very good. So this should be a separate piece of work. Not needed for performance though.

@chfast chfast merged commit e47172d into master Apr 5, 2023
@chfast chfast deleted the memory_grow branch April 5, 2023 08:51
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants