Closed Bug 1384034 Opened 7 years ago Closed 7 years ago

Label UpdateTimerCallback in ServiceWorkerManager

Categories

(Core :: DOM: Service Workers, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: bevis, Assigned: bevis)

References

Details

Attachments

(1 file)

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.
We should probably switch this timer to an idle callback instead.
Let's ask Catalin to do this when he's finished his current work.
Assignee: nobody → catalin.badea392
Priority: -- → P1
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)
(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.)
NI? to clarify comment 4.
Flags: needinfo?(bkelly)
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)
File bug 1386667 for the idle dispatch stuff.
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.
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)
Attachment #8902224 - Flags: review?(bkelly) → review+
Pushed by btseng@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f1e8a77c4143 Label UpdateTimerCallback in ServiceWorkerManager. r=bkelly
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: