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.

Sign up for GitHub

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

Jump to bottom

New metrics table #168

Merged
merged 6 commits into from
Sep 12, 2019
Merged

New metrics table #168

merged 6 commits into from
Sep 12, 2019

Conversation

chfast
Copy link
Member

@chfast chfast commented Sep 10, 2019 •
edited
Loading

Closes #161

This adds modified instruction metrics to op_table (previously containing only function pointers to instruction implementations). This should improve cache locality, but no performance improvement noticed.

Comparing bin/evmone-bench-master to bin/evmone-bench
Benchmark                                          Time             CPU      Time Old      Time New       CPU Old       CPU New
-------------------------------------------------------------------------------------------------------------------------------
blake2b_huff/analysis                           -0.0923         -0.0922            71            65            71            65
blake2b_shifts/analysis                         -0.0505         -0.0501            37            36            37            36
sha1_divs/analysis                              -0.1004         -0.1004             8             7             8             7
sha1_shifts/analysis                            -0.1041         -0.1037             7             7             7             7
weierstrudel/analysis                           -0.0497         -0.0492            86            82            86            82
micro/loop_with_many_jumpdests/analysis         +0.0308         +0.0311           268           276           268           276

@codecov-io
Copy link

codecov-io commented Sep 10, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@ae53cbb). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master     #168   +/-   ##
=========================================
  Coverage          ?   88.66%           
=========================================
  Files             ?       22           
  Lines             ?     2249           
  Branches          ?      217           
=========================================
  Hits              ?     1994           
  Misses            ?      228           
  Partials          ?       27

@gumb0
Copy link
Member

gumb0 commented Sep 11, 2019

This adds modified instruction metrics to op_table

How are they modified?

gumb0
gumb0 reviewed Sep 11, 2019
View reviewed changes
lib/evmone/analysis.cpp Outdated
const auto metrics = instr_table[opcode];
const auto instr_stack_req = metrics.num_stack_arguments;
const auto instr_stack_change = metrics.num_stack_returned_items - instr_stack_req;
const auto opcode_info = op_tbl[opcode];
Copy link
Member

Choose a reason for hiding this comment

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

Why not reference?

Copy link
Member Author

Choose a reason for hiding this comment

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

This adds modified instruction metrics to op_table

How are they modified?

It contains "stack height change" instead of "number of returned items".
This allows using the value directly in https://github.com/ethereum/evmone/pull/168/files#diff-79f19d4cf3a33e26add73cd9c0ad79cbR50 instead of computing 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.

Why not reference?

I'd prefer the values are loaded in single go, but when checking assembly - it does not matter: values are loaded individually anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, using reference is ~27% faster in one case.

Comparing bin/evmone-bench to bin/evmone-bench-ref
Benchmark                                          Time             CPU      Time Old      Time New       CPU Old       CPU New
-------------------------------------------------------------------------------------------------------------------------------
blake2b_huff/analysis                           -0.0002         -0.0002            40            40            40            40
blake2b_shifts/analysis                         -0.2770         -0.2770            26            19            26            19
sha1_divs/analysis                              -0.0052         -0.0052             4             4             4             4
sha1_shifts/analysis                            -0.0104         -0.0104             4             4             4             4
weierstrudel/analysis                           -0.0290         -0.0290            48            46            48            46
micro/loop_with_many_jumpdests/analysis         -0.0359         -0.0360           149           143           149           143

@chfast chfast requested a review from gumb0 September 11, 2019 19:28
@chfast chfast mentioned this pull request Sep 12, 2019
Open
@gumb0
Copy link
Member

gumb0 commented Sep 12, 2019

Would it make sense to create a unit test that checks op_table against metrics of EVMC ?

gumb0
gumb0 reviewed Sep 12, 2019
View reviewed changes
lib/evmone/instructions.cpp
table[OP_MSIZE] = op_msize;
table[OP_GAS] = op_gas;
table[OPX_BEGINBLOCK] = opx_beginblock; // Replaces JUMPDEST.
t = {op_undefined, 0, 0, 0};
Copy link
Member

Choose a reason for hiding this comment

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

This could be done in op_table_entry constructor

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 op_undefined is not available there. And I will need to declare 2 constructors.

@chfast
Copy link
Member Author

chfast commented Sep 12, 2019

Would it make sense to create a unit test that checks op_table against metrics of EVMC ?

Added.

@chfast chfast merged commit a3e55ec into master Sep 12, 2019
@chfast chfast deleted the op_metrics_table branch September 12, 2019 12:16
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

@halfalicious halfalicious Awaiting requested review from halfalicious

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Merge instruction metrics and function pointers tables
3 participants
@chfast @codecov-io @gumb0