Closed Bug 1011136 Opened 6 years ago Closed 6 years ago

Add telemetry measure for plugin process launch count and timing


(Core :: Plug-ins, defect)

Not set





(Reporter: benjamin, Assigned: qeole)




(1 file, 1 obsolete file)

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

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

Telemetry::AutoTimer's |id| template parameter is a histogram ID that uniquely identifies the timer. You will need to define the histogram in

Feel free to ping me on IRC. aklotz in #plugins or #perf
Assignee: nobody → qeole
Attached patch bug1011136-v1.patch (obsolete) — Splinter Review
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?

Attachment #8427924 - Flags: feedback?(aklotz)
Flags: needinfo?(benjamin)
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]

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+
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
(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
Yes, that's the one.
(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)
While you're waiting for L1 access, I'd suggest that you familiarize yourself with our try servers:
Flags: needinfo?(aklotz)
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)
Comment on attachment 8427924 [details] [diff] [review]

Eventually I pushed to Try with command from comment #11.
Here is the resulting link:

Benjamin, would you agree to review the patch?
Attachment #8427924 - Flags: review?(benjamin)
I just realized that comment at line 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?
Comment on attachment 8427924 [details] [diff] [review]

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

This looks great, thank you!
Attachment #8427924 - Flags: review?(benjamin) → review+
Blocks: 1018200
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
Keywords: checkin-needed
Nice! Thanks for your contribution, Qeole!
It's a pleasure. :)
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.