1372453 - Better naming for ProxyReleaseEvent

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

There are several way of using this runnable.
To break down the usage, we need a better way to naming these use cases similar to bug 1372426.

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (WIP) Part 1: Support to name the callers of ProxyReleaseEvent.

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (WIP) Part 2: Name the callers of ProxyReleaseEvent.

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Part 1: Support to name the callers of ProxyReleaseEvent.

The frequency of this runnable is pretty high according to bug 1333984 comment 11 but there are lots of use cases in the source tree.
This patch is to have |const char* aName| as the first parameter in the ProxyReleaseEvent to identify the caller of ProxyReleaseEvent to further break down the statistics of this runnable from telemetry data.

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Part 2: Name the caller of ProxyReleaseEvent.

Hold the review of this patch until granting r+ in patch part 1.

Comment 5

[Nathan Froyd [:froydnj]]

I guess this is OK, though I looked at https://bugzilla.mozilla.org/show_bug.cgi?id=1372426#c0 for more context and didn't really get an explanation there.  Can you explain what the extra precision in the runnable names gives us?  billm said in bug 1372426 that it prioritizes things for labeling...but I thought giving the runnables names in the first place was "labeling".  So I am confused, and must be missing something obvious.

Comment 6

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

(In reply to Nathan Froyd [:froydnj] from comment #5)
> I guess this is OK, though I looked at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1372426#c0 for more context and
> didn't really get an explanation there.  Can you explain what the extra
> precision in the runnable names gives us?  billm said in bug 1372426 that it
> prioritizes things for labeling...but I thought giving the runnables names
> in the first place was "labeling".  So I am confused, and must be missing
> something obvious.

Labeling is the process of assigning a SchedulerGroup to a runnable (either SystemGroup or TabGroup). Usually we name runnables at the same time since it's easy. But the names are actually more useful before we've labeled runnables so we can decide which ones are the most important to label. Naming is generally really easy to do while labeling can sometimes be tricky (if the runnable touches multiple tabs or something).

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8877440 [details] [diff] [review]
(v1) Part 1: Support to name the callers of ProxyReleaseEvent.

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

Aha!  OK, then, let's go with this.

Comment 8

[Nathan Froyd [:froydnj]]

Thanks for the explanation, billm!

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8877441 [details] [diff] [review]
(v2) Part 2: Name the caller of ProxyReleaseEvent.

Now, the use of ProxyReleaseEvent will be named with a prefix "ProxyReleaseEvent for " plus:
1. ClassName::MemeberName if applicable, or
2. ClassName of the object that the PtrHolder pointing to.
So, we still in ProxyReleaseEvent category in telemetry with more detail info about the callers.

Treeherder result looks fine:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d20d89002242bb71bd7f26596bf3394cb45ef0e

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bill McCloskey (:billm) from comment #6)
> Labeling is the process of assigning a SchedulerGroup to a runnable (either
> SystemGroup or TabGroup). Usually we name runnables at the same time since
> it's easy. But the names are actually more useful before we've labeled
> runnables so we can decide which ones are the most important to label.
> Naming is generally really easy to do while labeling can sometimes be tricky
> (if the runnable touches multiple tabs or something).

Thanks for giving more context about what we are going to do, billm!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9509d601accd
Part 1: Support to name the callers of ProxyReleaseEvent. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/89963ba78c35
Part 2: Name the caller of ProxyReleaseEvent. r=billm

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9509d601accd
https://hg.mozilla.org/mozilla-central/rev/89963ba78c35