1413266 - Make sure use counters are submitted on shutdown

Status: NEW

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Georg Fritzsche [:gfritzsche]]

Per bug 1409865, comment 28 ff, we are not sure that use counters get properly reported on shutdown.

Looking at callers of nsDocument::ReportUseCounters(), nsDocument::~nsDocument() seems to be the main one.
http://searchfox.org/mozilla-central/search?q=symbol:_ZN10nsDocument17ReportUseCountersENS_20UseCounterReportKindE&redirect=false

With some initial logging/checking, we haven't seen this report values on Firefox shutdown.

This could mean under-counting of some usage scenarios.
Ideally use counters would be submitted from "profile-before-change" at the latest.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Cameron, do you know how this is supposed to work during shutdown & across processes?

Comment 2

[Cameron McCormack (:heycam)]

As far as I know we don't do anything special to account for shutdown or e10s in the use counter reporting code.  We should be invoking the nsDocument destructor for all documents before the process exits (unless there's a shutdown hang that prevents a document from being destroyed, I guess).  nsDocument::ReportUseCounters just translates to Telemetry::Accumulate calls.  Can we lose telemetry data if we call Telemetry::Accumulate too late (after "profile-before-change")?

Did your logging/checking look at the telemetry that's reported, or did you set sDebugUseCounters (in nsDocument::ReportUseCounters) to true?

Comment 3

[Georg Fritzsche [:gfritzsche]]

Attachment: trace-usecounters.patch

With the attached patch, the following behaves fine:
- start Fx
- open http://webassembly.org/demo/
- close tab
- in about:memory, minimize memory usage

Then i see the following logged:
> ...
> JavaScript warning: http://webassembly.org/demo/Tanks/Build/UnityLoader.js, line 0: Successfully compiled asm.js code
> ...
> *** nsDocument::ReportUseCounters()
> *** internal_Accumulate(CONTENT_DOCUMENTS_DESTROYED, 1)
> *** internal_Accumulate(TOP_LEVEL_CONTENT_DOCUMENTS_DESTROYED, 1)
> *** internal_Accumulate(USE_COUNTER2_JS_ASMJS_DOCUMENT, 1)
> *** internal_Accumulate(USE_COUNTER2_JS_ASMJS_PAGE, 1)
> *** internal_Accumulate(USE_COUNTER2_JS_WASM_DOCUMENT, 1)
> *** internal_Accumulate(USE_COUNTER2_JS_WASM_PAGE, 1)
> ...
> *** TelemetryHistogram::CreateHistogramSnapshots()
> ...

Now, when i do e.g.:
- start Fx
- open http://webassembly.org/demo/
- close Fx

Then i see:
> ...
> JavaScript warning: http://webassembly.org/demo/Tanks/Build/UnityLoader.js, line 0: Successfully compiled asm.js code
> ...
> *** TelemetryHistogram::CreateHistogramSnapshots()
> ...

Notably i don't see any of the relevant use counters being recorded at shutdown.
Am i missing something?

Comment 4

[Georg Fritzsche [:gfritzsche]]

If doing this in time on shutdown is not a good option, we could also think about submitting use counters and page/doc counts eagerly, on first use.

Comment 5

[Georg Fritzsche [:gfritzsche]]

(In reply to Cameron McCormack (:heycam) from comment #2)
> Can we lose telemetry data if we call
> Telemetry::Accumulate too late (after "profile-before-change")?

Yes, specifically if its submitted after the "profile-before-change-telemetry" notification (which fires after "profile-before-change").
Everything accumulated after will get stored, but doesn't get saved/send with a ping.

Comment 6

[Andrew Overholt [:overholt]]

Seems like you're working on this, Georg, so assigning to you.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Hey, i'm not working on this, just raising a potential issue with this instrumentation.
I'm happy to help figure this out.

Comment 8

[Edgar Chen [:edgar]]

We report use counters earlier a bit after bug 1568950, this bug may benefit from it.