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

core/vm: use rsh instead of lookup #23472

Closed
wants to merge 1 commit into from

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Aug 26, 2021

This PR cleans up the code analysis a bit. There's this ugly lookup there, instead of a bitshift.

For some reason, whenever I try to remove that lookup, like this PR does, there's a performance loss. Results from go v1.17, with go test . -run - -bench JumpdestOp/\(PUSH1$\|STOP\) -count 10

name                        old time/op  new time/op  delta
JumpdestOpAnalysis/PUSH1-6  27.3ns ± 4%  31.0ns ± 0%  +13.41%  (p=0.000 n=10+8)
JumpdestOpAnalysis/STOP-6   22.0ns ± 8%  17.7ns ± 1%  -19.73%  (p=0.000 n=10+8)

So the PUSH1 degrades 13%, and the STOP, which should not be affected at all improves by ~20%.

😕

Do other folks get similar numbers?

@holiman
Copy link
Contributor Author

holiman commented Aug 27, 2021

Assigning this to @fjl who requested this change

@holiman
Copy link
Contributor Author

holiman commented Aug 30, 2021

Tested again on top of the benchmark changes from @chfast -- but see the same thing.

name                        old time/op    new time/op    delta
JumpdestOpAnalysis/PUSH1-6    1.01ms ± 3%    1.17ms ± 3%  +16.49%  (p=0.000 n=10+10)
JumpdestOpAnalysis/STOP-6      758µs ± 2%     597µs ± 4%  -21.31%  (p=0.000 n=9+10)

name                        old speed      new speed      delta
JumpdestOpAnalysis/PUSH1-6  1.22GB/s ± 3%  1.05GB/s ± 3%  -14.16%  (p=0.000 n=10+10)
JumpdestOpAnalysis/STOP-6   1.61GB/s ± 3%  2.06GB/s ± 4%  +27.59%  (p=0.000 n=10+10)

For some reason, this change makes the worst-case slower.

@chfast
Copy link
Member

chfast commented Aug 30, 2021

I can benchmark this after #23499 is merged.

@chfast
Copy link
Member

chfast commented Aug 30, 2021

Please rebase.

@chfast
Copy link
Member

chfast commented Aug 30, 2021

Note that codeSegment() does not use the lookup table.

@holiman
Copy link
Contributor Author

holiman commented Sep 7, 2021

Please rebase.

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants