1363877 - Label some SystemGroup runnables

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I have some patches to label a number of frequent unlabeled runnables that can be labeled with SystemGroup easily.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: label PredictorLearnRunnable

This runnable just sends an IPC message. Since it doesn't touch DOM or JS at all, SystemGroup is fine for it.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: label nsBrowserStatusFilter

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.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: label shmem created/destroyed messages

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.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: label telemetry IPC runnables as SystemGroup

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.)

Comment 5

[Chris H-C :chutten]

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.

Comment 6

[u408661]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/38eeb666a9c0
Revert "Bug 1363877 - Label IPC shmem messages as SystemGroup (r=dvander)" for valgrind failures

Comment 9

[Guilherme Lima]

The other three patches landed 10 hours ago:
https://hg.mozilla.org/mozilla-central/rev/4c3e0a17d467
https://hg.mozilla.org/mozilla-central/rev/e062bbfde215
https://hg.mozilla.org/mozilla-central/rev/263b21216568

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/465ef5e3914a
Label IPC shmem messages as SystemGroup (r=dvander)

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/465ef5e3914a