Closed Bug 1349683 Opened 3 years ago Closed 2 years ago
Use Move() when assigning from a Ref
Ptr to a Revocable Event Ptr
Bug 1349683: Give RevocableEventPtr a "move" assignment operator, and use it to reduce refcount churn.
59 bytes, text/x-review-board-request
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
(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
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)
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 email@example.com: https://hg.mozilla.org/integration/autoland/rev/ea313d4b3093 Give RevocableEventPtr a "move" assignment operator, and use it to reduce refcount churn. r=froydnj
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, 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. :)  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.