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

Drop BytesIO from CodeStream, ~7.5% import block speedup #1777

Merged
merged 3 commits into from May 15, 2019

Conversation

Projects
None yet
2 participants
@carver
Copy link
Contributor

commented May 14, 2019

What was wrong?

While inlining CodeStream.__next __(), it started to make sense to drop BytesIO altogether. We were basically doing all the same work anyway, to avoid any slowdowns caused by dispatching to CodeStream.

How was it fixed?

  • Use slots to speed up attribute/method access
  • Drop io.BytesIO and just keep a pc and bytes buffer explicitly
  • Added SlowCodeStream as a reference implementation for testing, until this new one "burns in" for a while
  • (Bonus) try to skip the ljust in push_xx opcodes because it's typically not needed.

All added to a nice speed bump:

__iter__  4.53% -> 1.58% = 2.95%
read      2.31% -> 0.56% = 1.75%
pc()      1.12% -> 0%    = 1.12%
peek      1.07% -> 0.09% = 0.98%

push_xx   5.40% -> 4.57% = 0.83%
--------------------------------
Total lift                 7.6%

TODO:

  • see how bad it would be to drop the jump-dest min() (it was proactive, since pc is not being explicitly fenced anymore)
  • add notes about why performance decisions were made
  • squash
  • More explicit commit message about speedup

Cute Animal Picture

put a cute animal picture link inside the parentheses

@carver carver force-pushed the carver:code-stream-inline-next branch from a557fad to e81bae9 May 14, 2019

@carver carver changed the title Drop BytesIO from CodeStream Drop BytesIO from CodeStream, ~7% import block speedup May 14, 2019

self.pc = self._length_cache
else:
self.pc = target_pc
return self._raw_code_bytes[old_pc:target_pc]

This comment has been minimized.

Copy link
@pipermerriam

pipermerriam May 14, 2019

Member

Did you try using a memoryview here to avoid the copy?

This comment has been minimized.

Copy link
@carver

carver May 14, 2019

Author Contributor

Nice, will try

This comment has been minimized.

Copy link
@carver

carver May 14, 2019

Author Contributor

Wow, quite surprised that it doesn't speed things up. (I tried both converting it to a memoryview in the read() and converting it once in the __init__. It was slower in the first case, and no measurable effect in the second, except breaking consumers :))

I stumbled on a 0.85% speedup in push_XX though (since switching to a memoryview broke things in there). Pushing...

@carver carver changed the title Drop BytesIO from CodeStream, ~7% import block speedup Drop BytesIO from CodeStream, ~7.5% import block speedup May 14, 2019

@@ -11,9 +11,9 @@ def pop(computation: BaseComputation) -> None:

def push_XX(computation: BaseComputation, size: int) -> None:
raw_value = computation.code.read(size)

if not raw_value.strip(b'\x00'):

This comment has been minimized.

Copy link
@carver

carver May 14, 2019

Author Contributor

This strip was spending time to on a conversion that we might never need. Just let 0-bytes get pushed into the stack.

if not raw_value.strip(b'\x00'):
computation.stack_push(0)
raw_len = len(raw_value)
if raw_len == size:

This comment has been minimized.

Copy link
@carver

carver May 14, 2019

Author Contributor

Calling len() is shorter than calling an ljust() that takes no action (according to profiling), so we try to avoid the ljust(). (In fact, the import_block() calls in the four mainnet blocks that I'm testing never call ljust(), over the course of 154k push opcodes).

@carver carver force-pushed the carver:code-stream-inline-next branch from 81b71d4 to 7c359f4 May 14, 2019

@carver carver removed the PR state: WIP label May 14, 2019

@carver carver requested a review from pipermerriam May 14, 2019

@carver carver referenced this pull request May 14, 2019

Merged

EVM stack operation speedup, ~9% speedup #1773

6 of 6 tasks complete

carver added some commits May 14, 2019

CodeStream speedup ~6.8%
- Inline CodeStream.__next__() into CodeStream.peek
- Drop underlying BytesIO from code_stream
- Drop CodeStream.pc indirection
- Use __slots__

Saw these improvements in total import_block() time:

method    old      new     saved
__iter__  4.53% -> 1.58% = 2.95%
read      2.31% -> 0.56% = 1.75%
pc()      1.12% -> ~0%   = 1.12%
peek      1.07% -> 0.09% = 0.98%
--------------------------------
Total lift                 6.8%

Tested against mainnet blocks: (7620447, 7620450, 7620453, 7620454)
0.85% import_block speedup in push_xx opcodes
Usually, the data read from bytecode is the right length, so we can skip
the ljust if it's not. Also, got rid of an unnecessary strip.

@carver carver force-pushed the carver:code-stream-inline-next branch from 7c359f4 to b9837ef May 15, 2019

@carver

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

@pipermerriam okay, actually ready for review now

@pipermerriam
Copy link
Member

left a comment

  • Maybe SlowCodeStream belongs in eth.tools? That segregates it nicely from the core library code.

@carver carver merged commit 5c08053 into ethereum:master May 15, 2019

29 checks passed

ci/circleci: py35-core Your tests passed on CircleCI!
Details
ci/circleci: py35-database Your tests passed on CircleCI!
Details
ci/circleci: py35-lint Your tests passed on CircleCI!
Details
ci/circleci: py35-native-blockchain-byzantium Your tests passed on CircleCI!
Details
ci/circleci: py35-native-blockchain-constantinople Your tests passed on CircleCI!
Details
ci/circleci: py35-native-blockchain-frontier Your tests passed on CircleCI!
Details
ci/circleci: py35-native-blockchain-homestead Your tests passed on CircleCI!
Details
ci/circleci: py35-native-blockchain-petersburg Your tests passed on CircleCI!
Details
ci/circleci: py35-native-blockchain-spurious_dragon Your tests passed on CircleCI!
Details
ci/circleci: py35-native-blockchain-tangerine_whistle Your tests passed on CircleCI!
Details
ci/circleci: py35-native-blockchain-transition Your tests passed on CircleCI!
Details
ci/circleci: py35-transactions Your tests passed on CircleCI!
Details
ci/circleci: py35-vm Your tests passed on CircleCI!
Details
ci/circleci: py36-benchmark Your tests passed on CircleCI!
Details
ci/circleci: py36-core Your tests passed on CircleCI!
Details
ci/circleci: py36-database Your tests passed on CircleCI!
Details
ci/circleci: py36-docs Your tests passed on CircleCI!
Details
ci/circleci: py36-lint Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-byzantium Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-constantinople Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-frontier Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-homestead Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-petersburg Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-spurious_dragon Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-tangerine_whistle Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-transition Your tests passed on CircleCI!
Details
ci/circleci: py36-transactions Your tests passed on CircleCI!
Details
ci/circleci: py36-vm Your tests passed on CircleCI!
Details
ci/circleci: py37-core Your tests passed on CircleCI!
Details

@carver carver deleted the carver:code-stream-inline-next branch May 15, 2019

carver added a commit that referenced this pull request May 15, 2019

carver added a commit that referenced this pull request May 20, 2019

Release notes for v0.2.0-alpha.43 (#1742)
* Add release notes for PR #1716

* Add release notes for PR #1751

* Add release notes for PR #1747

* Release notes for #1735

* Add release notes for #1765 and #1766

* Add release notes for #1764

Also add notes on performance gains

* Add release notes for #1770 #1771 #1772

* Add release notes for #1776

* Add release notes for #1773

* Add release notes for #1777

* Add release notes for #1778
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.