Add telemetry measure for plugin process launch count and timing

RESOLVED FIXED in mozilla32

Status

()

Core
Plug-ins
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bsmedberg, Assigned: qeole)

Tracking

unspecified
mozilla32
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
In bug 1007490 we are messing with the algorithm to determine when to shut down a plugin process. In beta I'd like to consider running some a/b/c experiments to see how this affects the total number of plugin process startups and also see how bad plugin startup jank is.

I think the measurement here can just be a timer around http://hg.mozilla.org/mozilla-central/annotate/6d32bbffc7e4/dom/plugins/ipc/PluginProcessParent.cpp#l78

aklotz are you willing to take this, or perhaps mentor it if qeole is willing to give it a shot?
Flags: needinfo?(qeole)
Flags: needinfo?(aklotz)
Yes and yes! Awaiting qeole's response.
Flags: needinfo?(aklotz)
(Assignee)

Comment 2

3 years ago
It's a yes for me as well!
I'll start to look at file PluginProcessParent.cpp.
Flags: needinfo?(qeole)
qeole, take a look at https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe for an overview of how telemetry probes are added.

For this bug you'll want to use Telemetry::AutoTimer to do the timing. It is defined at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.h#94

Telemetry::AutoTimer's |id| template parameter is a histogram ID that uniquely identifies the timer. You will need to define the histogram in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Histograms.json

Feel free to ping me on IRC. aklotz in #plugins or #perf
Assignee: nobody → qeole
(Assignee)

Comment 4

3 years ago
Created attachment 8427924 [details] [diff] [review]
bug1011136-v1.patch

Attached a first patch.
It defines a new PLUGIN_STARTUP_MS exponential histogram. It is called at [1] (as suggested) and measures time for plugin startup, in milliseconds (or at least I expect it does).

Question for bsmedberg (or whoever can answer): I am not sure how exactly would the “a/b/c” experiments would be run. As I got it, so far I only have to add a timer to measure plugins start-up duration.
Will we compare collected results for different values of the timeout set in bug #1007490 or something like that?

[1] http://hg.mozilla.org/mozilla-central/annotate/6d32bbffc7e4/dom/plugins/ipc/PluginProcessParent.cpp#l78
Attachment #8427924 - Flags: feedback?(aklotz)
Flags: needinfo?(benjamin)
(Reporter)

Comment 5

3 years ago
Yes, we'll compare the results by changing the timeout in different branches of an experiment (the branch will be recorded in telemetry).
Flags: needinfo?(benjamin)
Comment on attachment 8427924 [details] [diff] [review]
bug1011136-v1.patch

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

Looks good! Do you have level 1 commit access for pushing to Try? If not, are you interested in obtaining it?
Attachment #8427924 - Flags: feedback?(aklotz) → feedback+
(Assignee)

Comment 7

3 years ago
I haven't got any access so far (this is only my second bug). But I want to learn as much as possible, including the use of Try server, so I am definitely interested.

Should I follow the procedure described at https://www.mozilla.org/hacking/committer/?
(In reply to qeole from comment #7)
> I haven't got any access so far (this is only my second bug). But I want to
> learn as much as possible, including the use of Try server, so I am
> definitely interested.
> 
> Should I follow the procedure described at
> https://www.mozilla.org/hacking/committer/?
Yes, that's the one.
(Assignee)

Comment 9

3 years ago
(In reply to Aaron Klotz [:aklotz] from comment #8)
> Yes, that's the one.

I've applied for level 1 commit access in bug #1016395
Please Aaron, can I ask you to vouch for me?
Flags: needinfo?(aklotz)
Done!
While you're waiting for L1 access, I'd suggest that you familiarize yourself with our try servers:
https://wiki.mozilla.org/ReleaseEngineering/TryServer
Flags: needinfo?(aklotz)
(Assignee)

Comment 11

3 years ago
Thanks. I read it.
I'm not familiar enough with the different tests so far to determine which ones are necessary to perform.
As this bug is closely related to bug #1007490, maybe running the same tests would be correct?
Georg Fritzsche had used the following command line:

try: -b o -p linux64,linux64-asan,linux64-st-an,linux64-valgrind,macosx64,win32 -u all -t none
(As I understand it: optimized build for provided platforms, with all unit tests but no talos)
(Assignee)

Comment 12

3 years ago
Comment on attachment 8427924 [details] [diff] [review]
bug1011136-v1.patch

Eventually I pushed to Try with command from comment #11.
Here is the resulting link: https://tbpl.mozilla.org/?tree=Try&rev=0f0e87307ede

Benjamin, would you agree to review the patch?
Attachment #8427924 - Flags: review?(benjamin)
(Assignee)

Comment 13

3 years ago
I just realized that comment at line http://hg.mozilla.org/mozilla-central/file/a3848fbadb16/dom/plugins/base/nsPluginHost.cpp#l722 should be updated (now « 30 seconds » instead of « three minutes »). Is it relevant to do it here? Or in another bug? Or should we first perform experiments through current bug to determine optimal default value for the timeout?
(Reporter)

Comment 14

3 years ago
Comment on attachment 8427924 [details] [diff] [review]
bug1011136-v1.patch

Do change the "three minutes" to "a pref-controlled delay".

This looks great, thank you!
Attachment #8427924 - Flags: review?(benjamin) → review+
(Reporter)

Updated

3 years ago
Blocks: 1018200
(Assignee)

Comment 15

3 years ago
Created attachment 8431578 [details] [diff] [review]
bug1011136-v2.patch

Attached a new version of the patch:
− I corrected and updated the header of the patch itself;
− I modified the comments in nsPluginHost.cpp as described in comment #13 and comment #14.

There is no code modification since the patch was submitted to Try (comment #12), so I ask for checkin.
Attachment #8427924 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0ef3e69d3d5
Keywords: checkin-needed
Nice! Thanks for your contribution, Qeole!
(Assignee)

Comment 18

3 years ago
It's a pleasure. :)
https://hg.mozilla.org/mozilla-central/rev/f0ef3e69d3d5
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.