1477436 - Preferences::Get* is not threadsafe/is main thread only

Status: NEW

(Core :: Preferences: Backend, defect, P3)

Description

[Jeff Gilbert [:jgilbert]]

Especially with the explosion in APIs available in workers, it would be very convenient (and probably safer in the long run) to allow reads of pref values off the main thread.

Comment 1

[Eric Rahm [:erahm]]

For background, this request came out of a dev-platform thread [1] about changes from bug 1471025.

[1] https://groups.google.com/d/msg/mozilla.dev.platform/sjJIIwjxzsU/RI_6zuLpBAAJ

Comment 2

[Jonathan Kew (:jfkthame)]

In the dev-platform thread, an early response said:

> We definitely can't afford the locking overhead—preference
> look-ups already show up in profiles without it.

As one of the people who would like to read prefs from another thread, I was wondering how bad this really is. It's certainly true that "preference lookups already show up in profiles", but how much difference would it make if we added a RWLock?

Here's a profile of launching the browser (local opt build from mozilla-central), sampled at 0.1ms: https://perfht.ml/2A6Ddho. Filtering this for mozilla::Preferences, we see 16.3ms spent in Preferences code in the main process. (Much less in the two content processes, which have 2.8ms and 4.3ms.)

(Repeating the measurement shows considerable variation, with the total mozilla::Preferences time in the main process varying from a low of around 15ms to a high nearer 25ms on some runs.)

As a quick-and-dirty way to get an idea of how much impact a lock might have, I then added a global RWLock and made all the Preferences::Get...() functions do an AutoReadLock -- for good measure, I also made the Set functions acquire a write lock, although I doubt much writing happens during startup(?). I didn't attempt to really make Preferences thread-safe -- that would require a lot more care! -- but I figured this should give a rough idea of the overhead involved in just acquiring/releasing a lock every time we read a preference (without any actual contention).

Here's a profile of this modified build: https://perfht.ml/2Adymeu. Again, filtering for mozilla::Preferences, we see a total of 15.5ms in the main process. That's fractionally _less_ than the unpatched build, though not significantly; I'm seeing much greater variability of both measurements than the typical difference between them. (I've picked "good" profiles in both cases; plenty of times the total was well over 20ms.)

The other thing to note about https://perfht.ml/2Adymeu is that if we filter for mozilla::RWLock, the main process shows a grand total of 0.3ms (three individual samples) -- and of those, only one is from Preferences.

So the impression I'm getting from this admittedly crude experiment is that while adding a lock would obviously have _some_ overhead, it may be small enough that it's essentially insignificant in comparison to the rest of the work Preferences does. If there's code that is so hot it can't afford such a lock, then it shouldn't be using Preferences::Get...() or similar at all; it should be caching the value it needs.

> And even the
> current limited exception that we grant Stylo while it has the
> main thread blocked causes problems (bug 1474789), since it
> makes it impossible to update statistics for those reads, or
> switch to Robin Hood hashing (which would make our hash tables
> much smaller and more efficient, but requires read operations to
> be able to move entries).

Having read a little about Robin Hood hashing, my understanding is that only _writes_ need to be able to move entries, and so this should not block us from having an RWLock on the preferences table and allowing multiple threads to _read_ without contention. Only when a write is happening will readers be blocked. Is this an incorrect interpretation?

Comment 3

[(no longer active)]

(In reply to Jonathan Kew (:jfkthame) from comment #2)
> In the dev-platform thread, an early response said:
> 
> > We definitely can't afford the locking overhead—preference
> > look-ups already show up in profiles without it.
> 
> As one of the people who would like to read prefs from another thread, I was
> wondering how bad this really is. It's certainly true that "preference
> lookups already show up in profiles", but how much difference would it make
> if we added a RWLock?

I think if you want to answer this specific question accurately, you need to prepare a set of microbenchmarks that assess the overhead of RWLock under a number of scenarios (i.e., the lock being non-contended, and it being contended in various ways.)

> Here's a profile of launching the browser (local opt build from
> mozilla-central), sampled at 0.1ms: https://perfht.ml/2A6Ddho. Filtering
> this for mozilla::Preferences, we see 16.3ms spent in Preferences code in
> the main process. (Much less in the two content processes, which have 2.8ms
> and 4.3ms.)

Just to provide some context around the problem: first, it's not only startup workload that matters here (e.g. see bug 1375287 which is purely about a Web-facing API) but last year we worked on bug 1352525 specifically to reduce the number of pref accesses on the startup path, so the current state is a bit of an outcome of that effort, and we should expect more and more pref reads to creep back into that path when we aren't looking in the future.  :-)

> (Repeating the measurement shows considerable variation, with the total
> mozilla::Preferences time in the main process varying from a low of around
> 15ms to a high nearer 25ms on some runs.)
> 
> As a quick-and-dirty way to get an idea of how much impact a lock might
> have, I then added a global RWLock and made all the Preferences::Get...()
> functions do an AutoReadLock -- for good measure, I also made the Set
> functions acquire a write lock, although I doubt much writing happens during
> startup(?). I didn't attempt to really make Preferences thread-safe -- that
> would require a lot more care! -- but I figured this should give a rough
> idea of the overhead involved in just acquiring/releasing a lock every time
> we read a preference (without any actual contention).

As I said before, please consider using a microbenchmark instead.  What you are measuring is so small in the overall time that tiny differences before/after won't be visible like this, I think.  Also I think we'd want to do the measurements on Windows, and perhaps also on Android to ensure we're covering all important platforms.

> Here's a profile of this modified build: https://perfht.ml/2Adymeu. Again,
> filtering for mozilla::Preferences, we see a total of 15.5ms in the main
> process. That's fractionally _less_ than the unpatched build, though not
> significantly; I'm seeing much greater variability of both measurements than
> the typical difference between them. (I've picked "good" profiles in both
> cases; plenty of times the total was well over 20ms.)
> 
> The other thing to note about https://perfht.ml/2Adymeu is that if we filter
> for mozilla::RWLock, the main process shows a grand total of 0.3ms (three
> individual samples) -- and of those, only one is from Preferences.
> 
> So the impression I'm getting from this admittedly crude experiment is that
> while adding a lock would obviously have _some_ overhead, it may be small
> enough that it's essentially insignificant in comparison to the rest of the
> work Preferences does. If there's code that is so hot it can't afford such a
> lock, then it shouldn't be using Preferences::Get...() or similar at all; it
> should be caching the value it needs.

In practice though, we *do* have code that shouldn't be using Preferences::Get...() at all like you suggest in the first place, so the impact of a change like this is to slow down such code slightly more than it already is.  So I think a different question to ask would be, do we consider that an acceptable trade-off?

It would be nice to know what it is that we are getting in return?  I find this assertion that most of our preferences are needed to be read on background threads quite suspicious honestly.  I think most people who would be opposed to adding a RWLock would find the trade-off poor, as they would believe what we'd get in return isn't worth the added cost.  Saying the cost is small isn't going to convince those folks over since the benefit is also not all that huge!

> > And even the
> > current limited exception that we grant Stylo while it has the
> > main thread blocked causes problems (bug 1474789), since it
> > makes it impossible to update statistics for those reads, or
> > switch to Robin Hood hashing (which would make our hash tables
> > much smaller and more efficient, but requires read operations to
> > be able to move entries).
> 
> Having read a little about Robin Hood hashing, my understanding is that only
> _writes_ need to be able to move entries, and so this should not block us
> from having an RWLock on the preferences table and allowing multiple threads
> to _read_ without contention. Only when a write is happening will readers be
> blocked. Is this an incorrect interpretation?

I don't know of a robin hood lookup algorithm that requires modifying the underlying hashtable store personally...

Comment 4

[Nicholas Nethercote [inactive]]

FWIW, I'm also skeptical about allowing off-main-thread accesses.

Furthermore, a huge change to prefs landed recently (bug 1471025) and that should be given a chance to bake for some time before we consider any other large changes.