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

Query evmc_tx_context once in each execution #393

Merged
merged 1 commit into from
Sep 3, 2018
Merged

Query evmc_tx_context once in each execution #393

merged 1 commit into from
Sep 3, 2018

Conversation

axic
Copy link
Member

@axic axic commented Aug 30, 2018

Also independently discovered by @jakelang.

@@ -67,6 +67,9 @@ class EthereumInterface {
// set sane defaults
m_result.returnValue = std::vector<uint8_t>{};
m_result.isRevert = false;

// cache the transaction context here
m_context->fn_table->get_tx_context(&m_tx_context, m_context);
Copy link
Member

Choose a reason for hiding this comment

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

perhaps lazily populate this (upon first call by the EEI) and store it as a pair<bool, tx_context> that gets checked on each EEI call using it?

It would be a bit of a micro-optimization but contracts that do not need the tx_context don't need to call EVMC for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

But then again at every single EEI method querying that would have the extra branch checking that. I think it is better to assume some of these will be neeed and that simplifies code a lot.

@jakelang @chfast I would even consider changing EVMC to pass that down with execute.

Copy link
Member

Choose a reason for hiding this comment

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

@axic on the topic of obsessive micro-optimization, a branch comparing a u8 (bool) is only really a tradeoff towards code size rather than performance

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not considering to change the EVMC at the moment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Created issue: ethereum/evmc#120

@codecov-io
Copy link

Codecov Report

Merging #393 into master will decrease coverage by 0.18%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #393      +/-   ##
==========================================
- Coverage   67.66%   67.47%   -0.19%     
==========================================
  Files           8        8              
  Lines        1036     1030       -6     
  Branches      135      135              
==========================================
- Hits          701      695       -6     
  Misses        303      303              
  Partials       32       32

@axic axic merged commit d3c4384 into master Sep 3, 2018
@axic axic deleted the txcontext branch September 3, 2018 18:59
@axic axic removed the in progress label Sep 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants