From 7cb3445b9f54a06738abaaaa217d07d900ac2bed Mon Sep 17 00:00:00 2001 From: Jeremy Nimmer Date: Sun, 16 Jul 2023 15:10:38 -0700 Subject: [PATCH] [workspace] Patch googlebenchmark to avoid shadow warnings (#19796) This allows us to remove copy-pasta in every benchmark we write. --- geometry/benchmarking/render_benchmark.cc | 3 +- multibody/benchmarking/acrobot.cc | 2 - multibody/benchmarking/cassie.cc | 60 ++++++++++--------- multibody/benchmarking/position_constraint.cc | 3 +- systems/benchmarking/framework_benchmarks.cc | 4 -- .../multilayer_perceptron_benchmark.cc | 3 +- .../remove_overloaded_fixture_set_up.patch | 21 +++++++ .../workspace/googlebenchmark/repository.bzl | 1 + 8 files changed, 58 insertions(+), 39 deletions(-) create mode 100644 tools/workspace/googlebenchmark/patches/remove_overloaded_fixture_set_up.patch diff --git a/geometry/benchmarking/render_benchmark.cc b/geometry/benchmarking/render_benchmark.cc index fed094cb14b5..7003597ee521 100644 --- a/geometry/benchmarking/render_benchmark.cc +++ b/geometry/benchmarking/render_benchmark.cc @@ -84,8 +84,7 @@ class RenderBenchmark : public benchmark::Fixture { material_.AddProperty("label", "id", RenderLabel::kDontCare); } - using benchmark::Fixture::SetUp; - void SetUp(const ::benchmark::State&) { depth_cameras_.clear(); } + void SetUp(::benchmark::State&) { depth_cameras_.clear(); } template // NOLINTNEXTLINE(runtime/references) diff --git a/multibody/benchmarking/acrobot.cc b/multibody/benchmarking/acrobot.cc index 2c812f9fc616..305a90070e0b 100644 --- a/multibody/benchmarking/acrobot.cc +++ b/multibody/benchmarking/acrobot.cc @@ -49,7 +49,6 @@ class FixtureBase : public benchmark::Fixture { template class AcrobotFixture : public FixtureBase { public: - using benchmark::Fixture::SetUp; void SetUp(benchmark::State&) override { plant_ = std::make_unique>(); this->Populate(*plant_); @@ -81,7 +80,6 @@ BENCHMARK_F(AcrobotFixtureAdx, AcrobotAdxMassMatrix)(benchmark::State& state) { template class MultibodyFixture : public FixtureBase { public: - using benchmark::Fixture::SetUp; void SetUp(benchmark::State&) override { auto double_plant = multibody::benchmarks::acrobot::MakeAcrobotPlant( multibody::benchmarks::acrobot::AcrobotParameters(), true); diff --git a/multibody/benchmarking/cassie.cc b/multibody/benchmarking/cassie.cc index a90db12f4d97..f00434a40d18 100644 --- a/multibody/benchmarking/cassie.cc +++ b/multibody/benchmarking/cassie.cc @@ -22,9 +22,6 @@ using systems::ContinuousState; using systems::FixedInputPortValue; using systems::System; -// We use this alias to silence cpplint barking at mutable references. -using BenchmarkStateRef = benchmark::State&; - // In the benchmark case instantiations at the bottom of this file, we'll use // a bitmask for the case's "Arg" to denote which quantities are in scope as // either gradients (for T=AutoDiffXd) or variables (for T=Expression). @@ -44,11 +41,7 @@ class Cassie : public benchmark::Fixture { tools::performance::AddMinMaxStatistics(this); } - // This apparently futile using statement works around "overloaded virtual" - // errors in g++. All of this is a consequence of the weird deprecation of - // const-ref State versions of SetUp() and TearDown() in benchmark.h. - using benchmark::Fixture::SetUp; - void SetUp(BenchmarkStateRef state) override { + void SetUp(benchmark::State& state) override { SetUpNonZeroState(); SetUpGradientsOrVariables(state); tools::performance::TareMemoryManager(); @@ -85,7 +78,8 @@ class Cassie : public benchmark::Fixture { // For T=double, any request for gradients is an error. // For T=AutoDiffXd, sets the specified gradients to the identity matrix. // For T=Expression, sets the specified quantities to symbolic variables. - void SetUpGradientsOrVariables(BenchmarkStateRef state); + // NOLINTNEXTLINE(runtime/references) + void SetUpGradientsOrVariables(benchmark::State& state); // Use these functions to invalidate input- or state-dependent computations // each benchmarked step. Disabling the cache entirely would affect the @@ -103,7 +97,8 @@ class Cassie : public benchmark::Fixture { } // Runs the MassMatrix benchmark. - void DoMassMatrix(BenchmarkStateRef state) { + // NOLINTNEXTLINE(runtime/references) + void DoMassMatrix(benchmark::State& state) { DRAKE_DEMAND(want_grad_vdot(state) == false); DRAKE_DEMAND(want_grad_u(state) == false); for (auto _ : state) { @@ -113,7 +108,8 @@ class Cassie : public benchmark::Fixture { } // Runs the InverseDynamics benchmark. - void DoInverseDynamics(BenchmarkStateRef state) { + // NOLINTNEXTLINE(runtime/references) + void DoInverseDynamics(benchmark::State& state) { DRAKE_DEMAND(want_grad_u(state) == false); for (auto _ : state) { InvalidateState(); @@ -122,7 +118,8 @@ class Cassie : public benchmark::Fixture { } // Runs the ForwardDynamics benchmark. - void DoForwardDynamics(BenchmarkStateRef state) { + // NOLINTNEXTLINE(runtime/references) + void DoForwardDynamics(benchmark::State& state) { DRAKE_DEMAND(want_grad_vdot(state) == false); for (auto _ : state) { InvalidateInput(); @@ -195,16 +192,16 @@ void Cassie::SetUpNonZeroState() { mass_matrix_out_ = MatrixX::Zero(nv_, nv_); } -template <> -void Cassie::SetUpGradientsOrVariables(BenchmarkStateRef state) { +template <> // NOLINTNEXTLINE(runtime/references) +void Cassie::SetUpGradientsOrVariables(benchmark::State& state) { DRAKE_DEMAND(want_grad_q(state) == false); DRAKE_DEMAND(want_grad_v(state) == false); DRAKE_DEMAND(want_grad_vdot(state) == false); DRAKE_DEMAND(want_grad_u(state) == false); } -template <> -void Cassie::SetUpGradientsOrVariables(BenchmarkStateRef state) { +template <> // NOLINTNEXTLINE(runtime/references) +void Cassie::SetUpGradientsOrVariables(benchmark::State& state) { // For the quantities destined for InitializeAutoDiff, read their default // values (without any gradients). For the others, leave the matrix empty. VectorX q, v, vdot, u; @@ -241,8 +238,8 @@ void Cassie::SetUpGradientsOrVariables(BenchmarkStateRef state) { } } -template <> -void Cassie::SetUpGradientsOrVariables(BenchmarkStateRef state) { +template <> // NOLINTNEXTLINE(runtime/references) +void Cassie::SetUpGradientsOrVariables(benchmark::State& state) { if (want_grad_q(state)) { const VectorX q = MakeVectorVariable(nq_, "q"); plant_->SetPositions(context_.get(), q); @@ -269,28 +266,32 @@ void Cassie::SetUpGradientsOrVariables(BenchmarkStateRef state) { // // For T=Expression, the range arg sets which variables to use, using a bitmask. -BENCHMARK_DEFINE_F(CassieDouble, MassMatrix)(BenchmarkStateRef state) { +// NOLINTNEXTLINE(runtime/references) +BENCHMARK_DEFINE_F(CassieDouble, MassMatrix)(benchmark::State& state) { DoMassMatrix(state); } BENCHMARK_REGISTER_F(CassieDouble, MassMatrix) ->Unit(benchmark::kMicrosecond) ->Arg(kWantNoGrad); -BENCHMARK_DEFINE_F(CassieDouble, InverseDynamics)(BenchmarkStateRef state) { +// NOLINTNEXTLINE(runtime/references) +BENCHMARK_DEFINE_F(CassieDouble, InverseDynamics)(benchmark::State& state) { DoInverseDynamics(state); } BENCHMARK_REGISTER_F(CassieDouble, InverseDynamics) ->Unit(benchmark::kMicrosecond) ->Arg(kWantNoGrad); -BENCHMARK_DEFINE_F(CassieDouble, ForwardDynamics)(BenchmarkStateRef state) { +// NOLINTNEXTLINE(runtime/references) +BENCHMARK_DEFINE_F(CassieDouble, ForwardDynamics)(benchmark::State& state) { DoForwardDynamics(state); } BENCHMARK_REGISTER_F(CassieDouble, ForwardDynamics) ->Unit(benchmark::kMicrosecond) ->Arg(kWantNoGrad); -BENCHMARK_DEFINE_F(CassieAutoDiff, MassMatrix)(BenchmarkStateRef state) { +// NOLINTNEXTLINE(runtime/references) +BENCHMARK_DEFINE_F(CassieAutoDiff, MassMatrix)(benchmark::State& state) { DoMassMatrix(state); } BENCHMARK_REGISTER_F(CassieAutoDiff, MassMatrix) @@ -300,7 +301,8 @@ BENCHMARK_REGISTER_F(CassieAutoDiff, MassMatrix) ->Arg(kWantGradV) ->Arg(kWantGradX); -BENCHMARK_DEFINE_F(CassieAutoDiff, InverseDynamics)(BenchmarkStateRef state) { +// NOLINTNEXTLINE(runtime/references) +BENCHMARK_DEFINE_F(CassieAutoDiff, InverseDynamics)(benchmark::State& state) { DoInverseDynamics(state); } BENCHMARK_REGISTER_F(CassieAutoDiff, InverseDynamics) @@ -314,7 +316,8 @@ BENCHMARK_REGISTER_F(CassieAutoDiff, InverseDynamics) ->Arg(kWantGradV|kWantGradVdot) ->Arg(kWantGradX|kWantGradVdot); -BENCHMARK_DEFINE_F(CassieAutoDiff, ForwardDynamics)(BenchmarkStateRef state) { +// NOLINTNEXTLINE(runtime/references) +BENCHMARK_DEFINE_F(CassieAutoDiff, ForwardDynamics)(benchmark::State& state) { DoForwardDynamics(state); } BENCHMARK_REGISTER_F(CassieAutoDiff, ForwardDynamics) @@ -328,7 +331,8 @@ BENCHMARK_REGISTER_F(CassieAutoDiff, ForwardDynamics) ->Arg(kWantGradV|kWantGradU) ->Arg(kWantGradX|kWantGradU); -BENCHMARK_DEFINE_F(CassieExpression, MassMatrix)(BenchmarkStateRef state) { +// NOLINTNEXTLINE(runtime/references) +BENCHMARK_DEFINE_F(CassieExpression, MassMatrix)(benchmark::State& state) { DoMassMatrix(state); } BENCHMARK_REGISTER_F(CassieExpression, MassMatrix) @@ -338,7 +342,8 @@ BENCHMARK_REGISTER_F(CassieExpression, MassMatrix) ->Arg(kWantGradV) ->Arg(kWantGradX); -BENCHMARK_DEFINE_F(CassieExpression, InverseDynamics)(BenchmarkStateRef state) { +// NOLINTNEXTLINE(runtime/references) +BENCHMARK_DEFINE_F(CassieExpression, InverseDynamics)(benchmark::State& state) { DoInverseDynamics(state); } BENCHMARK_REGISTER_F(CassieExpression, InverseDynamics) @@ -352,7 +357,8 @@ BENCHMARK_REGISTER_F(CassieExpression, InverseDynamics) ->Arg(kWantGradV|kWantGradVdot) ->Arg(kWantGradX|kWantGradVdot); -BENCHMARK_DEFINE_F(CassieExpression, ForwardDynamics)(BenchmarkStateRef state) { +// NOLINTNEXTLINE(runtime/references) +BENCHMARK_DEFINE_F(CassieExpression, ForwardDynamics)(benchmark::State& state) { DoForwardDynamics(state); } BENCHMARK_REGISTER_F(CassieExpression, ForwardDynamics) diff --git a/multibody/benchmarking/position_constraint.cc b/multibody/benchmarking/position_constraint.cc index d4ba1320e8dd..c8e7e91ee904 100644 --- a/multibody/benchmarking/position_constraint.cc +++ b/multibody/benchmarking/position_constraint.cc @@ -23,8 +23,7 @@ using systems::Context; class IiwaPositionConstraintFixture : public benchmark::Fixture { public: - using benchmark::Fixture::SetUp; - void SetUp(const ::benchmark::State&) override { + void SetUp(::benchmark::State&) override { tools::performance::AddMinMaxStatistics(this); const int kNumIiwas = 10; diff --git a/systems/benchmarking/framework_benchmarks.cc b/systems/benchmarking/framework_benchmarks.cc index 023f847c8dab..a351ffdf9806 100644 --- a/systems/benchmarking/framework_benchmarks.cc +++ b/systems/benchmarking/framework_benchmarks.cc @@ -15,10 +15,6 @@ class BasicFixture : public benchmark::Fixture { public: BasicFixture() { tools::performance::AddMinMaxStatistics(this); } - // This apparently futile using statement works around "overloaded virtual" - // errors in g++. All of this is a consequence of the weird deprecation of - // const-ref State versions of SetUp() and TearDown() in benchmark.h. - using benchmark::Fixture::SetUp; void SetUp(benchmark::State&) override { builder_ = std::make_unique>(); } diff --git a/systems/benchmarking/multilayer_perceptron_benchmark.cc b/systems/benchmarking/multilayer_perceptron_benchmark.cc index 39286bdcf602..dfb8b2df86e5 100644 --- a/systems/benchmarking/multilayer_perceptron_benchmark.cc +++ b/systems/benchmarking/multilayer_perceptron_benchmark.cc @@ -24,8 +24,7 @@ class Mlp : public benchmark::Fixture { this->Unit(benchmark::kMicrosecond); } - using benchmark::Fixture::SetUp; - void SetUp(const benchmark::State& state) { + void SetUp(benchmark::State& state) { // NOLINT(runtime/references) // Number of inputs. const int num_inputs = state.range(0); DRAKE_DEMAND(num_inputs >= 1); diff --git a/tools/workspace/googlebenchmark/patches/remove_overloaded_fixture_set_up.patch b/tools/workspace/googlebenchmark/patches/remove_overloaded_fixture_set_up.patch new file mode 100644 index 000000000000..3967454a0e34 --- /dev/null +++ b/tools/workspace/googlebenchmark/patches/remove_overloaded_fixture_set_up.patch @@ -0,0 +1,21 @@ +Avoid GCC warnings about overloaded virtuals + +We only need the mutable spelling for Drake code. + +--- include/benchmark/benchmark.h ++++ include/benchmark/benchmark.h +@@ -1425,12 +1425,8 @@ + this->TearDown(st); + } + +- // These will be deprecated ... +- virtual void SetUp(const State&) {} +- virtual void TearDown(const State&) {} +- // ... In favor of these. +- virtual void SetUp(State& st) { SetUp(const_cast(st)); } +- virtual void TearDown(State& st) { TearDown(const_cast(st)); } ++ virtual void SetUp(State&) {} ++ virtual void TearDown(State&) {} + + protected: + virtual void BenchmarkCase(State&) = 0; diff --git a/tools/workspace/googlebenchmark/repository.bzl b/tools/workspace/googlebenchmark/repository.bzl index 33e8d9c362d3..a22c575998c0 100644 --- a/tools/workspace/googlebenchmark/repository.bzl +++ b/tools/workspace/googlebenchmark/repository.bzl @@ -11,6 +11,7 @@ def googlebenchmark_repository( mirrors = mirrors, patches = [ ":patches/console_allocs.patch", + ":patches/remove_overloaded_fixture_set_up.patch", ":patches/string_precision.patch", ], )