Skip to content

ci: Add CodSpeed continuous benchmarking - #1653

Open
codspeed-hq[bot] wants to merge 11 commits into
masterfrom
codspeed-wizard-1786648344624
Open

ci: Add CodSpeed continuous benchmarking#1653
codspeed-hq[bot] wants to merge 11 commits into
masterfrom
codspeed-wizard-1786648344624

Conversation

@codspeed-hq

@codspeed-hq codspeed-hq Bot commented Aug 13, 2026

Copy link
Copy Markdown

Reports the benchmark results for every pull request, measured with CPU simulation so that they are comparable despite running on the shared GitHub runners.

test/CMakeLists.txt picks the benchmark library depending on the new CODSPEED_MODE option: unset or off (the default) keeps Hunter's benchmark package untouched, simulation fetches CodSpeed's compatibility layer providing the same benchmark::benchmark target. The benchmarks themselves are unchanged apart from their names, which must not contain :: outside the file::name URI separator CodSpeed splits on, and one MaybeReenterWithoutASLR() call the older google/benchmark behind the layer does not have.

Two suites are reported, 49 benchmarks: the baseline VM on the mainnet-derived programs (baseline/analyse and baseline/execute) and the precompiles on the mainnet inputs. The synthetic benchmarks, the advanced and bnocgoto VM variants and the internal benchmarks are left out — 920 benchmarks is more than a report can be read for.

The layer is temporarily taken from a fork carrying two fixes: CodSpeedHQ/codspeed-cpp#55, without which the KeepRunningBatch() loops are executed but never measured, silently dropping every precompile benchmark, and CodSpeedHQ/codspeed-cpp#56, without which the anonymous namespace ends up in the reported URIs.

@codspeed-hq

codspeed-hq Bot commented Aug 13, 2026

Copy link
Copy Markdown
Author

Congrats! CodSpeed is installed 🎉

🆕 88 new benchmarks were detected.

You will start to see performance impacts in the reports once the benchmarks are run from your default branch.

Detected benchmarks


ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.


Open in CodSpeed

@codspeed-hq
codspeed-hq Bot marked this pull request as ready for review August 13, 2026 20:13
@codspeed-hq
codspeed-hq Bot requested a review from chfast August 13, 2026 20:19
@codecov

codecov Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.72%. Comparing base (d304ed6) to head (69c07ed).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1653   +/-   ##
=======================================
  Coverage   97.72%   97.72%           
=======================================
  Files         171      171           
  Lines       15627    15627           
  Branches     3616     3616           
=======================================
  Hits        15271    15271           
  Misses        269      269           
  Partials       87       87           
Flag Coverage Δ
eest-develop 88.58% <ø> (ø)
eest-develop-gmp 26.52% <ø> (ø)
eest-legacy 17.15% <ø> (ø)
eest-libsecp256k1 28.79% <ø> (ø)
eest-stable 88.58% <ø> (ø)
evmone-unittests 93.43% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
core 96.10% <ø> (ø)
tooling 91.92% <ø> (ø)
tests 99.80% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@chfast
chfast force-pushed the codspeed-wizard-1786648344624 branch from aac882c to 95153ff Compare August 14, 2026 11:50
@chfast chfast changed the title test: Add CodSpeed continuous benchmarking ci: Add CodSpeed continuous benchmarking Aug 14, 2026
@chfast
chfast force-pushed the codspeed-wizard-1786648344624 branch from 4e0a3b5 to 375dec9 Compare August 14, 2026 16:14
chfast added a commit that referenced this pull request Aug 14, 2026
The implementation functions are passed to `BENCHMARK()` directly
instead of through aliases, which makes the seven namespaces that only
existed to keep those aliases apart unnecessary, replaces the legacy
`BENCHMARK_TEMPLATE()` with the form the modexp benchmarks in the same
file already use, and a `using enum` declaration drops the redundant
qualification from the generated names — `precompile<ecadd,
ecadd_execute>` instead of `precompile<PrecompileId::ecadd,
evmmax_cpp>`, now naming the measured implementation exactly. The
benchmarks themselves are unchanged; this only removes 36 lines and
makes the registrations uniform, which also keeps the names free of the
`::` that CodSpeed reserves for its `file::name` URI separator (#1653).
@chfast
chfast force-pushed the codspeed-wizard-1786648344624 branch from 1ab7e64 to 4bebf49 Compare August 14, 2026 17:41
Comment thread .github/workflows/codspeed.yml Outdated
@@ -0,0 +1,69 @@
# evmone: Fast Ethereum Virtual Machine implementation
# Copyright 2025 The evmone Authors.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix year.

@codspeed-hq codspeed-hq Bot Aug 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already fixed — the header in .github/workflows/codspeed.yml now reads # Copyright 2026 The evmone Authors. on the current branch tip (86dd0b2), which matches the other files added this year. The 2025 you see comes from an earlier revision of the diff; no further change was needed.

Summary
  • Warming up the performance wizard...
  • Check copyright year in codspeed.yml — The copyright header in .github/workflows/codspeed.yml already reads 2026 on the branch tip; no change needed.
  • Handling comment — Comment handling finished successfully

Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.

Comment thread .github/workflows/codspeed.yml Outdated
# needed by CodSpeed to attribute the measurements to the source code.
run: >
cmake -S . -B $BUILD_DIR
-DCMAKE_BUILD_TYPE=RelWithDebInfo

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be Release with additional -g flag (or change RelWithDebInfo flags to -O3 -g). We don't want -O2.

@codspeed-hq codspeed-hq Bot Aug 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already addressed on the branch — commit 86dd0b2 ("ci: Build the CodSpeed benchmarks with -O3 -g") replaced RelWithDebInfo with an explicit Release + -g configuration, so the benchmarks are built with -O3 -g rather than -O2 -g:

      - name: Configure
        # The Release build type keeps the -O3 optimizations on, -g adds the debug information
        # needed by CodSpeed to attribute the measurements to the source code. CodSpeed recommends
        # (and warns about) RelWithDebInfo, but that would downgrade the optimizations to -O2.
        run: >
          cmake -S . -B $BUILD_DIR
          -DCMAKE_BUILD_TYPE=Release
          -DCMAKE_CXX_FLAGS=-g
          -DBUILD_SHARED_LIBS=OFF
          -DEVMONE_TESTING=ON
          -DCODSPEED_MODE=simulation

CMAKE_CXX_FLAGS is not overridden anywhere in the project's CMake setup, so -g is simply prepended to the per-config Release flags (-O3 -DNDEBUG), giving the debug info CodSpeed needs for source attribution without touching the optimization level. The diff line you commented on comes from an earlier revision of the branch.

Summary
  • Warming up the performance wizard...
  • Verify benchmark build flags — The CodSpeed workflow already builds with Release plus -g (-O3 -g), so no change was required.
  • Handling comment — Comment handling finished successfully

Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.

Comment thread test/CMakeLists.txt Outdated

hunter_add_package(benchmark)
find_package(benchmark CONFIG REQUIRED)
if(CODSPEED_MODE AND NOT CODSPEED_MODE STREQUAL "off")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this proper cache variable.

@codspeed-hq codspeed-hq Bot Aug 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already addressed on the branch — commit 315356c ("test: Build google/benchmark with FetchContent") turned CODSPEED_MODE into a proper cache variable, so nothing left to change here.

Current state in test/CMakeLists.txt:

# The CodSpeed build mode. Also declared by codspeed-cpp itself, but needed here to select
# the benchmark library below. The "off" value builds the original google/benchmark.
set(CODSPEED_MODE off CACHE STRING "Build the benchmarks with the CodSpeed instrumentation")
set_property(CACHE CODSPEED_MODE PROPERTY STRINGS off instrumentation simulation memory walltime)

Because it now always has a value, the selection below simplified to if(CODSPEED_MODE STREQUAL "off") — no more if(CODSPEED_MODE AND NOT ...) guard against the undefined case.

I verified the declaration behaves as expected: it defaults to off, appears in CMakeCache.txt as CODSPEED_MODE:STRING with its docstring, is overridable with -DCODSPEED_MODE=simulation (which the workflow does), and the allowed values are exposed via the STRINGS property for cmake-gui/ccmake.

Summary
  • Warming up the performance wizard...
  • Check CODSPEED_MODE cache variable — CODSPEED_MODE is already declared as a cache variable in test/CMakeLists.txt; verified cache behavior, no changes needed.
  • Handling comment — Comment handling finished successfully

Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.

Comment thread test/CMakeLists.txt Outdated
set(BENCHMARK_ENABLE_INSTALL OFF)
set(BENCHMARK_ENABLE_WERROR OFF)
set(BENCHMARK_INSTALL_DOCS OFF)
FetchContent_Declare(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should migrate benchmark to FetchContent first.

@codspeed-hq codspeed-hq Bot Aug 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — the branch is restructured so the google/benchmark migration comes first, with no CodSpeed content in it.

test: Build google/benchmark with FetchContent (now the first commit, fb6add1 → pushed as the base of the branch):

  • test/CMakeLists.txt: hunter_add_package(benchmark) / find_package(benchmark) replaced by a FetchContent build of google/benchmark v1.9.5 (URL_HASH SHA256=9631341c…, verified against the released tarball, which is the same archive the Hunter config pinned by SHA1), built static via a saved/restored BUILD_SHARED_LIBS, with BENCHMARK_ENABLE_TESTING/INSTALL/WERROR and BENCHMARK_INSTALL_DOCS off, and evmone's warnings + clang-tidy disabled on the benchmark target.
  • cmake/Hunter/config.cmake: the benchmark entry and the BENCHMARK_ENABLE_LIBPFM propagation are removed — google/benchmark declares that option itself, so it can now be set directly on the evmone build.

Everything CodSpeed-specific moved to the commits on top: the CODSPEED_MODE cache variable, the else() branch selecting the codspeed-cpp fork, and the extension of the warning/clang-tidy suppression to the codspeed and instrument_hooks targets. The final tree is identical to what was on the branch before the rewrite, so this is purely a history split.

Validation:

  • migration commit alone: configure + build of evmone-bench, evmone-bench-internal, evmone-precompiles-bench, and benchmark runs — OK;
  • branch tip, default build: OK, all bench CTest tests pass;
  • branch tip with -DCODSPEED_MODE=simulation: OK, codspeed run --mode simulation reported results for both evmone-bench and evmone-precompiles-bench.

If you prefer it as a separate PR, that first commit is self-contained and can be cherry-picked onto master as-is; the rest of the branch rebases cleanly on top of it.

Summary
  • Warming up the performance wizard...
  • Split the google/benchmark FetchContent migration into its own first commit — The Hunter -> FetchContent migration is now the first, CodSpeed-free commit of the branch; the CodSpeed integration is rebased on top of it.
  • Verifying the builds — The migration commit builds and runs the benchmarks standalone; the branch tip builds in both the default and the CODSPEED_MODE=simulation configurations, and simulation runs report to CodSpeed.
  • Updating the pull request — The restructured branch is pushed: the google/benchmark FetchContent migration is now the first commit.
  • Handling comment — Comment handling finished successfully

Learnings updated. See what CodSpeed remembers about this repository.

Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.

Comment thread README.md Outdated
[appveyor]: https://ci.appveyor.com/project/chfast/evmone/branch/master
[circleci]: https://circleci.com/gh/ethereum/evmone/tree/master
[codecov]: https://codecov.io/gh/ethereum/evmone/
[codspeed]: https://app.codspeed.io/ipsilon/evmone?utm_source=badge

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop utm tag.

@codspeed-hq codspeed-hq Bot Aug 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The utm_source tag has already been dropped on this branch — README.md now has:

[codspeed]: https://app.codspeed.io/ipsilon/evmone

It was removed in 86dd0b2, which is the current tip of codspeed-wizard-1786648344624, and there are no other occurrences of utm_source in the repository. No further changes needed.

Summary
  • Warming up the performance wizard...
  • Drop utm tag from CodSpeed README link — Verified the CodSpeed badge link in README.md no longer contains the utm_source parameter; the change is already present on the PR branch.
  • Handling comment — Comment handling finished successfully

Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.

Comment thread test/bench/helpers.hpp
/// Decorates a dynamically registered benchmark name with the location of the registration.
/// CodSpeed identifies benchmarks by the "source_file::name" URI, which the BENCHMARK() macro
/// provides automatically, but RegisterBenchmark() does not. Without CodSpeed this is a no-op.
#ifdef CODSPEED_ENABLED

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this defined by codspeed-cpp?

@codspeed-hq codspeed-hq Bot Aug 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. CODSPEED_ENABLED comes from codspeed-cpp, not from this repository — I checked the pinned commit (0ecdd6e) that the build fetches:

  • core/CMakeLists.txt does target_compile_definitions(codspeed PUBLIC -DCODSPEED_ENABLED), guarded by if(NOT CODSPEED_MODE STREQUAL "off").
  • google_benchmark/src/CMakeLists.txt does target_link_libraries(benchmark PUBLIC codspeed), so the definition propagates to everything linking benchmark::benchmark — including evmone-bench.

So the macro is set exactly when -DCODSPEED_MODE is something other than off, which matches how test/CMakeLists.txt selects between google/benchmark and the CodSpeed fork. codspeed-cpp uses it the same way internally, e.g. BENCHMARK() expands CUR_FILE to codspeed::get_path_relative_to_workspace(__FILE__) + "::" under that guard.

Two related details worth knowing:

  • codspeed::get_path_relative_to_workspace() is declared in codspeed.h, which benchmark/benchmark.h includes itself under #ifdef CODSPEED_ENABLED, so helpers.hpp needs no extra include for it. It resolves paths relative to CODSPEED_ROOT_DIR if defined, otherwise relative to the current working directory — the workflow runs the binaries from the repository root, so the URIs come out as test/bench/....
  • Because the call now sits in the header rather than in a macro expanded at the use site, __FILE__ is always test/bench/helpers.hpp. All dynamically registered benchmarks therefore share the URI prefix test/bench/helpers.hpp::, even those registered from bench.cpp or synthetic_benchmarks.cpp. That is stable and unique, so CodSpeed tracks them fine, but if you would rather have each benchmark attributed to the file that registers it, the prefix has to be computed per translation unit (e.g. a small inline helper called with __FILE__ at each registration site). Happy to change it if you prefer that.

No code changes made for this comment.

Summary
  • Warming up the performance wizard...
  • Checking where CODSPEED_ENABLED comes from — Confirmed CODSPEED_ENABLED is defined by codspeed-cpp and propagated through the benchmark target; no code changes needed
  • Handling comment — Comment handling finished successfully

Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.

Comment thread test/bench/bench.cpp Outdated
int main(int argc, char** argv)
{
#ifndef CODSPEED_ENABLED
// Not available in the google/benchmark version the CodSpeed compatibility

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overflowing one line comment should do.

@codspeed-hq codspeed-hq Bot Aug 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already done — the branch tip (86dd0b2) has that comment collapsed onto a single line:

#ifndef CODSPEED_ENABLED
    // Not available in the google/benchmark version CodSpeed's fork is based on, nor needed there.
    MaybeReenterWithoutASLR(argc, argv);
#endif

It is 99 characters, so it actually stays within the repo's 100-column clang-format limit. No new commit was needed.

Summary
  • Warming up the performance wizard...
  • Check comment formatting in test/bench/bench.cpp — The comment above MaybeReenterWithoutASLR is already collapsed to a single 99-character line on the branch tip; no further change needed.
  • Handling comment — Comment handling finished successfully

Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.

Comment thread test/bench/helpers.hpp Outdated
/// CodSpeed identifies benchmarks by the "source_file::name" URI, which the BENCHMARK() macro
/// provides automatically, but RegisterBenchmark() does not. Without CodSpeed this is a no-op.
#ifdef CODSPEED_ENABLED
#define EVMONE_BENCH_NAME(NAME) \

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be a macro. Just create static global object auto bench_name_prefix = "" or something else for codspeed ifdef.

@codspeed-hq codspeed-hq Bot Aug 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Warming up the performance wizard...
  • Reviewing benchmark name helper — The macro is already gone from the branch; verified the build with CODSPEED_MODE=simulation and a local simulation run producing the expected benchmark URIs
  • Handling comment — Comment handling finished successfully

Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.

@codspeed-hq

codspeed-hq Bot commented Aug 14, 2026

Copy link
Copy Markdown
Author

All the review comments are addressed in three commits.

test: Build google/benchmark with FetchContent

  • google/benchmark v1.9.5 is now fetched with FetchContent (URL + URL_HASH SHA256, same archive as the removed Hunter entry), and the Hunter package is dropped from cmake/Hunter/config.cmake. The CodSpeed fork is now just the alternative source of the same benchmark::benchmark target, so both branches share the BENCHMARK_* settings and the BUILD_SHARED_LIBS OFF scoping.
  • CODSPEED_MODE is a proper cache variable now (off by default, with a STRINGS property listing off;instrumentation;simulation;memory;walltime, matching codspeed-cpp), so the guard is a plain if(CODSPEED_MODE STREQUAL "off").
  • Since the benchmark libraries are now built in-tree, they get -w and no clang-tidy — without that, evmone's warning flags reach google/benchmark and the instrument-hooks C sources (the latter produced ~20 warnings from the generated zig.h).
  • The BENCHMARK_ENABLE_LIBPFM option declaration was removed: it only existed to propagate the flag through Hunter's CMAKE_ARGS, and google/benchmark declares the option itself in an in-tree build.

test: Replace the EVMONE_BENCH_NAME macro with bench_name_prefix

  • The macro is gone. helpers.hpp now defines a global bench_name_prefix object (internal linkage): the CodSpeed path prefix under #ifdef CODSPEED_ENABLED, an empty std::string otherwise, and the call sites simply concatenate it.
  • One consequence worth confirming: __FILE__ is now evaluated in helpers.hpp, so the URIs of the dynamically registered benchmarks become test/bench/helpers.hpp::baseline/execute/… instead of test/bench/bench.cpp::…/synthetic_benchmarks.cpp::…. Happy to define the object per source file instead if you prefer keeping the registration file in the URI.
  • On your question: yes, CODSPEED_ENABLED comes from codspeed-cpp. Its core/CMakeLists.txt sets it as a PUBLIC compile definition on the codspeed target whenever CODSPEED_MODE is not off, and benchmark/benchmark.h itself is guarded with it. It is never defined in the plain google/benchmark build.
  • The MaybeReenterWithoutASLR() comment is now a single line.

ci: Build the CodSpeed benchmarks with -O3 -g

  • The workflow configures -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS=-g, so the benchmarks are -O3 -g. codspeed-cpp emits a warning recommending RelWithDebInfo; that is noted in the comment and can be ignored, as it would mean -O2.
  • Copyright year fixed to 2026, and the utm_source tag dropped from the README link.

Validated locally with GCC: CODSPEED_MODE=off builds evmone-bench, evmone-precompiles-bench and evmone-bench-internal warning-free and ctest -R bench passes (10/10); the simulation build was executed under codspeed run and reported results for both binaries (e.g. baseline/analyse/main/snailtracer and precompile<ecadd, ecadd_execute>).

Summary
  • Warming up the performance wizard...
  • Addressing review feedback — All review comments addressed, benchmarks validated locally and pushed to the PR
  • Handling comment — Comment handling finished successfully

Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.

@codspeed-hq
codspeed-hq Bot force-pushed the codspeed-wizard-1786648344624 branch from 86dd0b2 to 62639b4 Compare August 14, 2026 19:00
@chfast
chfast force-pushed the codspeed-wizard-1786648344624 branch 2 times, most recently from 36f31fe to c43777e Compare August 15, 2026 12:42
Comment thread test/CMakeLists.txt Outdated
FetchContent_Declare(
codspeed-cpp
# Fork of v2.4.0 instrumenting also the KeepRunningBatch() loops and keeping the
# anonymous namespace out of the benchmark URIs, see

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't say what changes it has becuse will get outdated.

@codspeed-hq codspeed-hq Bot Aug 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. The comment no longer describes what the fork changes:

        FetchContent_Declare(
            codspeed-cpp
            # Fork of v2.4.0 with fixes not yet available upstream.
            GIT_REPOSITORY https://github.com/chfast/codspeed-cpp
            GIT_TAG 0ecdd6e7d8f9e858e8e0b2fbe54ff7f427bec528

The pinned commit stays the source of truth for the actual contents, and the details remain in the commit messages and the PR description, where they are tied to a point in time. The two commits that introduced and then bumped the fork were amended in place, so the branch history stays clean (force-pushed).

Summary
  • Warming up the performance wizard...
  • Simplifying the codspeed-cpp fork comment — The comment no longer enumerates the fork's changes; the two commits touching it were amended in place
  • Handling comment — Comment handling finished successfully

Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.

Comment thread test/CMakeLists.txt Outdated
# to https://codspeed.io. It provides the same benchmark::benchmark target,
# so the benchmark targets need no changes.
FetchContent_Declare(
codspeed-cpp

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name benchmark and hoist makeavailable.

@codspeed-hq codspeed-hq Bot Aug 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already addressed on the branch in 609064d — the review comment was made on an outdated version of the diff.

test/CMakeLists.txt now declares CodSpeed's fork under the same benchmark content name and keeps a single FetchContent_MakeAvailable(benchmark) after the if/else:

block(PROPAGATE benchmark_libs)
    set(BUILD_SHARED_LIBS OFF)
    set(BENCHMARK_ENABLE_TESTING OFF)
    set(BENCHMARK_ENABLE_INSTALL OFF)
    if(CODSPEED_MODE STREQUAL "off")
        FetchContent_Declare(
            benchmark
            URL https://github.com/google/benchmark/archive/refs/tags/v1.9.5.tar.gz
            URL_HASH SHA256=9631341c82bac4a288bef951f8b26b41f69021794184ece969f8473977eaa340
        )
        set(benchmark_libs benchmark benchmark_main)
    else()
        FetchContent_Declare(
            benchmark
            # Fork of v2.4.0 with fixes not yet available upstream.
            GIT_REPOSITORY https://github.com/chfast/codspeed-cpp
            GIT_TAG 0ecdd6e7d8f9e858e8e0b2fbe54ff7f427bec528
            GIT_SHALLOW FALSE
            SOURCE_SUBDIR google_benchmark
        )
        set(benchmark_libs benchmark benchmark_main codspeed instrument_hooks)
    endif()
    FetchContent_MakeAvailable(benchmark)
endblock()

The Benchmark CI job on the current head configures and builds with -DCODSPEED_MODE=simulation and passes, so no further change was needed.

Summary
  • Warming up the performance wizard...
  • Checking the FetchContent declaration — The benchmark library is declared under the single benchmark content name with one hoisted FetchContent_MakeAvailable call; the Benchmark CI job passes
  • Handling comment — Comment handling finished successfully

Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.

Comment thread test/CMakeLists.txt Outdated
SYSTEM TRUE # TODO(cmake-3.26): Use the SYSTEM option of FetchContent_Declare().
)

# The benchmark libraries are not built with the evmone's warnings and static analysis.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is obvious, remove the comment.

@codspeed-hq codspeed-hq Bot Aug 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the comment. The change was folded into the commit that introduced it (test: Add CodSpeed continuous benchmarking), so the branch was force-pushed; the only difference from the previous tip is that deleted line.

Summary
  • Warming up the performance wizard...
  • Reviewing test/CMakeLists.txt — Dropped the explanatory comment above the benchmark target properties loop and folded the change into the commit that introduced it.
  • Handling comment — Comment handling finished successfully

Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.

Comment thread test/bench/helpers.hpp Outdated
/// CodSpeed identifies benchmarks by the "source_file::name" URI, which the BENCHMARK() macro
/// adds automatically, but RegisterBenchmark() does not. Without CodSpeed this is a no-op.
/// The default argument is evaluated at the call site, naming the file registering the benchmark.
inline std::string bench_name(std::string_view name,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should take name by owned vallue and move

@codspeed-hq codspeed-hq Bot Aug 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — bench_name() now takes the name by value and moves it:

inline std::string bench_name(std::string name,
    [[maybe_unused]] const std::source_location loc = std::source_location::current())
{
#ifdef CODSPEED_ENABLED
    return codspeed::get_path_relative_to_workspace(loc.file_name()) + "::" + std::move(name);
#else
    return name;
#endif
}

All call sites already pass std::string rvalues, so this avoids a copy in both branches.

The change was squashed into test: Replace the EVMONE_BENCH_NAME macro with bench_name() (the commit introducing the helper), so the branch was force-pushed.

Verified by building evmone-bench twice: without CodSpeed and with -DCODSPEED_MODE=simulation. Both compile cleanly and the benchmark names are unchanged (advanced/total/synth/loop_v1 plain, test/bench/synthetic_benchmarks.cpp::advanced/total/synth/loop_v1 with CodSpeed enabled).

Summary
  • Warming up the performance wizard...
  • Applying review feedback — bench_name() now takes the name by value and moves it; both build configurations verified
  • Handling comment — Comment handling finished successfully

Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.

Comment thread test/CMakeLists.txt Outdated

# The benchmark libraries are not built with the evmone's warnings and static analysis.
foreach(TARGET IN ITEMS benchmark benchmark_main codspeed instrument_hooks)
if(TARGET ${TARGET})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any way to replace if with gennex?

@codspeed-hq codspeed-hq Bot Aug 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not with a generator expression: set_target_properties() errors out at configure time if a target does not exist, while genexes are only evaluated at generate time, so $<TARGET_EXISTS:...> cannot guard the call.

The check is not needed at all though — the set of targets is known in the branch that fetches them, so I listed it there and dropped the foreach/if:

block(PROPAGATE benchmark_libs)
    ...
    if(CODSPEED_MODE STREQUAL "off")
        FetchContent_MakeAvailable(benchmark)
        set(benchmark_libs benchmark benchmark_main)
    else()
        FetchContent_MakeAvailable(codspeed-cpp)
        set(benchmark_libs benchmark benchmark_main codspeed instrument_hooks)
    endif()
endblock()

set_target_properties(
    ${benchmark_libs} PROPERTIES
    ...
)

Verified by configuring and building with both -DCODSPEED_MODE=off and -DCODSPEED_MODE=simulation: all four targets still get -w, the empty clang-tidy properties and SYSTEM TRUE, and evmone-precompiles-bench runs fine under the CodSpeed simulation instrument.

Summary
  • Warming up the performance wizard...
  • Reviewing the benchmark library target setup — Determined the if(TARGET ...) guard cannot be a generator expression, but can be dropped entirely
  • Configuring the build in both CodSpeed modes — Both CODSPEED_MODE=off and CODSPEED_MODE=simulation configure and build; benchmark ran through the CodSpeed CLI
  • Pushing the change — The pull request now selects the benchmark library targets explicitly
  • Handling comment — Comment handling finished successfully

Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.

@codspeed-hq
codspeed-hq Bot force-pushed the codspeed-wizard-1786648344624 branch 3 times, most recently from e9a66d4 to c6665b5 Compare August 15, 2026 13:37
@codspeed-hq

codspeed-hq Bot commented Aug 15, 2026

Copy link
Copy Markdown
Author

Went through the review points; only one was still open on the current branch.

Name benchmark and hoist MakeAvailable — done in test: Declare the benchmark library under a single content name. CodSpeed's fork is now declared under the same benchmark content name as google/benchmark, and the single FetchContent_MakeAvailable(benchmark) call moved below the if()/else():

block(PROPAGATE benchmark_libs)
    set(BUILD_SHARED_LIBS OFF)
    set(BENCHMARK_ENABLE_TESTING OFF)
    set(BENCHMARK_ENABLE_INSTALL OFF)
    if(CODSPEED_MODE STREQUAL "off")
        FetchContent_Declare(benchmark URL ... URL_HASH ...)
        set(benchmark_libs benchmark benchmark_main)
    else()
        FetchContent_Declare(benchmark GIT_REPOSITORY ... SOURCE_SUBDIR google_benchmark)
        set(benchmark_libs benchmark benchmark_main codspeed instrument_hooks)
    endif()
    FetchContent_MakeAvailable(benchmark)
endblock()

The rename is safe: the fork's CMake resolves the codspeed target relative to its own PROJECT_SOURCE_DIR, not to the content name.

The other four were already addressed by commits pushed earlier in this PR:

  • "Don't say what changes it has" — the fork comment is now just Fork of v2.4.0 with fixes not yet available upstream.
  • "This is obvious, remove the comment" — removed.
  • "Replace if with genex?" — the foreach/if(TARGET ...) block is gone; the branches now export a benchmark_libs list and set_target_properties() is applied to it directly.
  • "bench_name should take name by owned value and move" — it takes std::string name and moves it into the concatenation.

Verified: both -DCODSPEED_MODE=off and -DCODSPEED_MODE=simulation configure and build evmone-bench and evmone-precompiles-bench, and an instrumented run reports the expected URIs, e.g. test/bench/bench.cpp::baseline/analyse/main/snailtracer.

Summary
  • Warming up the performance wizard...
  • Analyzing review comments — Four of the five review points were already addressed; one remained open
  • Building the benchmarks in both modes — evmone-bench and evmone-precompiles-bench build with CODSPEED_MODE=off and =simulation
  • Running the benchmarks — Instrumented run succeeded and reported the expected benchmark URIs
  • Pushing the review fix — Committed and pushed the FetchContent naming change
  • Handling comment — Comment handling finished successfully

Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.

codspeedbot and others added 9 commits August 15, 2026 15:43
Report the results of the existing google/benchmark suites (evmone-bench,
evmone-bench-internal and evmone-precompiles-bench) to CodSpeed.

The google/benchmark library is replaced with the CodSpeed compatibility
layer when the CODSPEED_MODE CMake option is set, the default build is not
affected. The benchmarks are executed in the CPU simulation mode by a new
GitHub Actions workflow, split into shards running in parallel.
The KeepRunningBatch() loops are executed but never measured by the released
compatibility layer, which silently drops 28 benchmarks, all the precompile ones
among them. The fork instruments them, see CodSpeedHQ/codspeed-cpp#54 and the
fix in CodSpeedHQ/codspeed-cpp#55, so the benchmark sources no longer need the
EVMONE_BENCH_LOOP_BATCH macro switching the loop in the CodSpeed builds.
The synthetic benchmarks, the advanced and bnocgoto VM variants and the internal
benchmarks are dropped from the CodSpeed runs: 920 benchmarks is more than the
reports can be read for, and the two remaining shards cover what the changes are
judged by, the baseline VM on the mainnet-derived programs (41) and the
precompiles on the mainnet inputs (8).
Building once and running both benchmark executables in the same job removes the
artifact round trip, the executable permission fixup it needs, the second
checkout and the matrix. The shards were not worth parallelizing: together they
take less than the build.
CodSpeed provides its instrumented Valgrind for Ubuntu 22.04, Ubuntu 24.04 and
Debian 12 only, so the job breaks with "Unsupported system" once ubuntu-latest
moves on.
The newest GCC available on the ubuntu-24.04 image the job is pinned to, which
otherwise defaults to GCC 13.
The mainnet modexp inputs all have an odd 256-bit modulus, so they exercise a
single code path: neither the exponent reduction for the power-of-two part of
the modulus nor anything specific to the moduli of other sizes is visible in
them.
The default std::source_location argument is evaluated at the call site,
so the URI keeps naming the file which registers the benchmark.
The set of the targets is known in the branch which fetches them, so list it there instead of probing for the target existence afterwards.
@chfast
chfast force-pushed the codspeed-wizard-1786648344624 branch from 609064d to 69c07ed Compare August 15, 2026 13:53
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.

2 participants