Closed Bug 706647 Opened 9 years ago Closed 9 years ago
measure plugin shutdown with telemetry
While discussing bug 662444, Vlad suggested that plugins might be a contributor to slow shutdowns. We should record times for said shutdowns and--even better--correlate them with which plugins are taking that time, to identify problematic cases.
From the hang detector, plugins seem like a fairly common source of hangs, so it would be good to be able to measure that.
Whiteboard: [Snappy] → [Snappy:P1]
Nathan, if you think this a P1 please take care of this. If not, please de-assign and change to [Snappy]
Assignee: nobody → nfroyd
Here's a patch that *looks* like it ought to DTRT, but I am very unfamiliar with this code. Is this the right place to be looking, or are there other nooks and crannies to instrument due to OOPP? Also, is this shutdown code going to be called, say, when you close a tab that's using Flash? Or does the shutdown code only get called when all references to the plugin go away, i.e. when the browser closes?
Attachment #579284 - Flags: feedback?(justin.lebar+bug)
Comment on attachment 579284 [details] [diff] [review] patch I'm not familiar with plugin code, but Josh should be able to help.
Attachment #579284 - Flags: feedback?(justin.lebar+bug) → feedback?(joshmoz)
I can help here but I want to wait for bug 90268 to land. Not a strict dependency, but it changes a lot of plugin code so it'll likely impact this, and I need to give it priority. Sorry for the review delay.
Comment on attachment 579284 [details] [diff] [review] patch Bug 90268 has landed (and stuck), we're ready for this now. Patch doesn't apply any more though. Wouldn't this patch give a bad measurement if "PluginDestructionGuard::DelayDestroy" is true? You'd be measuring the time it takes us to decide to delay.
Attachment #579284 - Flags: feedback?(joshmoz) → feedback-
OK, if we don't want to measure time spent determining whether we're not or already shutting down, how about this patch instead?
Comment on attachment 601371 [details] [diff] [review] patch I assume I was mistaken for Mr. Aas.
Attachment #601371 - Flags: review?(josh) → review?(joshmoz)
Nit: updating to proper reviewer. Carrying over r+.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.