1465852 - Assert when non-Atomic static prefs are accessed off the main thread

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

Seems like this could be a footgun.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Enforce use of Atomic for static prefs gotten off the main thread

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

[Nathan Froyd [:froydnj]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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

[Nathan Froyd [:froydnj]]

(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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm, ok.  So is this still worth doing?

Comment 7

[Nicholas Nethercote [inactive]]

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

[Nicholas Nethercote [inactive]]

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.

Comment 9

[Mike Hommey [:glandium]]

"Fun" fact: MSVC emits static initializers for global Atomic<T>s.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 11

[Boris Zbarsky [:bzbarsky]]

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>.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Now green on try

Comment 13

[Nicholas Nethercote [inactive]]

> 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

[Nicholas Nethercote [inactive]]

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!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

[Nicholas Nethercote [inactive]]

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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

  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

[Nicholas Nethercote [inactive]]

> 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

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/0df4de6a931f