1417741 - Add support of Atomic<> for Preferences::Add*VarCache

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Tim Huang[:timhuang]]

It would be nice if we can use Atomic<> for Preferences::Add*VarCache. There is an existing one, Preferences::AddAtomicUintVarCache(), we can add for others.

Comment 1

[Tim Huang[:timhuang]]

Attachment: Bug 1417741 - Add support of Atmoic<> for Preferences::Add*VarCache().

Review commit: https://reviewboard.mozilla.org/r/200218/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200218/

Comment 2

[Tim Huang[:timhuang]]

We cannot add for float since Atomic<> doesn't support float yet.

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8928901 [details]
Bug 1417741 - Add support of Atmoic<> for Preferences::Add*VarCache().

https://reviewboard.mozilla.org/r/200218/#review205390

I really really like this patch. I think it's extremely useful and I'll definitely use it on workers.
I would write a couple of lines to dev-platform about it if/when froydnj gives his r+.

Comment 4

[Ethan Tseng [:ethan]]

(In reply to Andrea Marchesini [:baku] from comment #3)
> I really really like this patch. I think it's extremely useful and I'll
> definitely use it on workers.
> I would write a couple of lines to dev-platform about it if/when froydnj
> gives his r+.

Nice!  :)

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8928901 [details]
Bug 1417741 - Add support of Atmoic<> for Preferences::Add*VarCache().

I am a little leery about using this, because the *safe* thing--an Atomic<T> cached variable that uses sequentially consistent memory ordering--isn't possible with this patch.  Trying that will result in peculiar link errors.  OTOH, we already use relaxed atomics in the preferences API, so maybe this is not so bad?  Punting review to njn.

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8928901 [details]
Bug 1417741 - Add support of Atmoic<> for Preferences::Add*VarCache().

https://reviewboard.mozilla.org/r/200218/#review205804

Please add new variants of MediaPrefs::PrefAddVarCache() and gfxPrefs::PrefAddVarCache() too. (Yes, the duplication is awful, I plan to get rid of it soon.)

::: modules/libpref/Preferences.cpp:4913
(Diff revision 1)
> +
> +template<MemoryOrdering Order>
> +/* static */ nsresult
> +Preferences::AddAtomicBoolVarCache(Atomic<bool, Order>* aCache, const char* aPref, bool aDefault)
> +{
> +  WATCHING_PREF_RAII();

Bug 1417806 just changed WATCHING_PREF_RAII(). You'll need to update the patch accordingly.

::: modules/libpref/Preferences.cpp:4969
(Diff revision 1)
> +/* static */ nsresult
> +Preferences::AddAtomicIntVarCache(Atomic<int32_t, Order>* aCache,
> +                                  const char* aPref,
> +                                  int32_t aDefault)
> +{
> +  WATCHING_PREF_RAII();

ditto

Comment 7

[Tim Huang[:timhuang]]

Comment on attachment 8928901 [details]
Bug 1417741 - Add support of Atmoic<> for Preferences::Add*VarCache().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200218/diff/1-2/

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8928901 [details]
Bug 1417741 - Add support of Atmoic<> for Preferences::Add*VarCache().

https://reviewboard.mozilla.org/r/200218/#review206240

Comment 9

[Tim Huang[:timhuang]]

Try looks good.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b4540a5fa2e

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b374242d870
Add support of Atmoic<> for Preferences::Add*VarCache(). r=baku,njn

Comment 11

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/2b374242d870

Comment 12

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

[Tracking Requested - why for this release]: This is required for Bug 1424341 to land. However, we don't need to take the entire patch, we can take only the part that adds AddAtomicBoolVarCache because that is all we need.

I will attach the partial backport.

Comment 13

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Beta: This is a partial backport of that only adds AddAtomicBoolVarCache because that is all we need

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Gerry is managing the 58 release.

Comment 15

[Gerry Chang [:gchang]]

Track 58+ as this is required by bug 1424341.

Comment 16

[Gerry Chang [:gchang]]

Hi Tom, since this is required by bug 1424341, please nominate beta uplift request.

Comment 17

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 8941558 [details] [diff] [review]
Beta: This is a partial backport of that only adds AddAtomicBoolVarCache because that is all we need

Approval Request Comment
[Feature/Bug causing the regression]: We would like the ability to easily tune timer precision on beta for the next release cycle to respond to Spectre. The pref is turned off by default right now.

[User impact if declined]: We will have to do dot releases instead of doing a system addon to tweak a pref. Also, the dot releases will be more work (much more work if we don't uplift this in one of them) to cover all timers - previously our chemspill was only to address performance.now()

[Is this code covered by automated tests?]: Yes.

[Has the fix been verified in Nightly?]: The full patch has been, the partial backport I wrote has not.

[Needs manual test from QE? If yes, steps to reproduce]: No

[List of other uplifts needed for the feature/fix]: This is a dependency for 1424341

[Is the change risky?]: No

[Why is the change risky/not risky?]: This patch only adds functions it doesn't add anything that calls those functions.

[String changes made/needed]: None

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8941558 [details] [diff] [review]
Beta: This is a partial backport of that only adds AddAtomicBoolVarCache because that is all we need

Required by bug 1424341. Beta58+.

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/574c337510b1

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/574c337510b15521c16142130a2d1fee407ac25b (FIREFOX_58b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/3ebefc043ac67bc941f8e74182d3ba8fdc9419f4