1011136 - Add telemetry measure for plugin process launch count and timing

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benjamin Smedberg]

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?

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Yes and yes! Awaiting qeole's response.

Comment 2

[Qeole]

It's a yes for me as well!
I'll start to look at file PluginProcessParent.cpp.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 4

[Qeole]

Attachment: 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

Comment 5

[Benjamin Smedberg]

Yes, we'll compare the results by changing the timeout in different branches of an experiment (the branch will be recorded in telemetry).

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

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?

Comment 7

[Qeole]

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/?

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

(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.

Comment 9

[Qeole]

(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?

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 11

[Qeole]

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 12

[Qeole]

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?

Comment 13

[Qeole]

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?

Comment 14

[Benjamin Smedberg]

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!

Comment 15

[Qeole]

Attachment: 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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0ef3e69d3d5

Comment 17

[(No longer employed by Mozilla) Aaron Klotz]

Nice! Thanks for your contribution, Qeole!

Comment 18

[Qeole]

It's a pleasure. :)

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f0ef3e69d3d5