Skip to content

build: fix building benchmark#532

Merged
qdeslandes merged 2 commits into
facebook:mainfrom
pzmarzly:push-kosxnmqrtslr
May 12, 2026
Merged

build: fix building benchmark#532
qdeslandes merged 2 commits into
facebook:mainfrom
pzmarzly:push-kosxnmqrtslr

Conversation

@pzmarzly
Copy link
Copy Markdown
Contributor

benchmark_bin currently cannot be built:

$ ninja -C build benchmark_bin
ninja: Entering directory `build'
[1/2] Building CXX object tools/benchmarks/CMakeFiles/benchmark_bin.dir/main.cpp.o
FAILED: [code=1] tools/benchmarks/CMakeFiles/benchmark_bin.dir/main.cpp.o
/usr/bin/c++  -I/root/repos/bpfilter-merge-bpf-maps/tools/benchmarks -I/root/repos/bpfilter-merge-bpf-maps/tests/harness -I/root/repos/bpfilter-merge-bpf-maps/src/libbpfilter/include -I/root/repos/bpfilter-merge-bpf-maps/build/src/libbpfilter/include -g -std=c++20 -DWITH_GZFILEOP -MD -MT tools/benchmarks/CMakeFiles/benchmark_bin.dir/main.cpp.o -MF tools/benchmarks/CMakeFiles/benchmark_bin.dir/main.cpp.o.d -o tools/benchmarks/CMakeFiles/benchmark_bin.dir/main.cpp.o -c /root/repos/bpfilter-merge-bpf-maps/tools/benchmarks/main.cpp
/root/repos/bpfilter-merge-bpf-maps/tools/benchmarks/main.cpp: In function ‘void {anonymous}::single_rule__ip4_saddr(benchmark::State&)’:
/root/repos/bpfilter-merge-bpf-maps/tools/benchmarks/main.cpp:105:20: error: no matching function for call to ‘bf::Rule::Rule(bf_verdict, bool, <brace-enclosed initializer list>, std::vector<bf::Matcher>)’
  105 |                   });
      |                    ^
  • there are 3 candidates
In file included from /root/repos/bpfilter-merge-bpf-maps/tests/harness/Chain.hpp:17,
                 from /root/repos/bpfilter-merge-bpf-maps/tools/benchmarks/main.cpp:31:
    • candidate 1: ‘bf::Rule::Rule(bf_verdict, std::optional<bf_counter>, uint8_t, std::vector<bf::Matcher>)’
      /root/repos/bpfilter-merge-bpf-maps/tests/harness/Rule.hpp:42:5:
         42 |     Rule(bf_verdict verdict,
            |     ^~~~
      • no known conversion for argument 2 from ‘bool’ to ‘std::optional<bf_counter>’
        /root/repos/bpfilter-merge-bpf-maps/tests/harness/Rule.hpp:43:43:
           43 |          std::optional<struct bf_counter> counters = std::nullopt,
              |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
    • candidate 2: ‘constexpr bf::Rule::Rule(const bf::Rule&)’
      /root/repos/bpfilter-merge-bpf-maps/tests/harness/Rule.hpp:25:7:
         25 | class Rule
            |       ^~~~
      • candidate expects 1 argument, 4 provided
    • candidate 3: ‘constexpr bf::Rule::Rule(bf::Rule&&)’
      • candidate expects 1 argument, 4 provided

tools/benchmarks/main.cpp has been broken since lib: rule: convert counters bool to a uint8_t bitfield (0e6c660, 2026-05-08), which updated tests/harness/Rule.hpp but didn't update the benchmark file. Result: every benchmark CI run since then has been silently skipping the benchmark, loading latest results (from 2026-05-08), and reporting "no significant change", i.e. green, for them.

Example: https://github.com/facebook/bpfilter/actions/runs/25743097177/job/75598766760

This PR fixes main.cpp, and updates bfbencher script so that it should fail when it fails to benchmark something.

@pzmarzly pzmarzly requested a review from qdeslandes as a code owner May 12, 2026 16:00
@meta-cla meta-cla Bot added the cla signed label May 12, 2026
@pzmarzly pzmarzly force-pushed the push-kosxnmqrtslr branch from d71721e to 8163129 Compare May 12, 2026 16:04
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 12, 2026

Claude review of PR #532 (4ff0525)

Suggestions

  • Check ordering with both fail flagstools/benchmarks/bfbencher:1081 — fixed in this push (last-commit check now runs first)

Nits

  • Truthiness vs is not Nonetools/benchmarks/bfbencher:73 — fixed in this push (now uses is not None)
  • Keyword-only parametertools/benchmarks/bfbencher:65commit_sha could be keyword-only (*) since all callers use keyword syntax

Workflow run

@qdeslandes
Copy link
Copy Markdown
Contributor

Thanks! For bf-bencher, could you instead add a flag like --fail-on-last-commit-fail? We can have failing benchmarks from time to time in a PR for example, as long as the last commit can be built and benchmarked.

@qdeslandes qdeslandes force-pushed the push-kosxnmqrtslr branch from 8163129 to 1214f34 Compare May 12, 2026 16:29
@pzmarzly pzmarzly force-pushed the push-kosxnmqrtslr branch from 1214f34 to 1d237a3 Compare May 12, 2026 16:36
Comment thread tools/benchmarks/bfbencher Outdated
Comment thread tools/benchmarks/bfbencher Outdated
@pzmarzly pzmarzly force-pushed the push-kosxnmqrtslr branch from 1d237a3 to 4ff0525 Compare May 12, 2026 16:51
Comment thread tools/benchmarks/bfbencher
Comment thread tools/benchmarks/bfbencher
Copy link
Copy Markdown
Contributor

@qdeslandes qdeslandes left a comment

Choose a reason for hiding this comment

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

Alright, lgtm! Thanks for fixing this!

@qdeslandes qdeslandes merged commit 5823ba5 into facebook:main May 12, 2026
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants