Closed Bug 1363877 Opened 2 years ago Closed 2 years ago

Label some SystemGroup runnables

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(4 files)

I have some patches to label a number of frequent unlabeled runnables that can be labeled with SystemGroup easily.
This runnable just sends an IPC message. Since it doesn't touch DOM or JS at all, SystemGroup is fine for it.
Attachment #8866520 - Flags: review?(hurley)
Felipe, you've probably never looked at this code before, and you probably have ignored most of the labeling work. However, you know how this class is used, so I think you're the right reviewer.

As long as we know that the browser never calls into content JS or touches content DOM from onProgressChange or onStatusChange, then it's safe to label this runnable as SystemGroup.
Attachment #8866522 - Flags: review?(felipc)
This patch labels the shmem creation/destruction messages with the SystemGroup. I could have done this on a protocol-by-protocol basis, but it's easier to do it for all the main thread protocols at once. That's what this patch does. None of this code runs JS or touches DOM, so using SystemGroup is safe.
Attachment #8866523 - Flags: review?(dvander)
This is more code that just sends some IPC messages, so labeling with SystemGroup is safe. (See https://wiki.mozilla.org/Quantum/DOM for some background.)
Attachment #8866524 - Flags: review?(chutten)
Attachment #8866523 - Flags: review?(dvander) → review+
Comment on attachment 8866524 [details] [diff] [review]
label telemetry IPC runnables as SystemGroup

Review of attachment 8866524 [details] [diff] [review]:
-----------------------------------------------------------------

Not fully conversant with the APIs in use here, but what I read suggests this ought to be fine.

Maybe run the telemetry xpcshell tests 5x for this change. I remember some nasty intermittents the last time I was in this code.
Attachment #8866524 - Flags: review?(chutten) → review+
Comment on attachment 8866520 [details] [diff] [review]
label PredictorLearnRunnable

Review of attachment 8866520 [details] [diff] [review]:
-----------------------------------------------------------------

This seems fine to me, based on my limited understanding of runnable labeling/grouping. I "worry" (probably too strong a word) about what might happen when we start labeling cache runnables (some of which may, in fact, be related to a particular document or tab) and the interaction of re-ordering between (but not within) groups. I suspect our predictor telemetry will be enough to tell us, though, if (1) some predictor-related cache actions get mis-grouped, and (2) that mis-grouping causes enough reordering to make the predictor less useful.
Attachment #8866520 - Flags: review?(hurley) → review+
Attachment #8866522 - Flags: review?(felipc) → review+
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c3e0a17d467
Label PredictorLearnRunnable with SystemGroup (r=hurley)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e062bbfde215
Label nsBrowserStatusFilter::TimeoutHandler as SystemGroup (r=felipe)
https://hg.mozilla.org/integration/mozilla-inbound/rev/546804e76ce7
Label IPC shmem messages as SystemGroup (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/263b21216568
Label telemetry IPC with SystemGroup (r=chutten)
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/38eeb666a9c0
Revert "Bug 1363877 - Label IPC shmem messages as SystemGroup (r=dvander)" for valgrind failures
Depends on: 1366147
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/465ef5e3914a
Label IPC shmem messages as SystemGroup (r=dvander)
Status: NEW → RESOLVED
Closed: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.