browser-thumbnails.js' _delayedCapture contributes to UI jank

RESOLVED FIXED in Firefox 57

Status

()

Firefox
New Tab Page
P1
normal
RESOLVED FIXED
5 months ago
7 days ago

People

(Reporter: florian, Assigned: Perry)

Tracking

(Blocks: 3 bugs)

unspecified
Firefox 57
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [reserve-photon-performance])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 months ago
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
(Reporter)

Updated

4 months ago
Blocks: 1354723

Updated

4 months ago
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [photon] → [photon-performance]
(Reporter)

Updated

3 months ago
Blocks: 1363771
(Reporter)

Updated

3 months ago
No longer blocks: 1348289
Florian, I think this is a dupe of bug 1350781.  What do you think?
Flags: needinfo?(florian)
Maybe we should repurpose this bug to move the thumbnail capture code to idle dispatch APIs?

Updated

3 months ago
Priority: P2 → P3
Whiteboard: [photon-performance] → [reserve-photon-performance]
(Reporter)

Comment 3

3 months ago
(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.
Flags: needinfo?(florian)
Assignee: nobody → jiangperry
Status: NEW → ASSIGNED

Updated

21 days ago
Priority: P3 → P1
(Assignee)

Comment 4

21 days ago
Created attachment 8891467 [details] [diff] [review]
Bug 1353584 - replace setTimeout with requestIdleCallback in _delayedCapture and return early on about: page in _shouldCapture
Attachment #8891467 - Flags: review?(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.
Attachment #8891467 - Flags: review?(mconley) → review-
(Assignee)

Comment 6

18 days ago
Created attachment 8892201 [details] [diff] [review]
[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
Attachment #8891467 - Attachment is obsolete: true
(Assignee)

Comment 7

16 days ago
Created attachment 8893155 [details] [diff] [review]
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).
Attachment #8892201 - Attachment is obsolete: true
Attachment #8893155 - Flags: review?(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.
Attachment #8893155 - Flags: review?(mconley) → review-
(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)
Blocks: 1348280
(Assignee)

Comment 10

10 days ago
Created attachment 8895117 [details] [diff] [review]
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.
Attachment #8893155 - Attachment is obsolete: true
Attachment #8895117 - Flags: review?(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?
Attachment #8895117 - Flags: review?(mconley) → review+
(Assignee)

Comment 12

8 days ago
Created attachment 8895916 [details] [diff] [review]
Bug 1353584 - replace setTimeout with requestIdleCallback in _delayedCapture and return early on about: page in _shouldCapture
Attachment #8895117 - Attachment is obsolete: true
Attachment #8895916 - Flags: review+

Comment 13

8 days ago
Pushed by felipc@gmail.com:
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

7 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4c172f37cccd
Status: ASSIGNED → RESOLVED
Last Resolved: 7 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Iteration: --- → 57.1 - Aug 15
You need to log in before you can comment on or make changes to this bug.