916076 - [OS.File] Add telemetry for jank in OS.File.writeAtomic

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Calls to OS.File.writeAtomic can cause jank if the data being sent across threads is too large. We need to record Telemetry data on this.

That telemetry data should give us the duration and file name.

(marking this as main-thread-io, although it's thread communication cost rather than disk I/O)

Comment 1

[David Teller [:Yoric] - still alive but not very active]

I'll need the data soon, so grabbing bug.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: jank

As it turns out, we have performance issues caused by the time it takes to launch the OS.File worker, so I have taken the opportunity to also add Telemetry on that.

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8361644 [details] [diff] [review]
jank

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

Can you write a test for this?  Maybe just making sure that a) the histograms get populated when you launch a worker and b) that OSFILE_WORKER_READY_MS >= OSFILE_WORKER_LAUNCH_MS?

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion tests

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Attachment: Telemetry for OS.File

Same code, just changed the commit message.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8362385 [details] [diff] [review]
Companion tests

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

::: toolkit/components/osfile/tests/xpcshell/test_telemetry.js
@@ +43,5 @@
> +  do_check_eq(getCount(after[LAUNCH]), getCount(before[LAUNCH]) + 1);
> +  do_check_eq(getCount(after[READY]), getCount(before[READY]) + 1);
> +
> +  do_print("Ensuring that ready > launch");
> +  do_check_true(after[LAUNCH].sum <= after[READY].sum);

Nit: Please make the order and logic of the informative output match the order and logic of the check statement.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: Telemetry for OS.File

Trivial fix

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion tests, v2

Applied feedback. I took the opportunity to move a test from mochi/ to xpcshell/ as I had to debug it due to a typo. Self-reviewing, since it's just a move.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

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

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion tests, v3

Should work better if I don't forget one file.
Try: https://tbpl.mozilla.org/?tree=Try&rev=a97447fb87c1

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Attachment: Telemetry for OS.File, v2

Fixed another typo.
Try: https://tbpl.mozilla.org/?tree=Try&rev=6bcdeebfe326

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/701ba4d78a3b
https://hg.mozilla.org/integration/fx-team/rev/d16ca3af1e22

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/701ba4d78a3b
https://hg.mozilla.org/mozilla-central/rev/d16ca3af1e22

Comment 14

[David Teller [:Yoric] - still alive but not very active]

According to http://telemetry.mozilla.org/#nightly/32/OSFILE_WRITEATOMIC_JANK_MS, jank is quite reasonable.