1353584 - browser-thumbnails.js' _delayedCapture contributes to UI jank

Status: RESOLVED FIXED

(Firefox :: New Tab Page, enhancement, P1)

Description

[Florian Quèze [:florian]]

See this profile where I just pressed ctrl+n 8 times on a slow i3 laptop: https://perf-html.io/public/339d2d63b06ddef6a368a65d10eb991f8a119dee/calltree/?jsOnly&range=10.6006_22.6804&thread=0

Thumbnails_delayedCapture is 6.2% of the CPU time, with 154ms spent.

148ms (6.0% of total time) is spent in get _topSiteURLs.

Several things could easily be improved here:
- this code should use window.requestIdleCallback to avoid having this piling up above existing jank.
- get _topSiteURLs / Links_getLinks is doing way too much computation. The result should be cached within NewTabUtils.jsm
- when it's obvious that we won't be capturing the current page, we should return early without even setting the timer. In the case of this profile, the page will always be about:home, which we won't capture. We currently discard about: pages very late in the process, at http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/toolkit/components/thumbnails/PageThumbUtils.jsm#281

Comment 1

[(no longer active)]

Florian, I think this is a dupe of bug 1350781.  What do you think?

Comment 2

[(no longer active)]

Maybe we should repurpose this bug to move the thumbnail capture code to idle dispatch APIs?

Comment 3

[Florian Quèze [:florian]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #2)
> Maybe we should repurpose this bug to move the thumbnail capture code to
> idle dispatch APIs?

So now that bug 1350781 is fixed, the parts that remain here are using the idle dispatch API, and returning early for about: pages to get out of the way when opening a new window loading about:home. But it's been long enough that we should probably reprofile this when we get a chance.

Comment 4

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1353584 - replace setTimeout with requestIdleCallback in _delayedCapture and return early on about: page in _shouldCapture

Comment 5

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

Comment on attachment 8891467 [details] [diff] [review]
Bug 1353584 - replace setTimeout with requestIdleCallback in _delayedCapture and return early on about: page in _shouldCapture

Review of attachment 8891467 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Perry - this looks pretty good, but there's a slight change needed. See below.

::: browser/base/content/browser-thumbnails.js
@@ +166,2 @@
>  
> +    let idleCallbackId = window.requestIdleCallback(() => {

I don't think this is what we actually want here.

requestIdleCallback will run when either:

a) The event loop is free enough to pop events out of the idle queue

OR

b) The timeout passed to requestIdleCallback will be reached, after which the callback will be placed inside a normal timer event which has higher priority.

This means that it's actually possible to have this callback run quite soon after _delayedCapture, instead of after _captureDelayMS.

What I recommend instead is that we continue to use a timer, but inside the timer callback, instead of calling this._capture(aBrowser) directly, wrap _that_ inside a requestIdleCallback.

This means that we have a very cheap call running on the timer (queuing the callback), after the right amount of time, and then _capture will run strictly _after_ the captureDelayMS time.

Comment 6

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: [WIP] Bug 1353584 - replace setTimeout with requestIdleCallback in _delayedCapture and return early on about: page in _shouldCapture

Without a timeout parameter (like 5000 ms, not a few minutes), requestIdleCallback causes browser_thumbnails_bug818225.js and browser_thumbnails_storage.js to time out on macOS 10.12.6.

Output for both tests:

GECKO(42943) | TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception - ReferenceError: executeSoon is not defined at whenFileExists@chrome://mochitests/content/browser/toolkit/components/thumbnails/test/head.js:224:3
GECKO(42943) | callback@chrome://mochitests/content/browser/toolkit/components/thumbnails/test/head.js:221:22
GECKO(42943) | run@chrome://mochikit/content/browser-test.js:1031:9
GECKO(42943) | JavaScript error: chrome://mochitests/content/browser/toolkit/components/thumbnails/test/head.js, line 224: ReferenceError: executeSoon is not defined

Comment 7

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1353584 - replace setTimeout with requestIdleCallback in _delayedCapture and return early on about: page in _shouldCapture

whenFileExists' executeSoon was blocking _delayedCapture's requestIdleCallback, so I changed it to a setTimeout(cb, 0).

Comment 8

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

Comment on attachment 8893155 [details] [diff] [review]
Bug 1353584 - replace setTimeout with requestIdleCallback in _delayedCapture and return early on about: page in _shouldCapture

Review of attachment 8893155 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Perry! Code looks mostly okay, but I have questions here... it seems like we might be wallpapering over an idleness bug here. I'd really like to understand why we need a timer here in the idleCallback - especially since that means that we might end up interrupting user input events.

::: browser/base/content/browser-thumbnails.js
@@ +170,5 @@
> +    };
> +
> +    let timeoutId = setTimeout(() => {
> +      let idleCallbackId = requestIdleCallback(idleCallback, {
> +        timeout: this._captureDelayMS * 30

* 30? Where is this magic number coming from?

Are you sure we need to set a timeout here? Have we found out why? And why this long?

@@ +177,3 @@
>      }, this._captureDelayMS);
>  
> +    this._timeouts.set(aBrowser, { stateIsTimeout: true, id: timeoutId });

Not sure if that's what I originally suggested, but I think "stateIsTimeout" is a bit overly verbose. Maybe just "isTimeout" is sufficient.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

(In reply to Mike Conley (:mconley) - Holiday until August 8th from comment #8)
> Comment on attachment 8893155 [details] [diff] [review]
> Bug 1353584 - replace setTimeout with requestIdleCallback in _delayedCapture
> and return early on about: page in _shouldCapture
> 
> Review of attachment 8893155 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks Perry! Code looks mostly okay, but I have questions here... it seems
> like we might be wallpapering over an idleness bug here. I'd really like to
> understand why we need a timer here in the idleCallback - especially since
> that means that we might end up interrupting user input events.
> 
> ::: browser/base/content/browser-thumbnails.js
> @@ +170,5 @@
> > +    };
> > +
> > +    let timeoutId = setTimeout(() => {
> > +      let idleCallbackId = requestIdleCallback(idleCallback, {
> > +        timeout: this._captureDelayMS * 30
> 
> * 30? Where is this magic number coming from?
> 
> Are you sure we need to set a timeout here? Have we found out why? And why
> this long?

We don't need this timeout, I suggested to add it just as a safety guard (with an arbitrary value), in order to avoid a situation where the thumbnail never gets created for some user due to some other addon or bug.

It was understood why it was happening, and it was a test-only problem. The function that was waiting for the thumbnail file to be created (and proceed with the test), was checking for the file existence and, if it did not exist, immediately schedule itself to run again with executeSoon().

And the function that causes this file to be created is.. the one that is moving to a requestIdleCallback.. So it always lost in precedence.

At least now we have proof that requestIdleCallback really works as it's meant to!

(although I'm a bit disappointed that setTimeout(..., 0) doesn't win over the idleCallback)

Comment 10

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1353584 - replace setTimeout with requestIdleCallback in _delayedCapture and return early on about: page in _shouldCapture

The timeout in the requestIdleCallback is just in case something like whenFileExists keeps blocking the idle callback, and the number 30 in the timeout is arbitrary to allow for a reasonable amount of time to pass.

Comment 11

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

Comment on attachment 8895117 [details] [diff] [review]
Bug 1353584 - replace setTimeout with requestIdleCallback in _delayedCapture and return early on about: page in _shouldCapture

Review of attachment 8895117 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Perry!

::: browser/base/content/browser-thumbnails.js
@@ +170,5 @@
> +    };
> +
> +    let timeoutId = setTimeout(() => {
> +      let idleCallbackId = requestIdleCallback(idleCallback, {
> +        timeout: this._captureDelayMS * 30

Could you please add a comment here explicitly calling out the need for the timeout?

Comment 12

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1353584 - replace setTimeout with requestIdleCallback in _delayedCapture and return early on about: page in _shouldCapture

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c172f37cccd
replace setTimeout with requestIdleCallback in _delayedCapture and return early on about: page in _shouldCapture. r=mconley

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4c172f37cccd