Closed Bug 1356183 Opened 3 years ago Closed 3 years ago

MousePosTracker should use event.clientX instead of event.screenX - window.mozInnerScreenX to avoid flushing layout

Categories

(Firefox :: General, defect, P1)

defect

Tracking

()

RESOLVED WONTFIX
Firefox 55
Tracking Status
firefox53 --- wontfix
firefox54 --- fixed
firefox55 --- disabled

People

(Reporter: dao, Assigned: dao)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(1 file)

(In reply to Dão Gottwald [::dao] from bug 1307134 comment 14)
>     var fullZoom = this._windowUtils.fullZoom;
>     this._x = event.screenX / fullZoom - window.mozInnerScreenX;
>     this._y = event.screenY / fullZoom - window.mozInnerScreenY;
> 
> Not sure why mozInnerScreenX ("the X coordinate of the top-left corner of
> the window's viewport, in screen coordinates") flushes layout in the first
> place, but maybe we can just use event.clientX instead? Not sure if this
> gives the same results, this whole stuff is a bit confusing.

(In reply to Dão Gottwald [::dao] from bug 1307134 comment 26)
> It would also be good if somebody could clarify if using event.clientX
> instead of event.screenX - window.mozInnerScreenX would be a good idea. It
> seems to work in my testing, but maybe it implies a layout flush as well?

(In reply to Timothy Nikkel (:tnikkel) from bug 1307134 comment 27)
> It doesn't look like clientX flushes:
> 
https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/dom/events/Event.cpp#983
Assignee: nobody → dao+bmo
Blocks: 1307134
Status: NEW → ASSIGNED
Keywords: perf
Summary: MousePosTracker should use event.clientX instead of event.screenX - window.mozInnerScreenX → MousePosTracker should use event.clientX instead of event.screenX - window.mozInnerScreenX to avoid flushing layout
Whiteboard: [photon-performance]
Flags: qe-verify?
Priority: -- → P1
Would be nice if QA could verify that there's no behavior change in how the status panel at the bottom of the window (that displays the loading status and link URLs) interacts with the mouse pointer, using different DPI settings and window modes (maximized, restored, fullscreen). Bonus points for verifying that the layout flush is gone, but I'm not sure if QA is equipped for that.
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu
Comment on attachment 8857875 [details]
Bug 1356183 - Let MousePosTracker use event.clientX instead of event.screenX - window.mozInnerScreenX to avoid flushing layout.

https://reviewboard.mozilla.org/r/129902/#review132518

Let's try this, thanks :-). On my fast macbook I can't get the code without the patch to be visible in the profiles, so I can't verify if applying the patch helps. Once this is on Nightly I'll profile again on my very old linux netbook to confirm that I no longer see it in profiles.
Attachment #8857875 - Flags: review?(florian) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3edd47730527
Let MousePosTracker use event.clientX instead of event.screenX - window.mozInnerScreenX to avoid flushing layout. r=florian
https://hg.mozilla.org/mozilla-central/rev/3edd47730527
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> Once this is on Nightly I'll profile again on my very old linux
> netbook to confirm that I no longer see it in profiles.

Here is a new profile from a 2017-04-14 nightly on that same netbook: https://perfht.ml/2ofbaB5

ClientX accounts for 2ms out of 50ms spent in that event listener.
The forEach overhead is 26ms (more than half the total time), that's bug 1355874.
nsDOMWindowUtils::GetFullZoom(float*) is 4ms, should we somehow cache this value?

This confirms that the layout flush is gone.
(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> (In reply to Florian Quèze [:florian] [:flo] from comment #3)
> > Once this is on Nightly I'll profile again on my very old linux
> > netbook to confirm that I no longer see it in profiles.
> 
> Here is a new profile from a 2017-04-14 nightly on that same netbook:
> https://perfht.ml/2ofbaB5
> 
> ClientX accounts for 2ms out of 50ms spent in that event listener.
> The forEach overhead is 26ms (more than half the total time), that's bug
> 1355874.

I think forEach can handle a listener removing itself in the loop? I can't recall how for..of behaves here.
Comment on attachment 8857875 [details]
Bug 1356183 - Let MousePosTracker use event.clientX instead of event.screenX - window.mozInnerScreenX to avoid flushing layout.

Approval Request Comment
[Feature/Bug causing the regression]: /
[User impact if declined]: possibly poor performance when moving the mouse over the browser window
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: it's been confirmed that the performance problem is solved
[Needs manual test from QE? If yes, steps to reproduce]: see comment 2
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: no (once verified that using clientX doesn't change behavior)
[Why is the change risky/not risky?]: very small, isolated fix
[String changes made/needed]:
Attachment #8857875 - Flags: approval-mozilla-aurora?
Comment on attachment 8857875 [details]
Bug 1356183 - Let MousePosTracker use event.clientX instead of event.screenX - window.mozInnerScreenX to avoid flushing layout.

Fix a perf issue when moving the mouse over the browser window. Aurora54+.
Attachment #8857875 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Iteration: --- → 55.3 - Apr 17
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/f18e288534fbc91b47a4d11a25409fdae274a1d2

clientX/Y don't work the way we need it with non-e10s content (bug 1357841), and this patch isn't necessary anymore since bug 1307134 is fixed. However I'm going to leave this patch in 54 unless bug 1307134 gets uplifted.
Resolution: FIXED → WONTFIX
Iteration: 55.3 - Apr 17 → ---
Flags: qe-verify+
QA Contact: adrian.florinescu
Whiteboard: [photon-performance]
You need to log in before you can comment on or make changes to this bug.