Closed
Bug 1132966
Opened 10 years ago
Closed 10 years ago
TSan: data race gfx/thebes/gfxASurface.cpp:633 GetType / RecordMemoryUsedForSurfaceType
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
References
(Blocks 1 open bug)
Details
(Whiteboard: [tsan], gfx-noted)
Attachments
(2 files)
9.68 KB,
text/plain
|
Details | |
3.17 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
[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
![]() |
Assignee | |
Comment 1•10 years ago
|
||
(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•10 years ago
|
||
Using size_t instead of int64_t should be fine. I'm a little surprised it wasn't already size_t.
![]() |
Assignee | |
Comment 3•10 years ago
|
||
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...
Attachment #8642563 -
Flags: review?(n.nethercote)
![]() |
||
Comment 4•10 years ago
|
||
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?
Attachment #8642563 -
Flags: review?(n.nethercote) → review+
![]() |
Assignee | |
Updated•10 years ago
|
Assignee: nobody → nfroyd
Comment 6•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•