Closed Bug 1363877 Opened 5 years ago Closed 5 years ago

Label some SystemGroup runnables


(Core :: XPCOM, enhancement)

Not set





(Reporter: billm, Assigned: billm)




(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 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
Label PredictorLearnRunnable with SystemGroup (r=hurley)
Label nsBrowserStatusFilter::TimeoutHandler as SystemGroup (r=felipe)
Label IPC shmem messages as SystemGroup (r=dvander)
Label telemetry IPC with SystemGroup (r=chutten)
Pushed by
Revert "Bug 1363877 - Label IPC shmem messages as SystemGroup (r=dvander)" for valgrind failures
Depends on: 1366147
Pushed by
Label IPC shmem messages as SystemGroup (r=dvander)
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.