Refactor opcodes #664

rumkin · 2020-02-24T12:13:24Z

Recreated PR #659

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

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
  // ...
}
```
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.
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,
  //...
}
```

codecov · 2020-02-24T12:22:18Z

Codecov Report

Merging #664 into master will increase coverage by 0.35%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #664      +/-   ##
==========================================
+ Coverage   91.00%   91.36%   +0.35%     
==========================================
  Files          47       44       -3     
  Lines        3046     2778     -268     
  Branches      501      433      -68     
==========================================
- Hits         2772     2538     -234     
+ Misses        157      140      -17     
+ Partials      117      100      -17

Flag	Coverage Δ
#account	`94.11% <ø> (ø)`
#block	`?`
#blockchain	`88.74% <ø> (-0.42%)`	⬇️
#common	`94.37% <ø> (+0.10%)`	⬆️
#tx	`94.11% <ø> (ø)`
#vm	`91.47% <100.00%> (+0.04%)`	⬆️

Impacted Files	Coverage Δ
packages/vm/lib/evm/interpreter.ts	`98.18% <100.00%> (-0.31%)`	⬇️
packages/vm/lib/evm/opcodes.ts	`100.00% <100.00%> (ø)`
packages/blockchain/src/cache.ts	`80.00% <0.00%> (-20.00%)`	⬇️
packages/vm/lib/evm/evm.ts	`91.85% <0.00%> (-0.12%)`	⬇️
packages/vm/lib/exceptions.ts	`100.00% <0.00%> (ø)`
packages/common/src/genesisStates/index.ts	`100.00% <0.00%> (ø)`
packages/block/src/header-from-rpc.ts
packages/block/src/block.ts
packages/block/src/from-rpc.ts
packages/block/src/header.ts
... and 2 more

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 e229b8e...ec3539d. Read the comment docs.

cgewecke · 2020-02-24T17:14:16Z

@rumkin Apologies for the delay reviewing here, will take a look at this today.

For background, in #660 you mention that:

I'm building a service for Ethereum developers and I need to allow people to experiment a lot with the Ethereum engine. Also it would be useful to researchers who want to use it in non-standard way.

Is this PR meant to address a specific issue with that project or are you mostly interested in performance optimization here?

rumkin · 2020-02-24T18:03:28Z

@cgewecke I'm not hurrying you up. I closed the original PR and created new one (to relocate branches) and added you to reviewers just to notify about this change.

Is this PR meant to address a specific issue with that project or are you mostly interested in performance optimization here?

This PR doesn't affect performance significantly, it's more like the first step to further optimizations.

cgewecke · 2020-02-24T18:14:52Z

@rumkin

This PR doesn't affect performance significantly, it's more like the first step to further optimizations

Are you able to elaborate a little on this?

rumkin · 2020-02-24T18:39:13Z

@cgewecke This PR is obviously reduce pressure on memory and allows V8 to optimize code. But, as I wrote earlier, to preserve backward compatibility with step event I recreated the opcode object in its previous view, what reduces effectiveness of the optimization a little. In the next major version it's possible to define new interface for step event and reveal all the potential from new Opcode class usage. Also there are another parts of code which could be optimized to receive more performance, but I don't want to rollup huge PR which will stop others work, especially in face of monorepo migration. And I need to know more about the current development process to be effective. So I'm going to make enhancements gradually.

alcuadrado · 2020-02-25T03:08:19Z

I like the idea of doing this kind of change. I think as long as they are kept this focused, they should be manageable.

This project is full of code that makes excessive allocations, even in hot paths. For example, we already saw huge performance improvements by caching some util.promisify results in #600 and #603.

Not all the PRs would bring dramatic changes by themselves. The challenge is to manage expectations when the change is not so big, or when performance comparisons are inconclusive. For instance, I tried multiple things like this and dropped them when building Buidler EVM, as they had almost no impact. Now I regret that.

I think many times doing these changes will still be worth their effort, just not individually. But in conjunction, they can dramatically change the performance of this project.

cgewecke · 2020-02-26T00:09:29Z

@rumkin @alcuadrado

Agree, and this PR LGTM to me as well. In principle it could speed things up a bit.

As an aside, the question of measuring performance has come up a few times in the last months here - I wonder how it might be done. Perhaps there's a way to record the VM inputs of a long Solidity test suite and replay them through a BuidlerEVM-like fixture.

Ethereumjs-vm doesn't really npm install from git because it's a transpiled product. It would be challenging to swap the changes of a specific branch here into (for example) a clone of Zeppelin's test suite as a benchmark.

lib/evm/opcodes.ts

holgerd77 · 2020-02-26T12:13:23Z

Side note: monorepo PR #666 merge is imminent, please don't do any other merges right now

rumkin · 2020-02-29T05:49:15Z

@cgewecke, about benchmarks and performance. Couldn't say much about how to apply this for this repo.

Ethereumjs-vm doesn't really npm install from git because it's a transpiled product. It would be challenging to swap the changes of a specific branch here into (for example) a clone of Zeppelin's test suite as a benchmark.

AFAIK Mocha supports plugins, and it is possible to rewrite modules paths. This is how code coverage works.

About @alcuadrado promise enhancement. Why not to rewrite state, trie and other parts to use async functions instead of callbacks? Is it a backward compatibility issue? It should enhance performance and simplify code itself. Also JS engines generates proper stack trace for async functions.

holgerd77 · 2020-03-03T16:13:37Z

@rumkin Trie rewrite to use async functions is in the works ethereumjs/merkle-patricia-tree#100, if you want to do a code review there that would be helpful

rumkin · 2020-03-03T20:39:21Z

@holgerd77 I will take a look into this.

cgewecke

This LGTM!

rumkin requested review from s1na, holgerd77 and cgewecke February 24, 2020 12:13

cgewecke reviewed Feb 26, 2020

View reviewed changes

lib/evm/opcodes.ts Outdated Show resolved Hide resolved

evertonfraga added the package: vm label Feb 27, 2020

rumkin force-pushed the f/opcodes branch from 2c67ce1 to c7a54c3 Compare February 28, 2020 07:19

rumkin added 4 commits March 4, 2020 23:35

Refactor opcodes

99328ca

Fix missing name variable

012d209

Fix missing key type

283ad34

Fix prettier linting issues

6d9c739

rumkin force-pushed the f/opcodes branch from c7a54c3 to 6d9c739 Compare March 4, 2020 20:57

holgerd77 mentioned this pull request Mar 31, 2020

VM v5 Release Planning #681

Closed

27 tasks

cgewecke self-assigned this Apr 15, 2020

cgewecke self-requested a review April 16, 2020 21:49

cgewecke approved these changes Apr 16, 2020

View reviewed changes

Merge branch 'master' into f/opcodes

ec3539d

holgerd77 merged commit 36fde16 into ethereumjs:master Apr 20, 2020

rumkin deleted the f/opcodes branch April 20, 2020 23:01

evertonfraga added this to the VM v5 milestone Jun 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor opcodes #664

Refactor opcodes #664

rumkin commented Feb 24, 2020 •

edited

Loading

codecov bot commented Feb 24, 2020 •

edited

Loading

cgewecke commented Feb 24, 2020

rumkin commented Feb 24, 2020

cgewecke commented Feb 24, 2020

rumkin commented Feb 24, 2020 •

edited

Loading

alcuadrado commented Feb 25, 2020 •

edited

Loading

cgewecke commented Feb 26, 2020

holgerd77 commented Feb 26, 2020

rumkin commented Feb 29, 2020

holgerd77 commented Mar 3, 2020

rumkin commented Mar 3, 2020

cgewecke left a comment

Refactor opcodes #664

Refactor opcodes #664

Conversation

rumkin commented Feb 24, 2020 • edited Loading

codecov bot commented Feb 24, 2020 • edited Loading

Codecov Report

cgewecke commented Feb 24, 2020

rumkin commented Feb 24, 2020

cgewecke commented Feb 24, 2020

rumkin commented Feb 24, 2020 • edited Loading

alcuadrado commented Feb 25, 2020 • edited Loading

cgewecke commented Feb 26, 2020

holgerd77 commented Feb 26, 2020

rumkin commented Feb 29, 2020

holgerd77 commented Mar 3, 2020

rumkin commented Mar 3, 2020

cgewecke left a comment

Choose a reason for hiding this comment

rumkin commented Feb 24, 2020 •

edited

Loading

codecov bot commented Feb 24, 2020 •

edited

Loading

rumkin commented Feb 24, 2020 •

edited

Loading

alcuadrado commented Feb 25, 2020 •

edited

Loading