search

google / benchmark

A microbenchmark support library
Apache License 2.0
8.61k stars 1.57k forks source link

State: Initialize counters with kAvgIteration in constructor #1652

Closed jmr closed 10 months ago

jmr commented 10 months ago

Previously, counters was updated in PauseTiming() with counters[name] += Counter(measurement, kAvgIteration).

The first counters[name] call inserts a counter with no flags.

There is no operator+= for Counter, so the insertion is done by converting the Counter to a double, then constructing a Counter to insert from the double, which drops the flags.

Pre-insert the Counter with the correct flags, then only update Counter::value.

Introduced in https://github.com/google/benchmark/commit/1c64a36c5b8ee75d462b3fe7a9d020c66a2a1094 ([perf-counters] Fix pause/resume (https://github.com/google/benchmark/pull/1643)).

jmr commented 10 months ago

@mtrofin

jmr commented 10 months ago

I'm not sure why most of the sanitizer tests are failing.

The history on my branch is getting out of control. This should be squashed-and-merged when it's ready.

dmah42 commented 10 months ago

I'm not sure why most of the sanitizer tests are failing.

they're passing at head, i'm not sure why your change would break them.. only thing i can think (as it's coming from __assert) is that you're referencing assert without including but that seems like an unlikely issue...

The history on my branch is getting out of control. This should be squashed-and-merged when it's ready.

it will be.. always do the squash when i merge :)

jmr commented 10 months ago

Can you trigger the presubmits on HEAD? I wonder if something changed with the sanitizer setup.

dmah42 commented 10 months ago

Can you trigger the presubmits on HEAD? I wonder if something changed with the sanitizer setup.

yeah they're failing for changes to documents

mtrofin commented 10 months ago

Thanks for fixing this. I forgot how Counter worked and assumed they are collected "in total" by design. Thanks!