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

Refactor opcodes #659

Closed
wants to merge 0 commits into from
Closed

Refactor opcodes #659

wants to merge 0 commits into from

Conversation

rumkin
Copy link
Contributor

@rumkin rumkin commented Feb 18, 2020

I've rewritten Opcodes creation logic what simplified Interpreter and made Opcode object itself easier to use and more informative.

  1. Added new Opcode class with fields code and fullName. Class field fullName created for opcodes like PUSH, JUMP, etc. which have similar names. Now this name is calculated only once. Classes are what V8 optimizer loves to use for faster function optimization. Also I protect it from accidental changes with Object.freeze call, so it could be passed anyway without creating of a duplicate. The interface now looks like this:
    class Opcode {
      readonly code: number
      readonly name: string
      readonly fullName: string
      readonly fee: number
      readonly isAsync: boolean
      // ...
    }
  2. Added createOpcodes factory into lib/evm/opcodes.ts. It simplified Interpreter#lookupOpInfo() and removed recalculation and recreation of opcode objects. Due to opcode is always same for same hardfork, so there is no reason to recreate it in runtime on each VM step.
  3. To preserve backward compatibility I reconstructed old Opcode object in the Interpreter#_runStepHook() method. This code should be removed in the next major release and replaced with a regular Opcode instance, it will reveal all the benefits of this PR.
    Now it is:
    {
      //...
      opcode: {
        name: opcode.fullName,
        fee: opcode.fee,
        isAsync: opcode.isAsync,
      },
      //...
    }
    And should become:
    {
      //...
      opcode,
      //...
    }

@rumkin rumkin changed the title Refactor opcodes [WIP] Refactor opcodes Feb 18, 2020
@codecov
Copy link

codecov bot commented Feb 18, 2020

Codecov Report

Merging #659 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #659      +/-   ##
==========================================
+ Coverage    91.4%   91.48%   +0.08%     
==========================================
  Files          31       31              
  Lines        1978     1997      +19     
  Branches      326      322       -4     
==========================================
+ Hits         1808     1827      +19     
  Misses         90       90              
  Partials       80       80
Flag Coverage Δ
#vm 91.48% <100%> (+0.08%) ⬆️
Impacted Files Coverage Δ
lib/evm/opcodes.ts 100% <100%> (ø) ⬆️
lib/evm/interpreter.ts 98.21% <100%> (-0.28%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1824cd0...2c67ce1. Read the comment docs.

@rumkin rumkin changed the title [WIP] Refactor opcodes Refactor opcodes Feb 18, 2020
@cgewecke cgewecke self-assigned this Feb 21, 2020
@rumkin rumkin closed this Feb 24, 2020
@rumkin rumkin mentioned this pull request Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants