1963506 - TSan data race on PerfStats::sCollectionMask

Status: RESOLVED FIXED

(Core :: Performance Engineering, defect)

Description

[James Teh [:Jamie]]

While working on an initial framework for accessibility engine performance tests in bug 1962317, I hit a data race in PerfStats in TSan builds. This seems to be caused by my test calling PerfStats::SetCollectionMask on the main thread, while the GPU thread happens to be calling PerfStats::RecordMeasurement. This causes a race on PerfStats::sCollectionMask.

It's probably unusual to run a perf test with TSan anyway, so the solution for my case is simple: disable the tests for TSan. However, this isn't obvious to a newcomer.

There are a few paths forward:

1. Leave this as is but document it somewhere.
2. Use Atomic<uint64_t, Relaxed> for sCollectionMask.
3. Suppress this error using no_sanitize("thread") as documented here.

Discussion from Matrix:

[Bas] That race is gauged to be acceptable and worth the performance advantage of fencing the read.
It is known but we could do a better job documenting it. The race should be benign.
[mstange] The right way to express to the compiler that it's fine for an integer variable to be accessed from multiple threads is to make it an atomic
And you can make it an atomic with Relaxed ordering to say that you don't care about ordering with anything else
Racy reads and writes of a non atomic integer are undefined behavior at the C++ semantics level and we definitely don't want that

Comment 1

[Paul Adenot (:padenot)]

2 is the only correct solution, the two others are simply UB as Markus say.

https://web.archive.org/web/20141225111616/http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong goes a bit deeper and explain why it is the only correct solution.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:bas.schouten, could you have a look please?

For more information, please visit BugBot documentation.

Comment 3

[Bas Schouten (:bas.schouten)]

I don't think any of the concerns in the cited article apply here, other than the theoretical undefined behavior one. Since we compile with C++11 or higher I think that's a fair reason to do it, even if it makes the code a little uglier. I will put up a patch.

Comment 4

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1963506: Mark PerfStats::sCollectionMask as a relaxed ordering atomic. r=mstange

Comment 5

[Bas Schouten (:bas.schouten)]

I confirmed there's no difference in the generated code on either ARM or x64 as far as I can tell. So that does seem to make this the better option.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b43bcc44e94b
Mark PerfStats::sCollectionMask as a relaxed ordering atomic. r=mstange

Comment 7

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/b43bcc44e94b