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

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Adam]

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.

Comment 1

[Liviu Cirdei]

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?

Comment 2

[Adam]

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.

Comment 3

[Liviu Cirdei]

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

Comment 4

[Liviu Cirdei]

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.

Comment 5

[Adam]

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.

Comment 6

[Adam]

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

[Liviu Cirdei]

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?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[Dão Gottwald [:dao]]

(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.

Comment 10

[:Gijs (he/him)]

(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?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 13

[Florian Quèze [:florian]]

Here is a profile with symbols: https://perfht.ml/2oYjGcA

Comment 14

[Dão Gottwald [:dao]]

(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.

Comment 15

[Dão Gottwald [:dao]]

Relevant discussion: bug 646038 comment 21 and below where roc seems to imply that mozInnerScreenX shouldn't flush layout.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

(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

[Olli Pettay [:smaug][[e-mail]]]

dbaron reviewed Bug 486200 so perhaps he can explain why flush was needed there or was that just a mistake.

Comment 18

[Dão Gottwald [:dao]]

(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.

Comment 19

[Marco Castelluccio [:marco]]

(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.

Comment 20

[Dão Gottwald [:dao]]

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.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 22

[Dão Gottwald [:dao]]

(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.

Comment 23

[Timothy Nikkel (:tnikkel)]

(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.

Comment 24

[Dão Gottwald [:dao]]

(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?

Comment 25

[Timothy Nikkel (: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.

Comment 26

[Dão Gottwald [:dao]]

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?

Comment 27

[Timothy Nikkel (:tnikkel)]

It doesn't look like clientX flushes:

https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/dom/events/Event.cpp#983

Comment 28

[Dão Gottwald [:dao]]

(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.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e5115f9958b1060e6366c4bcea51cd7bc022c8c&group_state=expanded

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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

Comment 33

[Timothy Nikkel (:tnikkel)]

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.

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ae32bc1ac57f
https://hg.mozilla.org/mozilla-central/rev/3067c20eeccb

Comment 37

[Adrian Florinescu [:aflorinescu]]

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?

Comment 38

[Adam]

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)

Comment 39

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 40

[Adrian Florinescu [:aflorinescu]]

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.