Review precompiles benchmarks to display mgas/s #8495

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.

Sign up for GitHub

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

Jump to bottom

Merged

ahamlat merged 12 commits into hyperledger:main from ahamlat:precompiles-and-opcodes-benchmarks

Apr 4, 2025

Contributor

ahamlat commented Mar 31, 2025 •

edited

Loading

PR description

Review precompiles benchmarking to handle both use cases :

Evaluate precompiles performance by displaying per call latency and mgas/s
Calculate the derived gas from the the precompile call latency and the target of 100 mgas/s

This PR adds the benchmark numbers for KZGPointEval precompile, and introduces a header section displaying the derived gas formula and system properties, ensuring that precompile performance output is structured and easy to read.

The output looks like this with this PR

Secp256k1 signature recovery   |   3,000 gas cost |   3,896 calculated gas for execution time per call    38,955 ns |   77.01 MGps
...
RIPMD 256 bytes                |   1,560 gas cost |     246 calculated gas for execution time per call     2,464 ns |  633.12 MGps
BNADD                          |     150 gas cost |     142 calculated gas for execution time per call     1,422 ns |  105.49 MGps
BNMUL                          |   6,000 gas cost |   4,423 calculated gas for execution time per call    44,225 ns |  135.67 MGps
BNPairings 2                   |  79,000 gas cost |  70,370 calculated gas for execution time per call   703,702 ns |  112.26 MGps
BNPairings 4                   | 113,000 gas cost | 107,233 calculated gas for execution time per call 1,072,328 ns |  105.38 MGps
...

You can find the whole output attached to this PR #8495 (comment).

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

Checked out our contribution guidelines?
Considered documentation and added the doc-change-required label to this PR if updates are required.
Considered the changelog and included an update if required.
For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

spotless: ./gradlew spotlessApply
unit tests: ./gradlew build
acceptance tests: ./gradlew acceptanceTest
integration tests: ./gradlew integrationTest
reference tests: ./gradlew ethereum:referenceTests:referenceTests


          Change the output of precompile (and opcodes) Benchmarks to display m…

bf20e67

…gas/s

Signed-off-by: Ameziane H. <ameziane.hamlat@consensys.net>

garyschulte reviewed

View reviewed changes

Contributor

garyschulte left a comment

This output format is more intuitive and useful imo. I think if we need to try to derive gas costs/targets, we can still do so.

evm/src/test/java/org/hyperledger/besu/evm/precompile/Benchmarks.java Outdated Show resolved Hide resolved

evm/src/test/java/org/hyperledger/besu/evm/precompile/Benchmarks.java Outdated Show resolved Hide resolved

Contributor Author

ahamlat commented Apr 1, 2025 •

edited

Loading

@shemnon What do you think of using actual execution time and the gas requirement to get the mgas/s, instead of deriving a theoretical gas cost from execution time, with the assumption that 35 mgas should take 1 second ?
I guess this was a way to detect if the pricing is fair for precompiles, at least on besu side. I would like to change it to display a report of all the precompiles performances. What do you think ?

My proposal is to update the benchmark output to focus on actual execution performance (e.g., time per call and Mgas/s), rather than estimating the gas cost.

ahamlat added 3 commits

April 1, 2025 15:57


          Update the other precompiles, and do some cleanup.

f44f00f

Signed-off-by: Ameziane H. <ameziane.hamlat@consensys.net>


          Merge branch 'main' into precompiles-and-opcodes-benchmarks

b3de209

Signed-off-by: ahamlat <ameziane.hamlat@consensys.net>


          Fix build issue following merge with main.

adfc989

Signed-off-by: Ameziane H. <ameziane.hamlat@consensys.net>

Contributor

shemnon commented Apr 1, 2025

If we are not trying to determine what a precompile should cost then the MGps is a fine metric. But if the gas schedule is up for debate I found the standard burn rate a reasonable proxy.

35 Mgps was an old 2020 era standard and I think we are starting to get pressure for 100 MGps. But the keccak opcode is going to hold us back.


          Remove the logger for now.

3f95039

Signed-off-by: Ameziane H. <ameziane.hamlat@consensys.net>

Contributor Author

ahamlat commented Apr 1, 2025 •

edited

Loading

I think we can keep both, then we can use the output for both use cases. I will update the PR. I agree on 100 mgas/s, I will update to that target.

ahamlat added 4 commits

April 1, 2025 19:48


          Add the derived gas.

d41c456

Signed-off-by: Ameziane H. <ameziane.hamlat@consensys.net>


          - Refactoring and formatting the output.

ad87421

- Add a header to the output

Signed-off-by: Ameziane H. <ameziane.hamlat@consensys.net>


          Merge branch 'main' into precompiles-and-opcodes-benchmarks

d4ad713


          Fix errorProne issues

eeb3c5d

Signed-off-by: Ameziane H. <ameziane.hamlat@consensys.net>

ahamlat marked this pull request as ready for review

April 2, 2025 10:10

Contributor Author

ahamlat commented Apr 2, 2025 •

edited

Loading

I updated the description, and added the output file below. @shemnon @garyschulte feel free to review.
PR8495-output.zip

siladu approved these changes

View reviewed changes

Contributor

siladu left a comment

LGTM

evm/src/test/java/org/hyperledger/besu/evm/precompile/Benchmarks.java Show resolved Hide resolved

ahamlat added 3 commits

April 3, 2025 12:03


          Merge branch 'main' into precompiles-and-opcodes-benchmarks

4852f4b


          Increase the warmup threshold

ab17f33

Signed-off-by: Ameziane H. <ameziane.hamlat@consensys.net>


          Merge branch 'main' into precompiles-and-opcodes-benchmarks

4ee19cf

ahamlat merged commit 506a32c into hyperledger:main

43 checks passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet