Closed Bug 1349683 Opened 3 years ago Closed 2 years ago

Use Move() when assigning from a RefPtr to a RevocableEventPtr

Categories

(Core :: XPCOM, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox55 --- wontfix
firefox56 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

Details

Attachments

(1 file)

When reviewing bug 1342863, I noticed that PresShell::ResizeReflowIgnoreOverride currently does:
> mResizeEvent = resizeEvent;
...and:
> mUpdateApproximateFrameVisibilityEvent = ev;
https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/layout/base/PresShell.cpp#2017
https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/layout/base/PresShell.cpp#6265

(The local variable will be changing name in bug 1342863, but functionally this line will still be the same even after that bug lands.)

Here, mResizeEvent is of type nsRevocableEventPtr, and "resizeEvent" (or "event") is of type RefPtr  (technically RefPtr<nsRunnableMethod<PresShell>>).

Really, we should be assigning using resizeEvent.forget() instead, to trigger the nsRevocableEventPtr assignment-operator that takes an already_AddRefed (and doesn't touch the refcount), so that we don't cause extra addref/release churn.
Summary: Use .forget() when assigning RefPtr to RevocableEventPtr → Use .forget() when assigning from a RefPtr to a RevocableEventPtr, in layout
Priority: -- → P3
(In reply to Daniel Holbert [:dholbert] from comment #0)
> Here, mResizeEvent is of type nsRevocableEventPtr, and [...] "event" is of type RefPtr

Note: IF mResizeEvent were a RefPtr, then we could use Move() to give it its value (slightly more modern than forget().

But, it's not a RefPtr -- it's a nsRevocableEventPtr (a special wrapper around a RefPtr), which does not have a Move constructor, but *does* have an already_AddRefed constructor:
https://dxr.mozilla.org/mozilla-central/rev/5e73b9798464c3f7106f0161dc9a49b234f42f9c/xpcom/threads/nsThreadUtils.h#1624-1631

So, for now, forget() is the best we can do here (and it's an improvement from the status quo of unnecessary refcount churn).
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
What if nsRevocableEventPtr had only an operator=(RefPtr<T>&& aEvent) and didn't hvae operator=(already_AddRefed<T> aEvent) or operator=(RefPtr<T>& aEvent)?  Would that change it so failing to use Move() would be a compiler error, or would the compiler do the implicit conversion to T* to get around that?
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #2)
> What if nsRevocableEventPtr had only an operator=(RefPtr<T>&& aEvent) and
> didn't hvae operator=(already_AddRefed<T> aEvent) or operator=(RefPtr<T>&
> aEvent)?  Would that change it so failing to use Move() would be a compiler
> error

Yeah, local tests seem to indicate that this does make failure-to-use-Move a compiler error.

Maybe that'd be worth it? I'll see what I can do...  It's possible we'll have some cases (outside of layout) where we want to do a few more things with the assignment-source after we've copied it into nsRevocableEventPtr... But I think we could hack around that sort of scenario with an extra local variable, and hopefully that'd be rare & not too messy.
(In reply to Daniel Holbert [:dholbert] from comment #5)
> (In reply to David Baron :dbaron: ⌚️UTC-7 from comment #2)
> > What if nsRevocableEventPtr had only an operator=(RefPtr<T>&& aEvent) and
> > didn't hvae operator=(already_AddRefed<T> aEvent) or operator=(RefPtr<T>&
> > aEvent)?  Would that change it so failing to use Move() would be a compiler
> > error
> 
> Yeah, local tests seem to indicate that this does make failure-to-use-Move a
> compiler error.

Correction -- this strategy (leaving only a Move assignment operator) *only* makes this a compile error:
  myRevocableEvent = myRefPtr;

...but these still work (presumably through implicit conversion producing a RefPtr<T>&& under the hood):
  myRevocableEvent = myRefPtr.forget(); // already_addRefed
  myRevocableEvent = myRefPtr.get();    // raw pointer
  myRevocableEvent = someRawPtr;        // raw pointer

So on the one hand, it seems like comment 2 wouldn't buy us much strictness. But on the other hand, the strictness that it *does* buy us is mostly sufficient to avoid/discourage the "unnecessary refcount churn" problem-scenario here. (Unless someone does myRefPtr.get(), but that's pretty well understood to be an anti-pattern and hopefully will throw up some red flags to reviewers...)
(Updating summary to reflect new plans here, post-comment 2.)

I'll tag froydnj for review on this once he's back from vacation.
Component: Layout → XPCOM
Flags: needinfo?(dholbert)
Summary: Use .forget() when assigning from a RefPtr to a RevocableEventPtr, in layout → Use Move() when assigning from a RefPtr to a RevocableEventPtr
Attachment #8887693 - Flags: review?(nfroyd)
Flags: needinfo?(dholbert)
Comment on attachment 8887693 [details]
Bug 1349683: Give RevocableEventPtr a "move" assignment operator, and use it to reduce refcount churn.

https://reviewboard.mozilla.org/r/158594/#review165828

r+ for refcount-churn reducing patches.
Attachment #8887693 - Flags: review?(nfroyd) → review+
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea313d4b3093
Give RevocableEventPtr a "move" assignment operator, and use it to reduce refcount churn. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/ea313d4b3093
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Too late for 55. Mark 55 won't fix.
This brought an improvement. Thanks!

== Change summary for alert #8196 (as of July 24 2017 19:45 UTC) ==

Improvements:

  7%  Strings PerfStripCharsCRLF windows10-64 opt      740,819.62 -> 689,997.75

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8196
I'm skeptical about whether that was a real win or not -- it looks like we just had a brief spike, and happened to return to the baseline value when this bug's commit landed (and I don't think this bug deserves any special credit).

Looking at the graph[1], it looks like we had a small regression (i.e. the points all jump upwards) for a 3-hour period on July 24 -- specifically, from 16:45 - 19:45.  The measurements in that range are higher than usual, but the measurements before/after that range seem to all be about the same.

I would bet this was just us briefly tripping a PGO quirk, and then un-tripping it somehow, or something else odd like that.  The first "bad" commit was https://hg.mozilla.org/integration/autoland/rev/bfdd14469c99 (which caused alert https://treeherder.mozilla.org/perf.html#/alerts?id=8190 ), and the first "good" commit was for this bug here (with alert linked in comment 13).

So anyway - I suspect neither of the good/bad commits was especially good/bad, and thank goodness the random spike was temporary. :)

[1] https://treeherder.mozilla.org/perf.html#/graphs?timerange=2592000&series=%5Bautoland,d99b99eda8a9a3978d58569b73f7348d838fb67e,1,6%5D&zoom=1500881148487.6404,1500957918204.4944,500000,977611.9402985075
(I'm not sure if this graph will continue to be correct as time moves on; hence, I'm including the actual wallclock date & time above, too.)
Thanks Daniel, for shedding light over this perf result. If not for this bug, then for the explanation.
You need to log in before you can comment on or make changes to this bug.