Closed
Bug 1357116
Opened 8 years ago
Closed 7 years ago
kinto-offline-client.js is being loaded by the blocklist updater, and can cause us to skip a frame
Categories
(Cloud Services :: Firefox: Common, enhancement)
Cloud Services
Firefox: Common
Tracking
(firefox55 fixed)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: mconley, Assigned: leplatrem)
References
Details
Attachments
(1 file)
Here's a profile I captured: https://perf-html.io/public/20232669f468e741f89f237bc9612ec0c4792596/calltree/?range=127.2570_130.2583~128.8764_128.9363&thread=0
It looks like the blocklist updater is somehow kicking this off.
This is the sort of papercut that can cause us to periodically jank the browser UI, and we should avoid this (or do a better job of hiding it) if at all possible.
Assignee | ||
Comment 1•8 years ago
|
||
Indeed, I can confirm: https://dxr.mozilla.org/mozilla-central/rev/722fdbff1efc308a22060e75b603311d23541bb5/toolkit/mozapps/extensions/nsBlocklistService.js#606-608
We could import the module on startup, especially now that it is enabled by default.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mathieu
Comment 2•8 years ago
|
||
(In reply to Mathieu Leplatre (:leplatrem) from comment #1)
> Indeed, I can confirm:
> https://dxr.mozilla.org/mozilla-central/rev/
> 722fdbff1efc308a22060e75b603311d23541bb5/toolkit/mozapps/extensions/
> nsBlocklistService.js#606-608
>
> We could import the module on startup, especially now that it is enabled by
> default.
We usually prefer to move as much as we can off the startup path. This may be another case where we need to run something on the ui thread while it is idle (ie. we would need bug 1353206).
Reporter | ||
Comment 3•8 years ago
|
||
Alternatively, (and I'm likely exposing my ignorance of what Kinto's function is in Firefox, but) is it possible to initialize the module lazily?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•8 years ago
|
||
Mike,
I submitted a patch that loads the module lazily. Let me know if that works for you :)
Thanks for suggesting this!
Flags: needinfo?(mconley)
Reporter | ||
Comment 6•8 years ago
|
||
Thanks leplatrem - while I did advise the change, I suspect MattN would be a better reviewer here (since I'm both on PTO, and also unfamiliar with this particular component).
Flags: needinfo?(mconley)
Reporter | ||
Updated•8 years ago
|
Attachment #8859924 -
Flags: review?(mconley) → review?(MattN+bmo)
Comment 7•8 years ago
|
||
mozreview-review |
Comment on attachment 8859924 [details]
Bug 1357116 - Load the blocklist updater module lazily
https://reviewboard.mozilla.org/r/131984/#review136826
Unfortunately this won't help, the "resource://services-common/blocklist-updater.js" module (and all its dependencies) will be imported synchronously when executing BlocklistUpdater.checkVersions().
From looking in the profile in comment 0, it seems accessing anything in Kinto initializes all (or most) of it synchronously. Or at least, it's in resource://services-common/blocklist-clients.js and its dependencies that the time is spent.
Attachment #8859924 -
Flags: review-
Updated•8 years ago
|
Attachment #8859924 -
Flags: review?(MattN+bmo)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•8 years ago
|
||
Thanks for your precious feedback! I was in the dark here..
I changed several direct imports to lazy module getters. Tests seem to pass locally :)
Is there a way to also lazy load ``Cu.importGlobalProperties(["fetch"]);``?
Flags: needinfo?(florian)
Assignee | ||
Updated•8 years ago
|
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8859924 [details]
Bug 1357116 - Load the blocklist updater module lazily
https://reviewboard.mozilla.org/r/131984/#review136922
Removing review flag for now as there are several test failures on try.
The nsBlocklistService.js changes won't actually help performance, as the module will be loaded at the first BlocklistUpdater.checkVersions() call, and attempting to load again an already loaded module is cheap. But I don't mind if you keep them.
The rest of the patch looks good to me, thanks! From code inspection I think it fixes the bug (but I haven't tried to test the patch locally). I'm ok with r+'ing these changes, but I'm not officially a reviewer for services/common.
(In reply to Mathieu Leplatre (:leplatrem) from comment #9)
> Is there a way to also lazy load ``Cu.importGlobalProperties(["fetch"]);``?
XPCOMUtils.defineLazyGetter would probably work for that, but it doesn't seem useful. I think importing a global property is cheap.
Attachment #8859924 -
Flags: review?(florian)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•8 years ago
|
||
Thanks for the feedback and your help :)
> Removing review flag for now as there are several test failures on try.
Sorry for that, I now fixed the code/tests.
> I'm not officially a reviewer for services/common.
I will request a review from Mark then.
Assignee | ||
Updated•8 years ago
|
Attachment #8859924 -
Flags: review?(mgoodwin)
Comment 13•8 years ago
|
||
mozreview-review |
Comment on attachment 8859924 [details]
Bug 1357116 - Load the blocklist updater module lazily
https://reviewboard.mozilla.org/r/131984/#review137362
Looks good to me, but let's wait for Mark's review :-).
Attachment #8859924 -
Flags: review?(florian) → review+
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8859924 [details]
Bug 1357116 - Load the blocklist updater module lazily
https://reviewboard.mozilla.org/r/131984/#review138492
Attachment #8859924 -
Flags: review?(mgoodwin) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 15•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/83fe6517d5be
Load the blocklist updater module lazily r=florian,mgoodwin
Keywords: checkin-needed
Comment 16•8 years ago
|
||
Backed out for eslint failures in blocklist-clients.js:
https://hg.mozilla.org/integration/autoland/rev/ab3f508d6afd858476d61638b43a71b67f9250f2
Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=83fe6517d5bed5ee65f76618decb29389bfe383c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=95997686&repo=autoland
[task 2017-05-02T17:25:43.441953Z] TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/services/common/blocklist-clients.js:22:60 | Multiple spaces found before '"resource://services-common/kinto-offline-client.js"'. (no-multi-spaces)
[task 2017-05-02T17:25:43.442074Z] TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/services/common/blocklist-clients.js:24:60 | Multiple spaces found before '"resource://services-common/kinto-storage-adapter.js"'. (no-multi-spaces)
[task 2017-05-02T17:25:43.442153Z] TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/services/common/blocklist-clients.js:25:60 | Multiple spaces found before '"resource://gre/modules/CanonicalJSON.jsm"'. (no-multi-spaces)
Flags: needinfo?(mathieu)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(mathieu)
Keywords: checkin-needed
Comment 19•7 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4f511c0a0d70
Load the blocklist updater module lazily r=florian,mgoodwin
Keywords: checkin-needed
Comment 20•7 years ago
|
||
bugherder |
Updated•7 years ago
|
Flags: needinfo?(florian)
You need to log in
before you can comment on or make changes to this bug.
Description
•