Better naming for ProxyReleaseEvent

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bevis, Assigned: bevis)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
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.
Attachment #8877107 - Attachment is obsolete: true
Attachment #8877440 - Flags: review?(nfroyd)
Hold the review of this patch until granting r+ in patch part 1.
Attachment #8877108 - Attachment is obsolete: true
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.
Flags: needinfo?(btseng)
(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 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.
Attachment #8877440 - Flags: review?(nfroyd) → review+
Thanks for the explanation, billm!
Flags: needinfo?(btseng)
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
Attachment #8877441 - Flags: review?(wmccloskey)
(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!
Attachment #8877441 - Flags: review?(wmccloskey) → review+

Comment 11

2 years ago
Pushed by btseng@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9509d601accd
https://hg.mozilla.org/mozilla-central/rev/89963ba78c35
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.