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

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

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

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

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

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

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.

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

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

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