Remove ANNOTATE_BENIGN_RACE from trace_event_synthetic_delay.cc

Remove ANNOTATE_BENIGN_RACE from trace_event_synthetic_delay.cc

There seem to be no data races behind these annotations, upon removing
them the trybots remain green, and it's unclear from the code whether a
race is possible.

BUG=chromium:702558
R=skyostil@chromium.org

[Alexander Potapenko, 2017-03-20 14:05:04]

Description was changed from

==========
Remove ANNOTATE_BENIGN_RACE from trace_event_synthetic_delay.cc

BUG=chromium:702558
==========

to

==========
Remove ANNOTATE_BENIGN_RACE from trace_event_synthetic_delay.cc

There seems to be no data races behind these annotations, upon removing
them the trybots remain green, and it's unclear from the code whether a
race is possible.

BUG=chromium:702558
R=skyostil@chromium.org
==========

[Alexander Potapenko, 2017-03-20 14:05:04]

glider@chromium.org changed reviewers:
+ primiano@chromium.org, skyostil@chromium.org

[Alexander Potapenko, 2017-03-20 14:07:23]

Description was changed from

==========
Remove ANNOTATE_BENIGN_RACE from trace_event_synthetic_delay.cc

There seems to be no data races behind these annotations, upon removing
them the trybots remain green, and it's unclear from the code whether a
race is possible.

BUG=chromium:702558
R=skyostil@chromium.org
==========

to

==========
Remove ANNOTATE_BENIGN_RACE from trace_event_synthetic_delay.cc

There seem to be no data races behind these annotations, upon removing
them the trybots remain green, and it's unclear from the code whether a
race is possible.

BUG=chromium:702558
R=skyostil@chromium.org
==========

[Sami, 2017-03-21 11:52:55]

https://codereview.chromium.org/2762643002/diff/20001/base/trace_event/trace_...
File base/trace_event/trace_event_synthetic_delay.cc (left):

https://codereview.chromium.org/2762643002/diff/20001/base/trace_event/trace_...
base/trace_event/trace_event_synthetic_delay.cc:84:
ANNOTATE_BENIGN_RACE(&target_duration_, "Synthetic delay duration");
There is actually a race here (SetTargetDuration writes to target_duration_ and
we read it without locking), but we just don't have enough code using synthetic
delays to trigger it. Actually I'm considering deleting the entire synthetic
delay subsystem since we haven't really made use of it recently. If you want to
remove the macros before that that's fine, but please leave a comment here
saying there is an actual race.

[Alexander Potapenko, 2017-03-21 12:09:25]

On 2017/03/21 11:52:55, Sami wrote:
>
https://codereview.chromium.org/2762643002/diff/20001/base/trace_event/trace_...
> File base/trace_event/trace_event_synthetic_delay.cc (left):
> 
>
https://codereview.chromium.org/2762643002/diff/20001/base/trace_event/trace_...
> base/trace_event/trace_event_synthetic_delay.cc:84:
> ANNOTATE_BENIGN_RACE(&target_duration_, "Synthetic delay duration");
> There is actually a race here (SetTargetDuration writes to target_duration_
and
> we read it without locking), but we just don't have enough code using
synthetic
> delays to trigger it.
Yeah, it didn't fire during trybot runs, which is a pity((
Do you know if the race is actually happening in prod?

> Actually I'm considering deleting the entire synthetic
> delay subsystem since we haven't really made use of it recently. If you want
to
> remove the macros before that that's fine, but please leave a comment here
> saying there is an actual race.
I'm fine with keeping it for now if you're going to delete the code anyway.

[Sami, 2017-03-21 12:12:27]

On 2017/03/21 12:09:25, Alexander Potapenko wrote:
> On 2017/03/21 11:52:55, Sami wrote:
> >
>
https://codereview.chromium.org/2762643002/diff/20001/base/trace_event/trace_...
> > File base/trace_event/trace_event_synthetic_delay.cc (left):
> > 
> >
>
https://codereview.chromium.org/2762643002/diff/20001/base/trace_event/trace_...
> > base/trace_event/trace_event_synthetic_delay.cc:84:
> > ANNOTATE_BENIGN_RACE(&target_duration_, "Synthetic delay duration");
> > There is actually a race here (SetTargetDuration writes to target_duration_
> and
> > we read it without locking), but we just don't have enough code using
> synthetic
> > delays to trigger it.
> Yeah, it didn't fire during trybot runs, which is a pity((
> Do you know if the race is actually happening in prod?

Not in practice because this API is only used on the perf waterfall.

> > Actually I'm considering deleting the entire synthetic
> > delay subsystem since we haven't really made use of it recently. If you want
> to
> > remove the macros before that that's fine, but please leave a comment here
> > saying there is an actual race.
> I'm fine with keeping it for now if you're going to delete the code anyway.

Works for me. Filed crbug.com/703604 to track the deletion.