Skip to content

Analysis refactoring#153

Merged
chfast merged 1 commit intomasterfrom
analysis_refactoring
Sep 10, 2019
Merged

Analysis refactoring#153
chfast merged 1 commit intomasterfrom
analysis_refactoring

Conversation

@chfast
Copy link
Copy Markdown
Member

@chfast chfast commented Sep 9, 2019 •
edited
Loading

Requires #166.

Refactors analysis to have new block creation in single place. It also removes some unneeded variables and code.

TODO

  • Squash commits.

Bechmarks:

Comparing bin/evmone-bench-master to bin/evmone-bench
Benchmark                                          Time             CPU      Time Old      Time New       CPU Old       CPU New
-------------------------------------------------------------------------------------------------------------------------------
blake2b_huff/analysis                           -0.0866         -0.0866            37            34            37            34
blake2b_shifts/analysis                         -0.0941         -0.0940            20            18            20            18
sha1_divs/analysis                              -0.0930         -0.0929             4             4             4             4
sha1_shifts/analysis                            -0.0826         -0.0826             4             3             4             3
weierstrudel/analysis                           -0.0622         -0.0622            45            42            45            42
micro/loop_with_many_jumpdests/analysis         -0.0163         -0.0163           329           324           329           324

@chfast chfast requested review from axic and gumb0 September 9, 2019 08:25
@codecov-io
Copy link
Copy Markdown

codecov-io commented Sep 9, 2019 •
edited
Loading

Codecov Report

Merging #153 into master will increase coverage by 0.22%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #153      +/-   ##
==========================================
+ Coverage   84.93%   85.15%   +0.22%     
==========================================
  Files          21       21              
  Lines        2244     2237       -7     
  Branches      218      217       -1     
==========================================
- Hits         1906     1905       -1     
+ Misses        311      305       -6     
  Partials       27       27

gumb0
gumb0 reviewed Sep 9, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@gumb0 gumb0 left a comment

Choose a reason for hiding this comment

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

minor comments

lib/evmone/analysis.cpp Outdated
case OP_SELFDESTRUCT:
block = nullptr;
break;
if (create_new_block || (code_pos != code_end && *code_pos == OP_JUMPDEST))
Copy link
Copy Markdown
Member

@gumb0 gumb0 Sep 9, 2019

Choose a reason for hiding this comment

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

Maybe you could set create_new_block to true in if (opcode == OP_JUMPDEST) above, then this would be simpler

Copy link
Copy Markdown
Member Author

@chfast chfast Sep 9, 2019

Choose a reason for hiding this comment

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

That's right, but if create_new_block is already true we don't have to perform other checks.

Copy link
Copy Markdown
Member Author

@chfast chfast Sep 10, 2019

Choose a reason for hiding this comment

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

Renamed that to is_terminator. This is consistent with future changes in #158.

@chfast chfast force-pushed the analysis_refactoring branch 2 times, most recently from ffe2b85 to c23e3bb Compare September 10, 2019 07:38
@chfast chfast requested a review from gumb0 September 10, 2019 07:40
@chfast
Copy link
Copy Markdown
Member Author

chfast commented Sep 10, 2019

Added benchmark results.

@chfast chfast force-pushed the analysis_refactoring branch from c23e3bb to d9ee8b0 Compare September 10, 2019 09:26
This was referenced Sep 10, 2019
Merged
Open
Closed
@axic
Copy link
Copy Markdown
Member

axic commented Sep 10, 2019

Would it make sense merging all the test changes separately or they are different due to refactoring?

@chfast
Copy link
Copy Markdown
Member Author

chfast commented Sep 10, 2019

Would it make sense merging all the test changes separately or they are different due to refactoring?

Will do.

@chfast
Copy link
Copy Markdown
Member Author

chfast commented Sep 10, 2019

#166.

@chfast chfast force-pushed the analysis_refactoring branch 3 times, most recently from fdc3510 to 1853e06 Compare September 10, 2019 14:01
@chfast
Copy link
Copy Markdown
Member Author

chfast commented Sep 10, 2019

How does it look now?

gumb0
gumb0 reviewed Sep 10, 2019
View reviewed changes
lib/evmone/analysis.cpp Outdated
// this is a terminating instruction or the next instruction is a JUMPDEST.
block = &analysis.blocks.emplace_back();
block_stack_change = 0;
analysis.instrs.emplace_back(fns[OPX_BEGINBLOCK]).arg.number =
Copy link
Copy Markdown
Member

@gumb0 gumb0 Sep 10, 2019

Choose a reason for hiding this comment

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

here maybe split into two lines, too

@chfast chfast force-pushed the analysis_refactoring branch from 1853e06 to 3cc7fa1 Compare September 10, 2019 19:38
@chfast chfast merged commit 21e263f into master Sep 10, 2019
@chfast chfast deleted the analysis_refactoring branch September 10, 2019 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gumb0 gumb0 gumb0 approved these changes

@axic axic Awaiting requested review from axic

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@chfast @codecov-io @axic @gumb0