-
Notifications
You must be signed in to change notification settings - Fork 995
Change stack size growth strategy to 0.5 rate #8869
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
Change stack size growth strategy to 0.5 rate #8869
Conversation
86da553 to
da60750
Compare
| } | ||
|
|
||
| private static int newLength(final int oldCapacity, final int minGrowth, final int prefGrowth) { | ||
| return oldCapacity + Math.max(minGrowth, prefGrowth); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a danger of int overflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @macfarla!
There was also another existing problem when someone sets maxSize == Integer.MAX_VALUE which is not a realistic value since the JVM is not able to allocate that many entries. You get something like this:
Exception in thread "main" java.lang.OutOfMemoryError: Requested array size exceeds VM limit
at java.base/java.lang.reflect.Array.newArray(Native Method)
at java.base/java.lang.reflect.Array.newInstance(Array.java:78)
at FlexStack.expandEntries(FlexStack.java:124)
at FlexStack.push(FlexStack.java:143)
at Test.main(Test.java:6)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed now. Couldn't write tests because we would have to increase the heap size to 16G for such a case so I checked it separately offline.
| top = nextTop; | ||
| } | ||
|
|
||
| private int newLength(final int oldCapacity, final int minGrowth, final int prefGrowth) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| private int newLength(final int oldCapacity, final int minGrowth, final int prefGrowth) { | |
| private int newLength(final int currentCapacity, final int minGrowth, final int prefGrowth) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to make consistent with other changes
evm/src/main/java/org/hyperledger/besu/evm/internal/FlexStack.java
Outdated
Show resolved
Hide resolved
evm/src/main/java/org/hyperledger/besu/evm/internal/FlexStack.java
Outdated
Show resolved
Hide resolved
4ffbfe5 to
8104be6
Compare
|
After speaking with @ahamlat today, it's best to assess performance on mainnet first and do some benchmarking for CALL* before pulling this in |
evm/src/main/java/org/hyperledger/besu/evm/internal/FlexStack.java
Outdated
Show resolved
Hide resolved
396ed88 to
92ed95e
Compare
|
moving to draft since we need to do performance testing on this before merging |
fbd2f92 to
3ecd27a
Compare
Signed-off-by: Luis Pinto <luis.pinto@consensys.net>
Signed-off-by: Luis Pinto <luis.pinto@consensys.net>
Signed-off-by: Luis Pinto <luis.pinto@consensys.net>
Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: Luis Pinto <luis.pinto@consensys.net>
Signed-off-by: Luis Pinto <luis.pinto@consensys.net>
Signed-off-by: Luis Pinto <luis.pinto@consensys.net>
3ecd27a to
ac0e781
Compare
siladu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Regression tested on mainnet and no regressions found.
Also no obvious difference in performance detected.
|
@lu-pinto maybe warrants a changelog entry |
Signed-off-by: Luis Pinto <luis.pinto@consensys.net>
This PR changes the stack rate growth strategy of FlexStack to grow at a 50% rate instead of constantly growing by 32 slots as is currently. The former is the approach adopted by the JDK for ArrayList. With a 50% allocation rate and considering a max stack size of 1024, setting the initial size to 91 one can reach the worse case (1024) in just 6 resizes compared to 32 required with the current approach. Also the current usage on mainnet has shown that CALLs never go above 150 deep during execution but this can change in the future. Without this PR: Benchmark (stackDepth) Mode Cnt Score Error Units OperandStackBenchmark.fillUp 6 avgt 15 12.804 ± 0.021 ns/op OperandStackBenchmark.fillUp 15 avgt 15 26.348 ± 0.028 ns/op OperandStackBenchmark.fillUp 34 avgt 15 145.041 ± 7.118 ns/op OperandStackBenchmark.fillUp 100 avgt 15 335.065 ± 0.573 ns/op OperandStackBenchmark.fillUp 234 avgt 15 843.487 ± 16.104 ns/op OperandStackBenchmark.fillUp 500 avgt 15 2102.937 ± 9.191 ns/op OperandStackBenchmark.fillUp 800 avgt 15 4502.884 ± 17.090 ns/op OperandStackBenchmark.fillUp 1024 avgt 15 6943.397 ± 18.922 ns/op With this PR: Benchmark (stackDepth) Mode Cnt Score Error Units OperandStackBenchmark.fillUp 6 avgt 15 22.623 ± 0.913 ns/op OperandStackBenchmark.fillUp 15 avgt 15 30.296 ± 0.149 ns/op OperandStackBenchmark.fillUp 34 avgt 15 58.666 ± 0.090 ns/op OperandStackBenchmark.fillUp 100 avgt 15 277.010 ± 0.520 ns/op OperandStackBenchmark.fillUp 234 avgt 15 700.040 ± 1.873 ns/op OperandStackBenchmark.fillUp 500 avgt 15 1610.791 ± 1.557 ns/op OperandStackBenchmark.fillUp 800 avgt 15 2632.757 ± 33.295 ns/op OperandStackBenchmark.fillUp 1024 avgt 15 3150.211 ± 13.124 ns/op
This PR changes the stack rate growth strategy of FlexStack to grow at a 50% rate instead of constantly growing by 32 slots as is currently. The former is the approach adopted by the JDK for ArrayList. With a 50% allocation rate and considering a max stack size of 1024, setting the initial size to 91 one can reach the worse case (1024) in just 6 resizes compared to 32 required with the current approach. Also the current usage on mainnet has shown that CALLs never go above 150 deep during execution but this can change in the future. Without this PR: Benchmark (stackDepth) Mode Cnt Score Error Units OperandStackBenchmark.fillUp 6 avgt 15 12.804 ± 0.021 ns/op OperandStackBenchmark.fillUp 15 avgt 15 26.348 ± 0.028 ns/op OperandStackBenchmark.fillUp 34 avgt 15 145.041 ± 7.118 ns/op OperandStackBenchmark.fillUp 100 avgt 15 335.065 ± 0.573 ns/op OperandStackBenchmark.fillUp 234 avgt 15 843.487 ± 16.104 ns/op OperandStackBenchmark.fillUp 500 avgt 15 2102.937 ± 9.191 ns/op OperandStackBenchmark.fillUp 800 avgt 15 4502.884 ± 17.090 ns/op OperandStackBenchmark.fillUp 1024 avgt 15 6943.397 ± 18.922 ns/op With this PR: Benchmark (stackDepth) Mode Cnt Score Error Units OperandStackBenchmark.fillUp 6 avgt 15 22.623 ± 0.913 ns/op OperandStackBenchmark.fillUp 15 avgt 15 30.296 ± 0.149 ns/op OperandStackBenchmark.fillUp 34 avgt 15 58.666 ± 0.090 ns/op OperandStackBenchmark.fillUp 100 avgt 15 277.010 ± 0.520 ns/op OperandStackBenchmark.fillUp 234 avgt 15 700.040 ± 1.873 ns/op OperandStackBenchmark.fillUp 500 avgt 15 1610.791 ± 1.557 ns/op OperandStackBenchmark.fillUp 800 avgt 15 2632.757 ± 33.295 ns/op OperandStackBenchmark.fillUp 1024 avgt 15 3150.211 ± 13.124 ns/op
This PR changes the stack rate growth strategy of FlexStack to grow at a 50% rate instead of constantly growing by 32 slots as is currently. The former is the approach adopted by the JDK for ArrayList. With a 50% allocation rate and considering a max stack size of 1024, setting the initial size to 91 one can reach the worse case (1024) in just 6 resizes compared to 32 required with the current approach. Also the current usage on mainnet has shown that CALLs never go above 150 deep during execution but this can change in the future. Without this PR: Benchmark (stackDepth) Mode Cnt Score Error Units OperandStackBenchmark.fillUp 6 avgt 15 12.804 ± 0.021 ns/op OperandStackBenchmark.fillUp 15 avgt 15 26.348 ± 0.028 ns/op OperandStackBenchmark.fillUp 34 avgt 15 145.041 ± 7.118 ns/op OperandStackBenchmark.fillUp 100 avgt 15 335.065 ± 0.573 ns/op OperandStackBenchmark.fillUp 234 avgt 15 843.487 ± 16.104 ns/op OperandStackBenchmark.fillUp 500 avgt 15 2102.937 ± 9.191 ns/op OperandStackBenchmark.fillUp 800 avgt 15 4502.884 ± 17.090 ns/op OperandStackBenchmark.fillUp 1024 avgt 15 6943.397 ± 18.922 ns/op With this PR: Benchmark (stackDepth) Mode Cnt Score Error Units OperandStackBenchmark.fillUp 6 avgt 15 22.623 ± 0.913 ns/op OperandStackBenchmark.fillUp 15 avgt 15 30.296 ± 0.149 ns/op OperandStackBenchmark.fillUp 34 avgt 15 58.666 ± 0.090 ns/op OperandStackBenchmark.fillUp 100 avgt 15 277.010 ± 0.520 ns/op OperandStackBenchmark.fillUp 234 avgt 15 700.040 ± 1.873 ns/op OperandStackBenchmark.fillUp 500 avgt 15 1610.791 ± 1.557 ns/op OperandStackBenchmark.fillUp 800 avgt 15 2632.757 ± 33.295 ns/op OperandStackBenchmark.fillUp 1024 avgt 15 3150.211 ± 13.124 ns/op Signed-off-by: jflo <justin+github@florentine.us>

This is a cherry-pick from the performance branch that we used to test the changes in the latest interop.
original PR: #8814
Fixed Issue(s)
Thanks for sending a pull request! Have you done the following?
doc-change-requiredlabel to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew spotlessApply./gradlew build./gradlew acceptanceTest./gradlew integrationTest./gradlew ethereum:referenceTests:referenceTests