google / benchmark

A microbenchmark support library
Apache License 2.0
8.69k stars 1.59k forks source link

[BUG] No indication in error of how to fix "DoNotOptimize<long long>' is deprecated: The const-ref version ..." #1580

Open monkeynova opened 1 year ago

monkeynova commented 1 year ago

Describe the bug I recently updated the version of this repository used by my bazel(isk) WORKSPACE and I started encountering the deprecation warning about the const-ref form of DoNotOptimize. While I appreciate the warning and want to do better, it's unclear to me what I need to do differently to make it go away.

System Which OS, compiler, and compiler version are you using:

To reproduce Steps to reproduce the behavior:

  1. sync to commit d29044d5da4b32e449073a365ad6524fb9bdf884
  2. bazelisk test :empty_test
  3. See error: "problems/open/empty_test.cc:19:16: warning: 'DoNotOptimize' is deprecated: The const-ref version of this method can permit undesired compiler optimizations in benchmarks [-Wdeprecated-declarations]"

Expected behavior I would like a description (or link to documentation) about what I need to change in that program to be getting ahead of the deprecation.

Additional context BUILD

load("@rules_cc//cc:defs.bzl", "cc_test")
load("//euler/computer_science:data_file.bzl", "data_cc_file")

cc_test(
    name = "empty_test",
    timeout = "short",
    srcs = ["empty_test.cc"],
    tags = ["benchmark"],
    deps = [
        "//euler/computer_science:vlog",
        "@com_google_absl//absl/log",
        "@com_google_absl//absl/log:check",
        "@com_google_googletest//:gtest",
        "@com_monkeynova_gunit_main//:test_main",
    ],
)

empty_test.cc

#include "absl/log/check.h"
#include "absl/log/log.h"
#include "benchmark/benchmark.h"
#include "euler/computer_science/vlog.h"
#include "gtest/gtest.h"

int64_t RunProblem(int64_t range) {
  int64_t ret = -1;
  VLOG(1) << "RunProblem(" << range << ") = " << ret;
  return ret;
}

TEST(Euler, Given) { EXPECT_EQ(RunProblem(-1), -1); }

TEST(Euler, Problem) { EXPECT_EQ(RunProblem(-1), -1); }

void BM_ProblemXXX(benchmark::State& state) {
  for (auto _ : state) {
    benchmark::DoNotOptimize( RunProblem(state.range(0)));
  }
}

BENCHMARK(BM_ProblemXXX)->Arg(-1);
dmah42 commented 1 year ago

store state.range(0) in a local variable and call DoNotOptimize on that. eg https://github.com/google/benchmark/blob/main/test/donotoptimize_test.cc

monkeynova commented 1 year ago

I tried that and still get the same warning. (And since the function overload comes from deduction based on the return of RunProblem, I wasn't sure how it could change things)

That is, transforming my code to the following, still gives me the deprecation warning.

void BM_ProblemXXX(benchmark::State& state) {
  int64_t range = state.range(0);
  for (auto _ : state) {
    benchmark::DoNotOptimize(RunProblem(range));
  }
}
LebedevRI commented 1 year ago

I tried that and still get the same warning. (And since the function overload comes from deduction based on the return of RunProblem, I wasn't sure how it could change things)

That is, transforming my code to the following, still gives me the deprecation warning.

void BM_ProblemXXX(benchmark::State& state) {
  int64_t range = state.range(0);
  for (auto _ : state) {
    benchmark::DoNotOptimize(RunProblem(range));
  }
}

@dmah42 meant to say

void BM_ProblemXXX(benchmark::State& state) {
  for (auto _ : state) {
    int64_t result = RunProblem(state.range(0));
    benchmark::DoNotOptimize(result);
  }
}

TBH, this deprecation looks rather bogus to me.

dmah42 commented 1 year ago

I tried that and still get the same warning. (And since the function overload comes from deduction based on the return of RunProblem, I wasn't sure how it could change things) That is, transforming my code to the following, still gives me the deprecation warning.

void BM_ProblemXXX(benchmark::State& state) {
  int64_t range = state.range(0);
  for (auto _ : state) {
    benchmark::DoNotOptimize(RunProblem(range));
  }
}

@dmah42 meant to say

void BM_ProblemXXX(benchmark::State& state) {
  for (auto _ : state) {
    int64_t result = RunProblem(state.range(0));
    benchmark::DoNotOptimize(result);
  }
}

TBH, this deprecation looks rather bogus to me.

you should probably pick that back up on the original issue. i thought the issues raised were fairly compelling (it doesn't do what we expect it to do).

monkeynova commented 1 year ago

That successfully makes the warning go away (yay?). This feels a lot cruftier than the previous code, but it can get where I want to go.

As far as the original bug, I suspect there are some good reasons for the change. I'm mostly looking for good documentation about how not to run into it (getting a human to review my code on github seems less scalable than I might like) and am still hoping that there could be some doc about what this error means, why the deprecation is needed, and how to move my code forward.