-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Improve shift opcodes performance (SAR, SHR and SHL) #9796
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
Changes from all commits
ab9e3cd
c6e21bd
9691670
9b20601
209e64b
9ada695
d4363f4
c721e15
b3cd533
aee6907
f030d93
cf20a2c
705b033
bf918a8
0ca7aa0
06587d2
3cabb0d
e4b8bbe
78126d8
ef66ae7
ca544de
268036a
f95e549
a99c10f
93c3623
83c7a17
8c1d5c1
a7514b1
a2d7ffd
31d0669
63eb9f2
86a85e6
8dd0384
8b30c59
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -656,6 +656,7 @@ subprojects { | |
| excludes = _strListCmdArg('excludes', []) | ||
| var asyncProfiler = _strCmdArg('asyncProfiler') | ||
| var asyncProfilerOptions = _strCmdArg('asyncProfilerOptions', 'output=flamegraph') | ||
| zip64.set(true) | ||
| jvmArgs = [ | ||
| '-XX:+EnableDynamicAgentLoading' | ||
| ] | ||
|
|
@@ -666,6 +667,10 @@ subprojects { | |
| // Force debug symbols of stack traces to the profiler | ||
| jvmArgs.addAll("-XX:+UnlockDiagnosticVMOptions", "-XX:+DebugNonSafepoints") | ||
| } | ||
| var gcProfiler = _strCmdArg('gcProfiler') | ||
| if (gcProfiler != null && gcProfiler.toBoolean()) { | ||
| profilers.add('gc') | ||
| } | ||
|
Comment on lines
+670
to
+673
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice addition 👍 |
||
| duplicateClassesStrategy = DuplicatesStrategy.INCLUDE | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,161 @@ | ||
| /* | ||
| * Copyright contributors to Besu. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on | ||
| * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations under the License. | ||
| * | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
| package org.hyperledger.besu.ethereum.vm.operations; | ||
|
|
||
| import static org.hyperledger.besu.ethereum.vm.operations.BenchmarkHelper.randomNegativeValue; | ||
| import static org.hyperledger.besu.ethereum.vm.operations.BenchmarkHelper.randomPositiveValue; | ||
| import static org.hyperledger.besu.ethereum.vm.operations.BenchmarkHelper.randomValue; | ||
|
|
||
| import java.util.concurrent.ThreadLocalRandom; | ||
|
|
||
| import org.apache.tuweni.bytes.Bytes; | ||
| import org.openjdk.jmh.annotations.Level; | ||
| import org.openjdk.jmh.annotations.Param; | ||
| import org.openjdk.jmh.annotations.Setup; | ||
|
|
||
| /** | ||
| * Abstract base class for SAR (Shift Arithmetic Right) operation benchmarks. | ||
| * | ||
| * <p>SAR has additional test cases for negative/positive values to test sign extension behavior. | ||
| */ | ||
| public abstract class AbstractSarOperationBenchmark extends BinaryOperationBenchmark { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to shift by a negative amount? If not, what code is guarding that?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not possible as it is spec'ed. The first input (shift) of SAR is unsigned. |
||
|
|
||
| /** Test cases covering different execution paths for SAR operations. */ | ||
| public enum Case { | ||
| /** Shift by 0 - early return path. */ | ||
| SHIFT_0, | ||
| /** Negative number (ALL_BITS) with shift=1 - tests sign extension OR path. */ | ||
| NEGATIVE_SHIFT_1, | ||
| /** value with all bits to 1 with shift=1 * */ | ||
| ALL_BITS_SHIFT_1, | ||
| /** Positive number with shift=1 - no sign extension needed. */ | ||
| POSITIVE_SHIFT_1, | ||
| /** Negative number with medium shift. */ | ||
| NEGATIVE_SHIFT_128, | ||
| /** Negative number with max shift. */ | ||
| NEGATIVE_SHIFT_255, | ||
| /** Positive number with medium shift. */ | ||
| POSITIVE_SHIFT_128, | ||
| /** positive number with max shift. */ | ||
| POSITIVE_SHIFT_255, | ||
| /** Overflow: shift >= 256. */ | ||
| OVERFLOW_SHIFT_256, | ||
| /** Overflow: shift amount > 4 bytes. */ | ||
| OVERFLOW_LARGE_SHIFT, | ||
| /** Random values (original behavior). */ | ||
| FULL_RANDOM | ||
| } | ||
|
|
||
| /** All bits set (32 bytes of 0xFF) - represents -1 in two's complement. */ | ||
| protected static final Bytes ALL_BITS = | ||
| Bytes.fromHexString("0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"); | ||
|
|
||
| @Param({ | ||
| "SHIFT_0", | ||
| "NEGATIVE_SHIFT_1", | ||
| "POSITIVE_SHIFT_1", | ||
| "ALL_BITS_SHIFT_1", | ||
| "NEGATIVE_SHIFT_128", | ||
| "NEGATIVE_SHIFT_255", | ||
| "POSITIVE_SHIFT_128", | ||
| "POSITIVE_SHIFT_255", | ||
| "OVERFLOW_SHIFT_256", | ||
| "OVERFLOW_LARGE_SHIFT", | ||
| "FULL_RANDOM" | ||
| }) | ||
| protected String caseName; | ||
|
|
||
| @Setup(Level.Iteration) | ||
| @Override | ||
| public void setUp() { | ||
| frame = BenchmarkHelper.createMessageCallFrame(); | ||
|
|
||
| final Case scenario = Case.valueOf(caseName); | ||
| aPool = new Bytes[SAMPLE_SIZE]; // shift amount (pushed second, popped first) | ||
| bPool = new Bytes[SAMPLE_SIZE]; // value (pushed first, popped second) | ||
|
|
||
| final ThreadLocalRandom random = ThreadLocalRandom.current(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we keep using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is more thread safe, it doesn't help a lot in this case but it is in general a best practice. |
||
|
|
||
| for (int i = 0; i < SAMPLE_SIZE; i++) { | ||
| switch (scenario) { | ||
| case SHIFT_0: | ||
| aPool[i] = Bytes.of(0); | ||
| bPool[i] = randomValue(random); | ||
| break; | ||
|
|
||
| case NEGATIVE_SHIFT_1: | ||
| // shiftAmount = 0x1, value = 0xfff...fff (negative, tests OR path) | ||
| aPool[i] = Bytes.of(1); | ||
| bPool[i] = randomNegativeValue(random); | ||
| break; | ||
|
|
||
| case ALL_BITS_SHIFT_1: | ||
| // shiftAmount = 0x1, value = 0xfff...fff (negative, tests OR path) | ||
| aPool[i] = Bytes.of(1); | ||
| bPool[i] = ALL_BITS; | ||
| break; | ||
|
|
||
| case POSITIVE_SHIFT_1: | ||
| // shiftAmount = 0x1, random positive value (no sign extension) | ||
| aPool[i] = Bytes.of(1); | ||
| bPool[i] = randomPositiveValue(random); | ||
| break; | ||
|
|
||
| case NEGATIVE_SHIFT_128: | ||
| aPool[i] = Bytes.of(128); | ||
| bPool[i] = randomNegativeValue(random); | ||
| break; | ||
|
|
||
| case NEGATIVE_SHIFT_255: | ||
| aPool[i] = Bytes.of(255); | ||
| bPool[i] = randomNegativeValue(random); | ||
| break; | ||
|
|
||
| case POSITIVE_SHIFT_128: | ||
| aPool[i] = Bytes.of(128); | ||
| bPool[i] = randomPositiveValue(random); | ||
| break; | ||
| case POSITIVE_SHIFT_255: | ||
| aPool[i] = Bytes.of(255); | ||
| bPool[i] = randomPositiveValue(random); | ||
| break; | ||
|
|
||
| case OVERFLOW_SHIFT_256: | ||
| // Shift of exactly 256 - overflow path | ||
| aPool[i] = Bytes.fromHexString("0x0100"); // 256 | ||
| bPool[i] = randomValue(random); | ||
| break; | ||
|
|
||
| case OVERFLOW_LARGE_SHIFT: | ||
| // Shift amount > 4 bytes - overflow path | ||
| aPool[i] = Bytes.fromHexString("0x010000000000"); // > 4 bytes | ||
| bPool[i] = randomValue(random); | ||
| break; | ||
|
|
||
| case FULL_RANDOM: | ||
| default: | ||
| // Original random behavior | ||
| final byte[] shift = new byte[1 + random.nextInt(2)]; | ||
| final byte[] value = new byte[1 + random.nextInt(32)]; | ||
| random.nextBytes(shift); | ||
| random.nextBytes(value); | ||
| aPool[i] = Bytes.wrap(shift); | ||
| bPool[i] = Bytes.wrap(value); | ||
| break; | ||
| } | ||
| } | ||
| index = 0; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| /* | ||
| * Copyright contributors to Besu. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on | ||
| * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations under the License. | ||
| * | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
| package org.hyperledger.besu.ethereum.vm.operations; | ||
|
|
||
| import static org.hyperledger.besu.ethereum.vm.operations.BenchmarkHelper.randomValue; | ||
|
|
||
| import java.util.concurrent.ThreadLocalRandom; | ||
|
|
||
| import org.apache.tuweni.bytes.Bytes; | ||
| import org.openjdk.jmh.annotations.Level; | ||
| import org.openjdk.jmh.annotations.Param; | ||
| import org.openjdk.jmh.annotations.Setup; | ||
|
|
||
| /** | ||
| * Abstract base class for shift operation benchmarks (SHL, SHR, SAR). | ||
| * | ||
| * <p>Provides shared test case definitions and setup logic. | ||
| */ | ||
| public abstract class AbstractShiftOperationBenchmark extends BinaryOperationBenchmark { | ||
|
|
||
| /** Test cases covering different execution paths for shift operations. */ | ||
| public enum Case { | ||
| /** Shift by 0 - no shift needed. */ | ||
| SHIFT_0, | ||
| /** Small shift by 1 bit. */ | ||
| SHIFT_1, | ||
| /** Medium shift by 128 bits (half word). */ | ||
| SHIFT_128, | ||
| /** Large shift by 255 bits (max valid). */ | ||
| SHIFT_255, | ||
| /** Overflow: shift of exactly 256. */ | ||
| OVERFLOW_SHIFT_256, | ||
| /** Overflow: shift amount > 4 bytes. */ | ||
| OVERFLOW_LARGE_SHIFT, | ||
| /** Random values (original behavior). */ | ||
| FULL_RANDOM | ||
| } | ||
|
|
||
| @Param({ | ||
| "SHIFT_0", | ||
| "SHIFT_1", | ||
| "SHIFT_128", | ||
| "SHIFT_255", | ||
| "OVERFLOW_SHIFT_256", | ||
| "OVERFLOW_LARGE_SHIFT", | ||
| "FULL_RANDOM" | ||
| }) | ||
| protected String caseName; | ||
|
|
||
| @Setup(Level.Iteration) | ||
| @Override | ||
| public void setUp() { | ||
| frame = BenchmarkHelper.createMessageCallFrame(); | ||
|
|
||
| final Case scenario = Case.valueOf(caseName); | ||
| aPool = new Bytes[SAMPLE_SIZE]; // shift amount (pushed second, popped first) | ||
| bPool = new Bytes[SAMPLE_SIZE]; // value (pushed first, popped second) | ||
|
|
||
| final ThreadLocalRandom random = ThreadLocalRandom.current(); | ||
|
|
||
| for (int i = 0; i < SAMPLE_SIZE; i++) { | ||
| switch (scenario) { | ||
| case SHIFT_0: | ||
| aPool[i] = Bytes.of(0); | ||
| bPool[i] = randomValue(random); | ||
| break; | ||
|
|
||
| case SHIFT_1: | ||
| aPool[i] = Bytes.of(1); | ||
| bPool[i] = randomValue(random); | ||
| break; | ||
|
|
||
| case SHIFT_128: | ||
| aPool[i] = Bytes.of(128); | ||
| bPool[i] = randomValue(random); | ||
| break; | ||
|
|
||
| case SHIFT_255: | ||
| aPool[i] = Bytes.of(255); | ||
| bPool[i] = randomValue(random); | ||
| break; | ||
|
|
||
| case OVERFLOW_SHIFT_256: | ||
| aPool[i] = Bytes.fromHexString("0x0100"); // 256 | ||
| bPool[i] = randomValue(random); | ||
| break; | ||
|
|
||
| case OVERFLOW_LARGE_SHIFT: | ||
| aPool[i] = Bytes.fromHexString("0x010000000000"); // > 4 bytes | ||
| bPool[i] = randomValue(random); | ||
| break; | ||
|
|
||
| case FULL_RANDOM: | ||
| default: | ||
| final byte[] shift = new byte[1 + random.nextInt(4)]; | ||
| final byte[] value = new byte[1 + random.nextInt(32)]; | ||
| random.nextBytes(shift); | ||
| random.nextBytes(value); | ||
| aPool[i] = Bytes.wrap(shift); | ||
| bPool[i] = Bytes.wrap(value); | ||
| break; | ||
| } | ||
| } | ||
| index = 0; | ||
| } | ||
| } |
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.
I fixed this on main too so watch out for conflicts/duplication