Closed
Bug 1301346
Opened 8 years ago
Closed 8 years ago
Add telemetry to collect how much time was spent in beforeunload and unload handler
Categories
(Core :: DOM: Events, defect)
Core
DOM: Events
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: kanru, Assigned: kanru)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
For bug 1279293 we would like to know how much time was spent in beforeunload and unload handler during shutdown. If one tab using sync xhr takes 200ms then 25 tabs can easily make shutdown taking more than 5 seconds.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8789664 -
Flags: review?(benjamin)
Comment 2•8 years ago
|
||
mozreview-review |
Comment on attachment 8789664 [details] Bug 1301346 - Add telemetry to collect how much time was spent in handling beforeunload and unload event. https://reviewboard.mozilla.org/r/77798/#review76284 ::: dom/events/EventDispatcher.cpp:500 (Diff revision 1) > NS_ERROR_DOM_INVALID_STATE_ERR); > > + Maybe<Telemetry::AutoTimer<Telemetry::HANDLE_UNLOAD_MS>> unloadTelemetryTimer; > + Maybe<Telemetry::AutoTimer<Telemetry::HANDLE_BEFOREUNLOAD_MS>> beforeUnloadTelemetryTimer; > + > + if (aDOMEvent) { Did you actually test this, since we may not have aDOMEvent at all here, just aEvent, at least in case of unload: http://searchfox.org/mozilla-central/rev/950e13cca1fda6507dc53c243295044e8eda4493/layout/base/nsDocumentViewer.cpp#1312,1321 And we don't want slow string comparisons here. Could you put the telemetry around the place where we dispatch those events (in DocumentViewer.cpp).
Attachment #8789664 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #2) > Comment on attachment 8789664 [details] > Bug 1301346 - Add telemetry to collect how much time was spent in > beforeunload and unload handler. data-review=bsmedberg > > https://reviewboard.mozilla.org/r/77798/#review76284 > > ::: dom/events/EventDispatcher.cpp:500 > (Diff revision 1) > > NS_ERROR_DOM_INVALID_STATE_ERR); > > > > + Maybe<Telemetry::AutoTimer<Telemetry::HANDLE_UNLOAD_MS>> unloadTelemetryTimer; > > + Maybe<Telemetry::AutoTimer<Telemetry::HANDLE_BEFOREUNLOAD_MS>> beforeUnloadTelemetryTimer; > > + > > + if (aDOMEvent) { > > Did you actually test this, since we may not have aDOMEvent at all here, > just aEvent, at least in case of unload: > http://searchfox.org/mozilla-central/rev/ > 950e13cca1fda6507dc53c243295044e8eda4493/layout/base/nsDocumentViewer. > cpp#1312,1321 Yes, tested but I didn't expect that unload might come as a WidgetEvent. > And we don't want slow string comparisons here. > Could you put the telemetry around the place where we dispatch those events > (in DocumentViewer.cpp). CreateEvent has a even longer string comparisons there so I thought this might be acceptable. I could put them around the place where events are dispatched.. there shouldn't be too many, right?
Comment 4•8 years ago
|
||
I mean you should put telemetry where we dispatch the events, so to DocumentViewer.cpp
Comment 5•8 years ago
|
||
remember that 'unload' for example is used in other cases too than just page unload.
Comment 6•8 years ago
|
||
mozreview-review |
Comment on attachment 8789664 [details] Bug 1301346 - Add telemetry to collect how much time was spent in handling beforeunload and unload event. https://reviewboard.mozilla.org/r/77798/#review77066
Attachment #8789664 -
Flags: review?(benjamin)
Comment hidden (mozreview-request) |
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8789664 [details] Bug 1301346 - Add telemetry to collect how much time was spent in handling beforeunload and unload event. https://reviewboard.mozilla.org/r/77798/#review86206 Just nit about the comment, since there can be event listeners and event handlers and dispatching an event call them all. So, it is about handling the event. ::: toolkit/components/telemetry/Histograms.json:10358 (Diff revision 2) > + "expires_in_version": "55", > + "kind": "exponential", > + "high": 10000, > + "n_buckets": 50, > + "bug_numbers": [1301346], > + "description": "The time spent in the unload event handler." The time spent handling unload event. ::: toolkit/components/telemetry/Histograms.json:10367 (Diff revision 2) > + "expires_in_version": "55", > + "kind": "exponential", > + "high": 10000, > + "n_buckets": 50, > + "bug_numbers": [1301346], > + "description": "The time spent in the beforeunload event handler." The time spent handling beforeunload event.
Attachment #8789664 -
Flags: review?(bugs) → review+
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(benjamin)
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8789664 [details] Bug 1301346 - Add telemetry to collect how much time was spent in handling beforeunload and unload event. https://reviewboard.mozilla.org/r/77798/#review87748 Also, this doesn't come with any automated tests. For a short-term exploratory measurement that's fine, although you should test this by hand. If you want to extend this to a long-term metric it should have an automated test. ::: toolkit/components/telemetry/Histograms.json:10358 (Diff revision 2) > + "expires_in_version": "55", > + "kind": "exponential", > + "high": 10000, > + "n_buckets": 50, > + "bug_numbers": [1301346], > + "description": "The time spent in the unload event handler." If there are multiple handlers for the unload event, will this record a separate value for each handler, or will it record a single value across all handlers? What documents does this measure? content/chrome or both? Presumably subframes are recorded separately? Please document more of the details.
Attachment #8789664 -
Flags: review?(benjamin) → review-
Assignee | ||
Comment 10•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8789664 [details] Bug 1301346 - Add telemetry to collect how much time was spent in handling beforeunload and unload event. https://reviewboard.mozilla.org/r/77798/#review87748 I tested this by hand. I'm curious and for future references, how should one normally write tests for telemetry? > If there are multiple handlers for the unload event, will this record a separate value for each handler, or will it record a single value across all handlers? > > What documents does this measure? content/chrome or both? Presumably subframes are recorded separately? > > Please document more of the details. If there are multiple handlers for the unload event in a document, this will record a single value across all handlers. It measures all documents and subframes separately. As the intention is to collect the average time spent in handling unlload event and the possibilty of affecting content shutdown, I think it meets the needs.
Comment hidden (mozreview-request) |
Comment 12•8 years ago
|
||
mozreview-review |
Comment on attachment 8789664 [details] Bug 1301346 - Add telemetry to collect how much time was spent in handling beforeunload and unload event. https://reviewboard.mozilla.org/r/77798/#review88122
Attachment #8789664 -
Flags: review?(benjamin) → review+
Comment hidden (mozreview-request) |
Comment 14•8 years ago
|
||
Pushed by kchen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/466b0de6dfcb Add telemetry to collect how much time was spent in handling beforeunload and unload event. r=bsmedberg,smaug
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(benjamin)
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/466b0de6dfcb
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment 16•8 years ago
|
||
Track 51+ as this can help get more data for bug 1279293. Hi Kan-ru, Can we uplift this to 51 aurora?
tracking-firefox51:
--- → +
Flags: needinfo?(kchen)
Assignee | ||
Comment 17•8 years ago
|
||
(In reply to Gerry Chang [:gchang] from comment #16) > Track 51+ as this can help get more data for bug 1279293. > Hi Kan-ru, > Can we uplift this to 51 aurora? I think we don't need to uplift to 51 because ShutDownKill on Nightly represents about 50% of content crashes. Based on the current number collected by the probes I think the spent in the unload handler is hardly the main reason of the ShutDownKill.
Flags: needinfo?(kchen)
You need to log in
before you can comment on or make changes to this bug.
Description
•