Put instruction immediate values in the instruction table #154

chfast · 2019-09-09T10:08:51Z

Requires #144, #153.

axic · 2019-09-10T10:27:34Z

Needs a rebase?

codecov-io · 2019-09-10T10:28:23Z

Codecov Report

Merging #154 into master will increase coverage by 4%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #154   +/-   ##
=======================================
+ Coverage   85.31%   89.31%   +4%     
=======================================
  Files          22       22           
  Lines        2261     2256    -5     
  Branches      219      219           
=======================================
+ Hits         1929     2015   +86     
+ Misses        305      214   -91     
  Partials       27       27

gumb0 · 2019-09-11T16:55:10Z

Needs rebase

chfast · 2019-09-12T15:18:14Z

Rebased. No performance gains. And vector allocation is super messy and annoying - slight change to the preallocation has huge effect on analysis performance.

gumb0

Looks fine overall, just more documentation would be great, and I'm not sure if dump is correct.
Also why bother with these complications, if it doesn't give performance gains?

gumb0 · 2019-09-12T17:50:34Z

lib/evmone/analysis.cpp

@@ -37,16 +37,16 @@ code_analysis analyze(evmc_revision rev, const uint8_t* code, size_t code_size)

    code_analysis analysis;

-    const auto max_instrs_size = code_size + 1;
-    analysis.instrs.reserve(max_instrs_size);
+    analysis.instrs.reserve(2 * (code_size + 1));


comment why 2x ?

gumb0 · 2019-09-13T14:23:28Z

lib/evmone/analysis.hpp


 struct block_info
 {
    /// The total base gas cost of all instructions in the block.
    /// This cannot overflow, see the static_assert() below.
-    int32_t gas_cost = 0;
+    int32_t gas_cost;


why removing initializer?

gumb0 · 2019-09-13T15:31:18Z

lib/evmone/analysis.hpp

-union instr_argument
-{
-    int number;
-    const uint8_t* data;


was this data member never used?

gumb0 · 2019-09-13T15:34:40Z

lib/evmone/analysis.cpp

@@ -98,7 +96,8 @@ code_analysis analyze(evmc_revision rev, const uint8_t* code, size_t code_size)
            // TODO: Consier the same endianness-specific loop as in ANY_LARGE_PUSH case.
            while (code_pos < push_end && code_pos < code_end)
                *insert_pos++ = *code_pos++;
-            instr.arg.small_push_value = load64be(value_bytes);
+
+            analysis.instrs.emplace_back().small_push_value = load64be(value_bytes);


Maybe it would be more clear to emplace_back a placeholder right after emplacing fn on line 74, and here just assign, so that emplace_back calls are always in pairs. Also it would be less repetition.

gumb0 · 2019-09-13T15:36:37Z

lib/evmone/analysis.hpp

@@ -180,13 +170,15 @@ struct op_table_entry

 using op_table = std::array<op_table_entry, 256>;

-struct instr_info
+union instr_info


Maybe some comment for this union?

gumb0 · 2019-09-13T15:49:12Z

lib/evmone/instructions.cpp

@@ -546,7 +546,7 @@ const instr_info* op_jumpi(const instr_info* instr, execution_state& state) noex

 const instr_info* op_pc(const instr_info* instr, execution_state& state) noexcept
 {
-    state.stack.push(instr->arg.number);
+    state.stack.push((++instr)->number);


just a thought, maybe helpers like these could improve clarity

inline const instr_info& arg(const instr_info* instr) { return *(instr+1); } inline const instr_info* next_instr(const instr_info* instr) { return instr += 2; }

gumb0 · 2019-09-13T16:02:31Z

test/utils/dump.cpp

@@ -28,7 +28,7 @@ void dump(const evmone::code_analysis& analysis)

        if (c == OPX_BEGINBLOCK)
        {
-            block = &instr.arg.block;
+            block = &instr.block;


I didn't get how this works without changing the way you iterate over analysis.instrs
(instr points here to the union containing fn, not block_info, right?)

Also I'm not sure what c is here actually. Least significant byte of function pointer?

Put the immediate value of small pushes just after the instruction pointer in the program table.

Put the immediate value of the pointer to the large push data just after the instruction pointer in the program table.

Put the immediate value with block info just after the instruction pointer in the program table.

This makes the program table 2x smaller

chfast · 2019-09-17T09:09:19Z

Also why bother with these complications, if it doesn't give performance gains?

You have to do the work to be able to benchmark it later. This way is more memory efficient - we allocate space for instruction arguments (immediate values) only when needed. This may be important factor when we decide to cache the evmone loaded programs (not to repeat the analysis for the same contracts).

I still don't see big improvements, ~1-2% only. I believe my CPU is good enough in fetching memory quickly enough - old version has the same memory layout, just wastes some space.

Old:

 Performance counter stats for 'bin/evmone-bench-master ../../test/benchmarks':

        21 502 116      cache-references                                            
         1 119 073      cache-misses              #    5,204 % of all cache refs    
   127 269 289 930      cycles                                                      
   352 107 262 713      instructions              #    2,77  insn per cycle         

      28,971433332 seconds time elapsed

New:

 Performance counter stats for 'bin/evmone-bench ../../test/benchmarks':

        17 453 822      cache-references                                            
         1 046 808      cache-misses              #    5,998 % of all cache refs    
   127 385 133 793      cycles                                                      
   358 617 530 718      instructions              #    2,82  insn per cycle         

      28,987604886 seconds time elapsed

The above shows that the new version has similar number of cache misses, it just uses less memory in general.

chfast · 2019-09-17T09:11:25Z

I'm leaving this for next release.

Simplify signatures of Host methods

chfast requested review from gumb0, axic and halfalicious September 9, 2019 10:08

chfast mentioned this pull request Sep 10, 2019

Move instructions' additional data to instruction table #76

Closed

chfast force-pushed the instr_data2 branch from 6b71df5 to b993262 Compare September 10, 2019 10:28

chfast mentioned this pull request Sep 10, 2019

Instruction table (metrics) optimization #158

Closed

chfast force-pushed the instr_data2 branch 5 times, most recently from 842f8a5 to c66003a Compare September 12, 2019 14:22

gumb0 reviewed Sep 13, 2019

View reviewed changes

chfast added 7 commits September 17, 2019 10:38

Put small push data in the program table

99a8c27

Put the immediate value of small pushes just after the instruction pointer in the program table.

Put large push data pointer in the program table

c7f283e

Put the immediate value of the pointer to the large push data just after the instruction pointer in the program table.

Put block info in the program table

c955152

Put the immediate value with block info just after the instruction pointer in the program table.

Put immediate values of all other instructions in the program table

daac250

Change instr_info into union/variant type

d3e21b4

This makes the program table 2x smaller

Convert instr_info into POD type

276dabe

Adjust program size estimation

4403727

chfast force-pushed the instr_data2 branch from c66003a to 4403727 Compare September 17, 2019 09:01

jwasinger pushed a commit to jwasinger/evmone that referenced this pull request Apr 27, 2021

Merge pull request ethereum#154 from ethereum/simplify-host

adbe01c

Simplify signatures of Host methods

chfast force-pushed the master branch from 7717742 to d5e55fc Compare March 26, 2024 20:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put instruction immediate values in the instruction table #154

Put instruction immediate values in the instruction table #154

chfast commented Sep 9, 2019 •

edited

Loading

axic commented Sep 10, 2019

codecov-io commented Sep 10, 2019 •

edited

Loading

gumb0 commented Sep 11, 2019

chfast commented Sep 12, 2019

gumb0 left a comment

gumb0 Sep 12, 2019

gumb0 Sep 13, 2019

gumb0 Sep 13, 2019

gumb0 Sep 13, 2019

gumb0 Sep 13, 2019

gumb0 Sep 13, 2019

gumb0 Sep 13, 2019

gumb0 Sep 13, 2019

chfast commented Sep 17, 2019

chfast commented Sep 17, 2019

Put instruction immediate values in the instruction table #154

Are you sure you want to change the base?

Put instruction immediate values in the instruction table #154

Conversation

chfast commented Sep 9, 2019 • edited Loading

axic commented Sep 10, 2019

codecov-io commented Sep 10, 2019 • edited Loading

Codecov Report

gumb0 commented Sep 11, 2019

chfast commented Sep 12, 2019

gumb0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast commented Sep 17, 2019

chfast commented Sep 17, 2019

chfast commented Sep 9, 2019 •

edited

Loading

codecov-io commented Sep 10, 2019 •

edited

Loading