|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
59 bytes, text/x-review-board-request
|Details | Review|
Browser:Thumbnail:CheckState handling shows up during non-idle periods when running speedometer.
Here's the whole profile of the first 50 seconds of speedometer, filtered to only samples that include processing this message: http://bit.ly/2sY0ACD The only time it seems to take up any time at all is the first time it's called, which is when it loads the PageThumbUtils.jsm module. That instance of the handler takes 2.3ms. Unfortunately this first handler falls right into one of the async tests, so it contributes to the speedometer score. All the other instances of this message take between 0.1ms and 0.3ms.
Will this change with Activity Stream?
Just cc'ing ursula, who could provide a better answer
I don't think this will change with Activity Stream to be honest - Activity Stream will still attempt to get the thumbnails in the same way that the current about:newtab does, which means it will still be loading PageThumbUtils.jsm. I can look into this further - it might be a good opportunity to deal with Bug 1353584 now, which may help with this here.
I agree that this code: http://searchfox.org/mozilla-central/rev/b7e531410ebc1c7d74a78e86a894e69608db318c/toolkit/components/thumbnails/PageThumbUtils.jsm#256-320 could probably be made to use requestIdleCallback, and could be broken up into smaller tasks. We might also want to have the parent avoid querying the child for this information if we know the tab has become busy again. We do something similar if the tab is closed. : http://searchfox.org/mozilla-central/rev/b7e531410ebc1c7d74a78e86a894e69608db318c/browser/base/content/browser-thumbnails.js#157-168
Comment on attachment 8895111 [details] Bug 1376511 - Handle Browser:Thumbnail:CheckState during idle period My (rather simplistic) approach seems to work (I don't see Browser:Thumbnail:CheckState in my profile anymore). However, I am concerned that I did not really follow your advice from Comment 5. Is this approach ok? Also, how can I tell if it is running too long for an idle callback and needs to be broken up?
Comment on attachment 8895111 [details] Bug 1376511 - Handle Browser:Thumbnail:CheckState during idle period https://reviewboard.mozilla.org/r/166226/#review171732 I actually think this is a fine start. Looking back through the `shouldStoreContentThumbnail` code, I'm less convinced now that we could usefully break that up into smaller chunks. If we _were_ to do that, I'd recommend making the `shouldStoreContentThumbnail` an `async function`, and having it immediately await on a `requestIdleCallback`, and accepting the `IdleDeadline` object that a `requestIdleCallback` gets. Then, as the `shouldStoreContentThumbnail` progresses, check the `IdleDeadline` to see if we've exceeded it. As soon as that happens, we should await on another `requestIdleCallback` and overwrite the `IdleDeadline` with what the callback eventually gets. However, that sounds like a lot of work given the fact that we don't really have a good sense of how expensive `shouldStoreContentThumbnail` is (we just know it can run at inconvenient times). So I suggest we take your patch here, and see if the times that `shouldStoreContentThumbnail` is called are problematically long - if so, we can then revisit breaking it up. Sound good?
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/1a3018f82800 Handle Browser:Thumbnail:CheckState during idle period r=mconley
Hopefully this version of the patch fixes this problem. Pushing to try to confirm.
Comment on attachment 8895111 [details] Bug 1376511 - Handle Browser:Thumbnail:CheckState during idle period Tests seem to be passing on try. Look good to re-merge?
Comment on attachment 8895111 [details] Bug 1376511 - Handle Browser:Thumbnail:CheckState during idle period https://reviewboard.mozilla.org/r/166226/#review171882
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/9499e1097909 Handle Browser:Thumbnail:CheckState during idle period r=mconley