Closed
Bug 1465852
Opened 5 years ago
Closed 5 years ago
Assert when non-Atomic static prefs are accessed off the main thread
Categories
(Core :: Preferences: Backend, enhancement)
Core
Preferences: Backend
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(1 file, 1 obsolete file)
12.21 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
Seems like this could be a footgun.
![]() |
Assignee | |
Comment 1•5 years ago
|
||
Attachment #8982274 -
Flags: review?(n.nethercote)
![]() |
Assignee | |
Comment 2•5 years ago
|
||
Hrm. Try says that we hit that assert in various cases. I wonder what the practical upshot is of access to these prefs on background threads...
![]() |
||
Comment 3•5 years ago
|
||
In a lot of cases, it's probably just that readers might not see the writes from other threads. We might have enough memory fences running around (e.g. mutex lock/unlock) that it doesn't matter too much in practice, and I'd bet there are prefs whose owners have said "we don't care *that* much about consistency". Still seems like it'd be something good to fix.
![]() |
Assignee | |
Comment 4•5 years ago
|
||
> In a lot of cases, it's probably just that readers might not see the writes from other threads.
That seems fine if that's all it is. That said, those are also the RelaxedAtomic semantics, no?
A concrete case I'm looking at used to have a pref cache on the main thread writing to int globals and then used those globals off the main thread by just reading the global. It was converted to a non-Atomic static pref, which does seem like it preserves the old semantics; the question is whether those old semantics make sense. Could we end up seeing a partial write?
![]() |
||
Comment 5•5 years ago
|
||
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4) > > In a lot of cases, it's probably just that readers might not see the writes from other threads. > > That seems fine if that's all it is. That said, those are also the > RelaxedAtomic semantics, no? They are; the Relaxed bit just makes it more obvious to the compiler that's what you're doing, so it doesn't format your hard drive or something. The Relaxed atomics also signal to TSan etc. that you know what you're doing. > A concrete case I'm looking at used to have a pref cache on the main thread > writing to int globals and then used those globals off the main thread by > just reading the global. It was converted to a non-Atomic static pref, > which does seem like it preserves the old semantics; the question is whether > those old semantics make sense. Could we end up seeing a partial write? I think the guarantee on everything that we care about is that writes <= word size are "atomic" in the sense that you never see part of the old value and part of the new.
![]() |
Assignee | |
Comment 6•5 years ago
|
||
Hmm, ok. So is this still worth doing?
![]() |
||
Comment 7•5 years ago
|
||
I think it's worth doing; the underlying problem was filed as bug 1436916. However, we can't land it as-is if we're getting failures. Also, the lack of Atomic<float> may make life difficult. I was contemplating using Atomic<int32_t> to represent VarCache floats and then casting on get/set... I'm not sure if it's a brilliant idea or a terrible idea.
![]() |
||
Comment 8•5 years ago
|
||
Comment on attachment 8982274 [details] [diff] [review] Enforce use of Atomic for static prefs gotten off the main thread Review of attachment 8982274 [details] [diff] [review]: ----------------------------------------------------------------- r=me, once this doesn't trigger assertions in practice.
Attachment #8982274 -
Flags: review?(n.nethercote) → review+
Comment 9•5 years ago
|
||
"Fun" fact: MSVC emits static initializers for global Atomic<T>s.
![]() |
Assignee | |
Comment 10•5 years ago
|
||
Yeah, I'm going to post an updated patch once I have a green try run. Slowly getting there. ;) Mostly media bits need to become atomic... Comment 9 does not sound good for our purposes here. :(
![]() |
Assignee | |
Comment 11•5 years ago
|
||
Oh, and at the moment we have no float-typed StaticPrefs... When we want to add one that's accessed on a background thread, we will need to worry about lack of Atomic<float>.
![]() |
Assignee | |
Comment 12•5 years ago
|
||
Attachment #8982676 -
Flags: review?(n.nethercote)
![]() |
Assignee | |
Updated•5 years ago
|
Attachment #8982274 -
Attachment is obsolete: true
![]() |
||
Comment 13•5 years ago
|
||
> we will need to worry about lack of Atomic<float>. Yep, as per comment 7 I wonder if we could fake it with Atomic<int32_t>.
![]() |
||
Comment 14•5 years ago
|
||
Comment on attachment 8982676 [details] [diff] [review] Now green on try Review of attachment 8982676 [details] [diff] [review]: ----------------------------------------------------------------- I'm pleased by how simple the checking functionality is. I'm also pleased that so many pref accesses are now race-free!
Attachment #8982676 -
Flags: review?(n.nethercote) → review+
Comment 15•5 years ago
|
||
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0df4de6a931f Enforce use of Atomic for static prefs gotten off the main thread. r=njn
![]() |
||
Comment 16•5 years ago
|
||
I guess one downside here is that this change could block the conversion of float prefs to StaticPrefs, unless there is a way to carve out an exception for floats in the checking.
![]() |
Assignee | |
Comment 17•5 years ago
|
||
template<> struct IsAtomic<float> : TrueType {} should do it, I would think. Probably with a renaming from IsAtomic to IsOKOffMainThread or something... That said, if the float prefs are not accessed off-main-thread there is no problem, and if they are accessed off-main-thread then we have racy access, right?
![]() |
||
Comment 18•5 years ago
|
||
> That said, if the float prefs are not accessed off-main-thread there is no
> problem, and if they are accessed off-main-thread then we have racy access,
> right?
Yes, but unlike the non-float cases, we don't have an easy way to fix the racy-ness.
Comment 19•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0df4de6a931f
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•