976000 - Collect file IO statistics after startup and before shutdown.

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Roberto Agostino Vitillo (:rvitillo)]

It would be great if we could collect data only after startup and before shutdown.

Comment 1

[Roberto Agostino Vitillo (:rvitillo)]

On second thought this feature is really necessary. We don't want to bug developers for files that are accessed only at startup or shutdown.

Comment 2

[(dormant account)]

(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #1)
> On second thought this feature is really necessary. We don't want to bug
> developers for files that are accessed only at startup or shutdown.

why not?

Comment 3

[Roberto Agostino Vitillo (:rvitillo)]

What’s the benefit of moving IO off the main-thread during shutdown? At startup, there must be a certain "minimal" amount of IO that we have to complete before the user can start to interact with the browser at all; I can see how moving off the main-thread the IO not belonging to the aforementioned set could be beneficial though. But shouldn’t we prioritise files accessed intermittently during execution as they can cause potentially more damage?

Comment 4

[(dormant account)]

I agree that main thread IO is not worse than OMT during early-startup/shutdown. However we should still push to avoid IO when when reasonable.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

I think that we can get the best of both worlds if we add information to the Telemetry.fileIOReports output that would allow us to differentiate between startup/normal/shutdown I/O. Is that acceptable to you, Roberto?

Comment 6

[Roberto Agostino Vitillo (:rvitillo)]

(In reply to Aaron Klotz [:aklotz] from comment #5)
> I think that we can get the best of both worlds if we add information to the
> Telemetry.fileIOReports output that would allow us to differentiate between
> startup/normal/shutdown I/O. Is that acceptable to you, Roberto?

That would be great.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch v1

This patch splits the telemetry I/O data into three buckets per file, where each bucket represents the stage of the process lifecycle (startup|normal|shutdown).

I chose nsAppStartup to add calls to Telemetry::LeavingStartupStage() and Telemetry::EnteringShutdownStage() because that is where the xperf probes are triggered. I would like our telemetry data to match what xperf tests would generate as closely as possible.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8391799 [details] [diff] [review]
Patch v1

Review of attachment 8391799 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/Telemetry.cpp
@@ +395,5 @@
>  };
>  
> +const char16_t* TelemetryIOInterposeObserver::sStageStrs[] = {
> +  MOZ_UTF16("startup"), MOZ_UTF16("normal"), MOZ_UTF16("shutdown")
> +};

Micro-optimization nit: Can you make this:

const char16_t ...sStageStrings[][9];

to avoid runtime relocations?  Please do rename it to sStageStrings or sStageDescriptions; we don't need to be parsimonious with our names.

And why UTF16?  Why not plain old ASCII?

@@ +507,5 @@
> +    stats[4].setNumber(fileStats.fsyncs);
> +    stats[5].setNumber(fileStats.stats);
> +
> +    // Create jsEntry as array of elements above
> +    JS::RootedObject jsStats(cx, JS_NewArrayObject(cx, stats));

Comment should now refer to jsStats.

@@ +511,5 @@
> +    JS::RootedObject jsStats(cx, JS_NewArrayObject(cx, stats));
> +    if (!jsStats) {
> +      continue;
> +    }
> +    if (!JS_DefineUCProperty(cx, jsEntry, sStageStrs[s], -1,

How many of these entries does a usual packet contain?  I'm just wondering if duplicating the stage name over many entries time many telemetry packets times... is wasting space compared to just using an integer to denote the stage.

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Nathan Froyd (:froydnj) from comment #8)
> Comment on attachment 8391799 [details] [diff] [review]
> How many of these entries does a usual packet contain?  I'm just wondering
> if duplicating the stage name over many entries time many telemetry packets
> times... is wasting space compared to just using an integer to denote the
> stage.
There's a lot of them. I'm going to implement your suggestion.

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch v2

Sending back to r? since I changed the format again.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8392467 [details] [diff] [review]
Patch v2

Review of attachment 8392467 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/Telemetry.cpp
@@ +508,3 @@
>    }
>  
> +  JS::RootedObject jsEntry(cx, JS_NewArrayObject(cx, stages));

You need to null-check jsEntry here or you're gonna have a bad time.

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch v3

Addressed comments and updated description of fileIOReports format in nsITelemetry.idl comments. Carrying forward r+.

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

https://tbpl.mozilla.org/?tree=Try&rev=af2bdedbf352

https://hg.mozilla.org/integration/mozilla-inbound/rev/38f46f772a2e

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

The format is changed such that each file's data is an array of three elements representing startup, normal operation, and shutdown, respectively. Each element is either null or a six-element array containing the same statistics as before.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/38f46f772a2e