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

RESOLVED FIXED in Firefox 58

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: timhuang, Assigned: timhuang)

Tracking

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58+ fixed, firefox59 fixed)

Details

Attachments

(2 attachments)

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 hidden (mozreview-request)
(Assignee)

Comment 2

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

Comment 3

a year ago
mozreview-review
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+.
Attachment #8928901 - Flags: review?(amarchesini) → review+
Attachment #8928901 - Flags: review?(nfroyd)
(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 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.
Attachment #8928901 - Flags: review?(nfroyd) → review?(n.nethercote)

Comment 6

a year ago
mozreview-review
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
Attachment #8928901 - Flags: review?(n.nethercote)
Comment hidden (mozreview-request)

Comment 8

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

https://reviewboard.mozilla.org/r/200218/#review206240
Attachment #8928901 - Flags: review?(n.nethercote) → review+

Comment 10

a year ago
Pushed by toros@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2b374242d870
Add support of Atmoic<> for Preferences::Add*VarCache(). r=baku,njn
Keywords: checkin-needed

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2b374242d870
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
[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.
Blocks: 1424341
status-firefox58: --- → affected
tracking-firefox58: --- → ?
Flags: needinfo?(rkothari)
Gerry is managing the 58 release.
Flags: needinfo?(rkothari) → needinfo?(gchang)
Track 58+ as this is required by bug 1424341.
tracking-firefox58: ? → +
Flags: needinfo?(gchang)
Hi Tom, since this is required by bug 1424341, please nominate beta uplift request.
Flags: needinfo?(tom)
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
Flags: needinfo?(tom)
Attachment #8941558 - Flags: approval-mozilla-beta?
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+.
Attachment #8941558 - Flags: approval-mozilla-release+
Attachment #8941558 - Flags: approval-mozilla-beta?
Attachment #8941558 - Flags: approval-mozilla-beta+
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-beta/rev/574c337510b1
status-firefox58: affected → fixed
Keywords: checkin-needed
Whiteboard: https://hg.mozilla.org/releases/mozilla-beta/rev/574c337510b15521c16142130a2d1fee407ac25b(FIREFOX_58b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-beta/rev/574c337510b15521c16142130a2d1fee407ac25b (FIREFOX_58b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/3ebefc043ac67bc941f8e74182d3ba8fdc9419f4
Whiteboard: https://hg.mozilla.org/releases/mozilla-beta/rev/574c337510b15521c16142130a2d1fee407ac25b(FIREFOX_58b_RELBRANCH)
You need to log in before you can comment on or make changes to this bug.