Closed
Bug 1372453
Opened 7 years ago
Closed 7 years ago
Better naming for ProxyReleaseEvent
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: bevis, Assigned: bevis)
References
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Comment 3•7 years ago
|
||
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)
Assignee | ||
Comment 4•7 years ago
|
||
Hold the review of this patch until granting r+ in patch part 1.
Attachment #8877108 -
Attachment is obsolete: true
Comment 5•7 years ago
|
||
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 7•7 years ago
|
||
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+
Assignee | ||
Comment 9•7 years ago
|
||
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)
Assignee | ||
Comment 10•7 years ago
|
||
(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•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9509d601accd
https://hg.mozilla.org/mozilla-central/rev/89963ba78c35
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•