Closed
Bug 1376511
Opened 7 years ago
Closed 7 years ago
Browser:Thumbnail:CheckState should be handled during idle period in child process
Categories
(Firefox :: New Tab Page, defect)
Firefox
New Tab Page
Tracking
()
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: smaug, Assigned: bytesized)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
Browser:Thumbnail:CheckState handling shows up during non-idle periods when running speedometer.
Comment 1•7 years ago
|
||
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.
Comment 3•7 years ago
|
||
Just cc'ing ursula, who could provide a better answer
Flags: needinfo?(tspurway) → needinfo?(usarracini)
Comment 4•7 years ago
|
||
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.
Flags: needinfo?(usarracini)
Updated•7 years ago
|
Whiteboard: [qf] → [qf:p1]
Comment 5•7 years ago
|
||
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[1] if we know the tab has become busy again. We do something similar if the tab is closed. [1]: http://searchfox.org/mozilla-central/rev/b7e531410ebc1c7d74a78e86a894e69608db318c/browser/base/content/browser-thumbnails.js#157-168
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ksteuber
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
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?
Attachment #8895111 -
Flags: feedback?(mconley)
Comment 8•7 years ago
|
||
mozreview-review |
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?
Attachment #8895111 -
Flags: review+
Updated•7 years ago
|
Attachment #8895111 -
Flags: feedback?(mconley)
Pushed by ksteuber@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1a3018f82800 Handle Browser:Thumbnail:CheckState during idle period r=mconley
Comment 10•7 years ago
|
||
Backed out for failing browser-chrome's browser_thumbnails_bug726727.js and browser_thumbnails_bug818225.js: https://hg.mozilla.org/integration/autoland/rev/c40dce56082e54b22e9ec0a81473cb3f31d5f4a3 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1a3018f828002b511b693f0508cd90a62175acc8&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=122024337&repo=autoland 09:32:01 INFO - TEST-START | toolkit/components/thumbnails/test/browser_thumbnails_bug726727.js 09:32:01 INFO - GECKO(1931) | JavaScript error: chrome://global/content/browser-child.js, line 354: NS_ERROR_PORT_ACCESS_NOT_ALLOWED: Component returned failure code: 0x804b0013 (NS_ERROR_PORT_ACCESS_NOT_ALLOWED) [nsIWebNavigation.loadURIWithOptions] 09:32:01 INFO - GECKO(1931) | JavaScript error: chrome://global/content/browser-child.js, line 562: ReferenceError: requestIdleCallback is not defined 09:32:46 INFO - TEST-INFO | started process screencapture 09:32:46 INFO - TEST-INFO | screencapture: exit 0 09:32:46 INFO - Buffered messages logged at 09:32:01 09:32:46 INFO - Console message: [JavaScript Error: "NS_ERROR_PORT_ACCESS_NOT_ALLOWED: Component returned failure code: 0x804b0013 (NS_ERROR_PORT_ACCESS_NOT_ALLOWED) [nsIWebNavigation.loadURIWithOptions]" {file: "chrome://global/content/browser-child.js" line: 354}] 09:32:46 INFO - Console message: [JavaScript Error: "ReferenceError: requestIdleCallback is not defined" {file: "chrome://global/content/browser-child.js" line: 562}] 09:32:46 INFO - Buffered messages logged at 09:32:10 <telemetry error messages> 09:32:46 INFO - Buffered messages finished 09:32:46 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/thumbnails/test/browser_thumbnails_bug726727.js | Test timed out -
Flags: needinfo?(ksteuber)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
Hopefully this version of the patch fixes this problem. Pushing to try to confirm.
Flags: needinfo?(ksteuber)
Assignee | ||
Comment 13•7 years ago
|
||
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?
Attachment #8895111 -
Flags: review?(mconley)
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8895111 [details] Bug 1376511 - Handle Browser:Thumbnail:CheckState during idle period https://reviewboard.mozilla.org/r/166226/#review171882
Attachment #8895111 -
Flags: review?(mconley) → review+
Comment 15•7 years ago
|
||
Pushed by ksteuber@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9499e1097909 Handle Browser:Thumbnail:CheckState during idle period r=mconley
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9499e1097909
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•2 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•