1132966 - TSan: data race gfx/thebes/gfxASurface.cpp:633 GetType / RecordMemoryUsedForSurfaceType

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Nathan Froyd [:froydnj]]

Attachment: gfx-memory-race.txt

[cribbing from decoder's script, hopefully he won't mind]

The attached logfile shows a thread/data race detected by TSan (ThreadSanitizer).

Typically, races reported by TSan are not false positives, but it is possible that the race is benign. Even in this case though, we should try to come up with a fix unless this would cause unacceptable performance issues. Also note that seemingly benign races can possibly be harmful (also depending on the compiler and the architecture) [1].

If the bug cannot be fixed, then this bug should be used to either make a compile-time annotation for blacklisting or add an entry to the runtime blacklist.

The stack from TSan says GetType, but the source code line pretty clearly comes from RecordMemoryUsedForSurfaceType.  It looks like we can call RecordMemoryUsedForSurfaceType from the main thread and the compositor thread, and both can try to update the same locations.  Given that this is not critical code, it's reasonable to use relaxed Atomics here, I think.

[1] http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong

Comment 1

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #0)
> Given that this is not critical code, it's reasonable to use relaxed Atomics here, I think.

...except that we don't support Atomic<int64_t> on 32-bit platforms, bleh...

Comment 2

[Nicholas Nethercote [inactive]]

Using size_t instead of int64_t should be fine. I'm a little surprised it wasn't already size_t.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: use relaxed Atomic integers for tracking graphics surface memory usage

It looks like Atomics.h has grown support for 64-bit integers on 32-bit
platforms, so I don't think we have to worry about the restriction to size_t
anymore, unless try says differently...

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 8642563 [details] [diff] [review]
use relaxed Atomic integers for tracking graphics surface memory usage

Review of attachment 8642563 [details] [diff] [review]:
-----------------------------------------------------------------

Interestingly enough I've been wondering whether it's possible to remove these counters, and instead do traversal-based measurement. But that's not going to happen for a while, if it ever does.

::: gfx/thebes/gfxASurface.cpp
@@ +575,5 @@
> +// assure dynamic race checkers like TSan that this is OK, we use
> +// relaxed memory ordering here.  We do have to be careful to not
> +// perform read-write-modify memory operations with these values, as
> +// such operations would still require some sort of barrier.
> +static Atomic<int64_t, Relaxed> gSurfaceMemoryUsed[size_t(gfxSurfaceType::Max)];

Can you move this inside SurfaceMemoryReporter and use getters/setters to ensure this requirement is met?

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5399666bb0ca

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5399666bb0ca