Closed Bug 1011136 Opened 6 years ago Closed 6 years ago
Add telemetry measure for plugin process launch count and timing
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?
Yes and yes! Awaiting qeole's response.
It's a yes for me as well! I'll start to look at file PluginProcessParent.cpp.
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
Attached a first patch. It defines a new PLUGIN_STARTUP_MS exponential histogram. It is called at  (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?  http://hg.mozilla.org/mozilla-central/annotate/6d32bbffc7e4/dom/plugins/ipc/PluginProcessParent.cpp#l78
Attachment #8427924 - Flags: feedback?(aklotz)
Yes, we'll compare the results by changing the timeout in different branches of an experiment (the branch will be recorded in telemetry).
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+
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.
(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?
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
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] 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)
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 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+
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
Nice! Thanks for your contribution, Qeole!
It's a pleasure. :)
Status: NEW → RESOLVED
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.