Closed
Bug 1384034
Opened 7 years ago
Closed 7 years ago
Label UpdateTimerCallback in ServiceWorkerManager
Categories
(Core :: DOM: Service Workers, enhancement, P1)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: bevis, Assigned: bevis)
References
Details
Attachments
(1 file)
1.95 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
According to the analysis in bug 1382172 comment 6, the usage of UpdateTimerCallback is high. We need this to be labeled to gain more benefit from qDOM scheduler.
Comment 1•7 years ago
|
||
We should probably switch this timer to an idle callback instead.
Comment 2•7 years ago
|
||
Let's ask Catalin to do this when he's finished his current work.
Assignee: nobody → catalin.badea392
Priority: -- → P1
Comment 3•7 years ago
|
||
Actually, given Catalin's other work, maybe Andrew can take this? I'm hoping it's not a giant amount of work.
Flags: needinfo?(bugmail)
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Ben Kelly [:bkelly] from comment #1)
> We should probably switch this timer to an idle callback instead.
I am not pretty sure how idle callback could fix the labeling problem.
IIUC, this is to switch the timer implementation as a runnable to be dispatched via NS_IdleDispatchTo(Current)Thread() w/ or w/o a timeout.
If a timeout is specified, we still have the same problem to label the use of IdleRunnableWrapper::mTimer:
http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/xpcom/threads/nsThreadUtils.cpp#321
Furthermore, even though we enqueue this task into Idle event queue, it's still an unlabeled runnable in the IdlePriorityQueue in bug 1350432 which could still impact the scheduling. (The same impact is moved from the NormalPriorityQueue to the IdlePriorityQueue.)
Comment 6•7 years ago
|
||
The idle dispatch comment was partly an aside. We can file the idle dispatch thing as a separate bug if you prefer.
Flags: needinfo?(bkelly)
Comment 7•7 years ago
|
||
File bug 1386667 for the idle dispatch stuff.
Assignee | ||
Comment 8•7 years ago
|
||
Note: idle dispatch (bug 1386667) still can be an option to lower priority of supporting labeling this runnable to get performance benefit from the scheduler if labeling this runnable is not possible for now.
Assignee | ||
Comment 9•7 years ago
|
||
After checking the implementation of this timer callback, we could label it with SystemGroup because the main purpose of this callback is to send PServiceWorkerUpdaterConstructor message to parent without touching any web contents.
The test result in treeherder looks fine, btw:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=406a8220f8b44166085aadc07f5d52277196ce61
Assignee: catalin.badea392 → btseng
Flags: needinfo?(bugmail)
Attachment #8902224 -
Flags: review?(bkelly)
Updated•7 years ago
|
Attachment #8902224 -
Flags: review?(bkelly) → review+
Comment 10•7 years ago
|
||
Pushed by btseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1e8a77c4143
Label UpdateTimerCallback in ServiceWorkerManager. r=bkelly
Comment 11•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•