MousePosTracker flushes layout on mouse move (via mozInnerScreenX/Y)

VERIFIED FIXED in Firefox 55

Status

()

Core
DOM: Core & HTML
P1
normal
VERIFIED FIXED
11 months ago
13 days ago

People

(Reporter: Adam, Assigned: dbaron)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla55
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified, firefox56 verified, firefox57 verified)

Details

(Whiteboard: [photon-performance] [qf:p1][qa-commented])

Attachments

(2 attachments)

(Reporter)

Description

11 months ago
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160922183953

Steps to reproduce:

my system is linux mint 18 with xorg 7.7, kernel 4.4.0-38, cpu intel i5 mobile and gpu i915.

1. open a fresh install of firefox
2. disable all plugins
3. open bugzilla page
4. start making circles with the mouse in an empty area.


Actual results:

top reports cpu usage of about 22% for firefox and 13% for Xorg (totals to about 35%)

in the setup above e10s was enabled. in my current setup it's disabled and usage is "only" 28%.


Expected results:

chrome stays at about 18% total cpu usage. other programs even lower. i think firefox should be smarter about when to ignore events.
(Reporter)

Updated

11 months ago
OS: Unspecified → Linux
Hardware: Unspecified → x86_64

Comment 1

11 months ago
Hi Adam, 

I tried to reproduce the issue on Ubuntu 15.04 (I don't have Linux Mint), but the CPU usage reached only 3% for Firefox.
Do you use a standard mouse?
Flags: needinfo?(mozilla_org)
(Reporter)

Comment 2

11 months ago
I right now tried a standard usb mouse and the (apple) touchpad. Both give the same result.

the percentage most probably depends on the cpu speed. I have a Intel(R) Core(TM) i5-4260U CPU @ 1.40GHz from 2013.
(http://ark.intel.com/es/products/75030/Intel-Core-i5-4260U-Processor-3M-Cache-up-to-2_70-GHz)

This is not really problematic in terms of performance, but in terms of energy efficiency. Once i move the mouse, the cpu has to clock up, often using turbo boost. This is only one of the energy efficiency issues, I often switch to a different browser when on battery.
Flags: needinfo?(mozilla_org)

Comment 3

10 months ago
Do you have Linux Mint on a MacBook?

Could you make a performance profile using Cleopatra [1]? Maybe like this we can figure out what causes the high CPU usage.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Reporting_a_Performance_Problem
Flags: needinfo?(mozilla_org)

Comment 4

10 months ago
Closing as incomplete due to lack of response from reporter. Adam, if you still reproduce the issue on latest Firefox version feel free to reopen it and provide the needed info.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 10 months ago
Flags: needinfo?(mozilla_org)
Resolution: --- → INCOMPLETE
(Reporter)

Comment 5

10 months ago
sorry for the delay, lot of things going on atm, it was on my list..

here it is: https://cleopatra.io/#report=f49eef49fff7b19f7c080eb9e4dbb3a480e4f845&selection=0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15

i'll be able to respond quicker now.
Status: RESOLVED → UNCONFIRMED
Resolution: INCOMPLETE → ---
(Reporter)

Comment 6

10 months ago
Oh, and - i'm unsure whether it was a rhetorical question or not - yes, linux mint on a macbook air.

i set a lower max frequency in battery mode, which of course increases the effect of circling the mouse..

Comment 7

10 months ago
Olly, could you have a look at the Cleopatra profile from comment 5? Is there anything that can help to identify what causes the issue?
Flags: needinfo?(bugs)

Comment 8

10 months ago
FWIW, I get 1-2% cpu usage.

If I interpret the profile right, most of the time we're just waiting for more events in poll.

But I do see something unexpected, MousePosTracker.
Dao, what is that for, and why do we need to handle mousemove events in parent process when
mouse is moving over a web page? We don't seem to even have any _listeners in MousePosTracker most of the time, 
MousePosTracker.handleEvent doesn't end up calling anything.
Shouldn't MousePosTracker listener have the listener registered on window only when someone actually needs the event and not have this
http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/browser/base/content/browser.js#1327-1328 by default.
Flags: needinfo?(bugs) → needinfo?(dao+bmo)

Comment 9

9 months ago
(In reply to Olli Pettay [:smaug] from comment #8)
> FWIW, I get 1-2% cpu usage.
> 
> If I interpret the profile right, most of the time we're just waiting for
> more events in poll.
> 
> But I do see something unexpected, MousePosTracker.
> Dao, what is that for, and why do we need to handle mousemove events in
> parent process when
> mouse is moving over a web page? We don't seem to even have any _listeners
> in MousePosTracker most of the time, 
> MousePosTracker.handleEvent doesn't end up calling anything.
> Shouldn't MousePosTracker listener have the listener registered on window
> only when someone actually needs the event and not have this
> http://searchfox.org/mozilla-central/rev/
> 4b6cab91f93c73ae591dafaea40fd5704b41810e/browser/base/content/browser.
> js#1327-1328 by default.

The problem is that we need up-to-date mouse coordinates by the time a consumer adds a listener, see bug 646038.
Flags: needinfo?(dao+bmo)

Updated

8 months ago
Component: Untriaged → General

Comment 10

8 months ago
(In reply to Dão Gottwald [:dao] from comment #9)
> (In reply to Olli Pettay [:smaug] from comment #8)
> > FWIW, I get 1-2% cpu usage.
> > 
> > If I interpret the profile right, most of the time we're just waiting for
> > more events in poll.
> > 
> > But I do see something unexpected, MousePosTracker.
> > Dao, what is that for, and why do we need to handle mousemove events in
> > parent process when
> > mouse is moving over a web page? We don't seem to even have any _listeners
> > in MousePosTracker most of the time, 
> > MousePosTracker.handleEvent doesn't end up calling anything.
> > Shouldn't MousePosTracker listener have the listener registered on window
> > only when someone actually needs the event and not have this
> > http://searchfox.org/mozilla-central/rev/
> > 4b6cab91f93c73ae591dafaea40fd5704b41810e/browser/base/content/browser.
> > js#1327-1328 by default.
> 
> The problem is that we need up-to-date mouse coordinates by the time a
> consumer adds a listener, see bug 646038.

Olli, is there (and/or can we implement) an alternative API we can use for this so we can simply ask for the mouse coordinates when we need them instead?
Flags: needinfo?(bugs)
Duplicate of this bug: 1355805
Blocks: 1338595, 1348289
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: making circles with the mouse on any website causes 35% cpu load (firefox + Xorg) → MousePosTracker flushes layout on mouse move
Whiteboard: [photon][qf]
Version: 49 Branch → Trunk

Updated

4 months ago
Flags: qe-verify+
Priority: -- → P2
QA Contact: adrian.florinescu
Whiteboard: [photon][qf] → [photon-performance] [qf]

Comment 12

4 months ago
Could MousePosTracker register the mousemove event listener only when mouseover happens on some particular target and remove listener when mouseout is triggered?


The flushing happens when script accesses mozInnerScreenX. Do we need to access that in event listener?
Flags: needinfo?(bugs)
Here is a profile with symbols: https://perfht.ml/2oYjGcA
(In reply to Olli Pettay [:smaug] from comment #12)
> Could MousePosTracker register the mousemove event listener only when
> mouseover happens on some particular target and remove listener when
> mouseout is triggered?

I think that element would be the content area / the selected browser for our primary use case, so not really practical.

> The flushing happens when script accesses mozInnerScreenX. Do we need to
> access that in event listener?

Hmm, this is what we're doing:

    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.
Relevant discussion: bug 646038 comment 21 and below where roc seems to imply that mozInnerScreenX shouldn't flush layout.

Comment 16

4 months ago
(In reply to Dão Gottwald [::dao] from comment #14)
> (In reply to Olli Pettay [:smaug] from comment #12)
> > Could MousePosTracker register the mousemove event listener only when
> > mouseover happens on some particular target and remove listener when
> > mouseout is triggered?
> 
> I think that element would be the content area / the selected browser for
> our primary use case, so not really practical.
What is the use case?
I thought it was used for tabbar or something.

Comment 17

4 months ago
dbaron reviewed Bug 486200 so perhaps he can explain why flush was needed there or was that just a mistake.
Flags: needinfo?(dbaron)
(In reply to Olli Pettay [:smaug] from comment #16)
> (In reply to Dão Gottwald [::dao] from comment #14)
> > (In reply to Olli Pettay [:smaug] from comment #12)
> > > Could MousePosTracker register the mousemove event listener only when
> > > mouseover happens on some particular target and remove listener when
> > > mouseout is triggered?
> > 
> > I think that element would be the content area / the selected browser for
> > our primary use case, so not really practical.
> What is the use case?
> I thought it was used for tabbar or something.

It's for the status panel at the bottom of the window that displays the loading status and link target URLs, and switches to the other side when the pointer comes too close.
Whiteboard: [photon-performance] [qf] → [photon-performance] [qf:p1]
(In reply to Dão Gottwald [::dao] from comment #18)
> (In reply to Olli Pettay [:smaug] from comment #16)
> > (In reply to Dão Gottwald [::dao] from comment #14)
> > > (In reply to Olli Pettay [:smaug] from comment #12)
> > > > Could MousePosTracker register the mousemove event listener only when
> > > > mouseover happens on some particular target and remove listener when
> > > > mouseout is triggered?
> > > 
> > > I think that element would be the content area / the selected browser for
> > > our primary use case, so not really practical.
> > What is the use case?
> > I thought it was used for tabbar or something.
> 
> It's for the status panel at the bottom of the window that displays the
> loading status and link target URLs, and switches to the other side when the
> pointer comes too close.

Is it an important enough feature? Couldn't we remove it altogether? The mouse cursor can only hide a couple of characters from the status text and the user can move the mouse again if he actually wants to read them.
Yes, it's important enough. It's not about the pointer hiding part of the status text but the status panel hiding part of the Web content.
For context, mozInnerScreenX/Y were added in bug 486200.

Here's where, I think, we flush: http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/dom/base/nsGlobalWindow.cpp#5991

Why do we need to do this? Why is the flush necessary? Can we not expose a way of getting potentially stale numbers?
(In reply to Mike Conley (:mconley) from comment #21)
> Why do we need to do this? Why is the flush necessary? Can we not expose a
> way of getting potentially stale numbers?

We need up-to-date numbers to avoid issues like bug 646038. However, it's unclear to me how mozInnerScreenX even depends on the document's layout.
(In reply to Dão Gottwald [::dao] from comment #22)
> (In reply to Mike Conley (:mconley) from comment #21)
> > Why do we need to do this? Why is the flush necessary? Can we not expose a
> > way of getting potentially stale numbers?
> 
> We need up-to-date numbers to avoid issues like bug 646038. However, it's
> unclear to me how mozInnerScreenX even depends on the document's layout.

nsGlobalWindow::GetInnerScreenRect flushes in the root window (not the current window), so if mozInnerScreenX is asked for on an iframe the iframe could change position after a reflow.
(In reply to Timothy Nikkel (:tnikkel) from comment #23)
> (In reply to Dão Gottwald [::dao] from comment #22)
> > (In reply to Mike Conley (:mconley) from comment #21)
> > > Why do we need to do this? Why is the flush necessary? Can we not expose a
> > > way of getting potentially stale numbers?
> > 
> > We need up-to-date numbers to avoid issues like bug 646038. However, it's
> > unclear to me how mozInnerScreenX even depends on the document's layout.
> 
> nsGlobalWindow::GetInnerScreenRect flushes in the root window (not the
> current window), so if mozInnerScreenX is asked for on an iframe the iframe
> could change position after a reflow.

Can we avoid this when the current window is the root window?
Flags: needinfo?(tnikkel)
(In reply to Dão Gottwald [::dao] from comment #24)
> > nsGlobalWindow::GetInnerScreenRect flushes in the root window (not the
> > current window), so if mozInnerScreenX is asked for on an iframe the iframe
> > could change position after a reflow.
> 
> Can we avoid this when the current window is the root window?

That seems logical if moving iframes is the only reason for the flush. I'll defer to dbaron in case I'm missing something.
Flags: needinfo?(tnikkel)
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?
Summary: MousePosTracker flushes layout on mouse move → MousePosTracker flushes layout on mouse move (via mozInnerScreenX/Y)
It doesn't look like clientX flushes:

https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/dom/events/Event.cpp#983
(In reply to Timothy Nikkel (:tnikkel) from comment #27)
> It doesn't look like clientX flushes:

Okay, filed bug 1356183, and moving this bug to Core::DOM.
Component: General → DOM: Core & HTML
Product: Firefox → Core

Updated

4 months ago
Depends on: 1356183
(Assignee)

Comment 29

4 months ago
(In reply to Olli Pettay [:smaug] from comment #17)
> dbaron reviewed Bug 486200 so perhaps he can explain why flush was needed
> there or was that just a mistake.

So the patch I reviewed there added a nsGlobalWindow::GetInnerScreenRect that flushed.  On the *assumption* that that's the flush you're asking about:  I would assume that getting a screen relative rect for a window requires flushing ancestor documents (although not the self document) because those ancestor documents might have pending layout changes that change the position of the iframe.  I don't think that flushing the entire subtree is necessary, though -- it could be limited to only ancestors of the current document.  (Though we'd need to do siblings and siblings-of-ancestors as well, if those siblings include seamless iframes, if we ever implement seamless iframes.  Though if we do, presumably we'd have to adjust the flushing code for that.)  That said, the code there looks like it flushes only the toplevel document, which seems buggy (unless there's some transitive flushing that I've forgotten about).  It would be interesting to know whether other browsers have similar APIs and whether they flush.
Flags: needinfo?(dbaron)
(Assignee)

Comment 30

4 months ago
Perhaps the existing nsGlobalWindow::EnsureSizeUpToDate helper should have an EnsurePositionUpToDate alias, which I believe would be the same for now, although it might be worth preserving the difference in intent.  We can then rely on the parent to flush its parent, since nsDocument::FlushPendingNotifications does the necessary promotion to the parent.
(Assignee)

Updated

4 months ago
Assignee: nobody → dbaron
(Assignee)

Comment 31

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e5115f9958b1060e6366c4bcea51cd7bc022c8c&group_state=expanded
(Assignee)

Comment 32

4 months ago
Created attachment 8858738 [details] [diff] [review]
Only flush in ancestor documents for window.mozInnerScreenX/Y

This reduces the amount of flushing we do when these APIs are called on
the root document, but increases the amount of flushing we do (probably
fixing existing bugs) when these APIs are called in a document at depth
three or more (if you consider the root depth one).

I considered the idea of adding a EnsurePositionUpToDate alias, but it
seems that some of the existing users of EnsureSizeUpToDate actually
care about position (e.g.,
nsLayoutUtils::GetDeviceContextForScreenInfo), so I just added a comment
instead.

MozReview-Commit-ID: B3L5DDQ5krc
Attachment #8858738 - Flags: review?(tnikkel)

Updated

4 months ago
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8858738 [details] [diff] [review]
Only flush in ancestor documents for window.mozInnerScreenX/Y

Since there are only a handful of uses, might be easier to rename it to EnsureSizeAndPositionUpToDate.
Attachment #8858738 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 34

4 months ago
Created attachment 8858969 [details] [diff] [review]
Rename nsGlobalWindow::EnsureSizeUpToDate to EnsureSizeAndPositionUpToDate

This is because it does ensure both, and some of the callers care about
size and some care about position.

MozReview-Commit-ID: 3e8II6Lf72X
Attachment #8858969 - Flags: review?(tnikkel)
Attachment #8858969 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 35

4 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae32bc1ac57f30144fb13e82345f448ea44df579
Bug 1307134 - Only flush in ancestor documents for window.mozInnerScreenX/Y.  r=tnikkel

https://hg.mozilla.org/integration/mozilla-inbound/rev/3067c20eeccb1671b379ef882c37288219f4207b
Bug 1307134 - Rename nsGlobalWindow::EnsureSizeUpToDate to EnsureSizeAndPositionUpToDate.  r=tnikkel

Comment 36

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ae32bc1ac57f
https://hg.mozilla.org/mozilla-central/rev/3067c20eeccb
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

4 months ago
Iteration: --- → 55.4 - May 1
See Also: → bug 1354194
I don't have a Mint distro to try to reproduce the initial bug, so I can't verify the fix (on Ubuntu I can't repro the initial report -as in the initial comments, I don't see the increase in usage.)
Adam, could you please lend us a hand in verifying this fix?

David, could you please advise on an alternative method for us in order to verify this?
Flags: needinfo?(mozilla_org)
Flags: needinfo?(dbaron)
No longer blocks: 1348289
Blocks: 1348289
(Reporter)

Comment 38

2 months ago
hi,
sorry for the long wait. I was very busy and didn't check the mail.

This is my new profile: https://perfht.ml/2tyNrT9

The original bug report is not resolved, it got worse. The cpu load is now between 60 and 80% on ff 56 nighlty.

However, since the original bugreport was renamend, i'm leaving this one closed and I opened a new one (#1377768)
Flags: needinfo?(mozilla_org)
For QA:

For testing this, at least for what the patch eventually addressed, you'll want to move the mouse around during a pageload where the status display (The thing that says "Transferring", "Waiting", etc in the bottom left corner during page loads, and then disappears) properly moves to the other side of the page when the mouse approaches it when visible.
Whiteboard: [photon-performance] [qf:p1] → [photon-performance] [qf:p1][qa-commented]
Marking bug as verified on: Ubuntu 16.04, Windows 10 and since I wasn't sure if the patch affected only linux, I gave it a short run on Windows as well.

55.0 20170803103124
56.0b1  20170808170225
57.0a1 20170810100255

Regression and exploratory related only to the status display only comment 39, as far as I can tell the performance issue is being currently investigated in bug 1377768.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
status-firefox56: --- → verified
status-firefox57: --- → verified
Flags: qe-verify+
Flags: needinfo?(dbaron)
You need to log in before you can comment on or make changes to this bug.