1346866 - Make the IPC serialization/deserialization time probes main-thread-only

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Nika Layzell [:nika] (ni? for response)]

See bug 1342635 comment 14.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Make the IPC serialization/deserialization time probes main-thread-only

MozReview-Commit-ID: 5Sc820Jh494

Comment 2

[Benjamin Smedberg]

Please update the telemetry descriptions so that it's clear from reading them that they are main-thread only.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Make the IPC serialization/deserialization time probes main-thread-only

MozReview-Commit-ID: 5Sc820Jh494

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8846776 [details] [diff] [review]
Make the IPC serialization/deserialization time probes main-thread-only

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

::: toolkit/components/telemetry/Histograms.json
@@ +11137,5 @@
>      "labels": ["installAmoAccepted", "installAmoRejected", "installLocalAccepted", "installLocalRejected", "installWebAccepted", "installWebRejected", "sideloadAccepted", "sideloadRejected", "updateAccepted", "updateRejected"],
>      "description": "Results of displaying add-on installation notifications.",
>      "releaseChannelCollection": "opt-out"
>    },
> +  "IPC_READ_MAIN_LATENCY_MS": {

Can you say MAIN_THREAD instead of MAIN? Our histogram names aren't notable for their brevity, and I think this will be easier to understand.

@@ +11150,2 @@
>    },
> +  "IPC_WRITE_MAIN_LATENCY_MS": {

Same.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Make the IPC serialization/deserialization time probes main-thread-only

MozReview-Commit-ID: 5Sc820Jh494

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/722daec5cc3e
Make the IPC serialization/deserialization time probes main-thread-only, r=billm

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/722daec5cc3e

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8846879 [details] [diff] [review]
Make the IPC serialization/deserialization time probes main-thread-only

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: Not collecting useful telemetry from beta/aurora
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: In nightly
[Needs manual test from QE? If yes, steps to reproduce]: No  
[List of other uplifts needed for the feature/fix]: bug 1342635 must be uplifted before this bug
[Is the change risky?]: No
[Why is the change risky/not risky?]: Just adds a telemetry probe
[String changes made/needed]: None

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Attachment: AURORA Make the IPC serialization/deserialization time probes main-thread-only

MozReview-Commit-ID: 5Sc820Jh494

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: BETA Make the IPC serialization/deserialization time probes main-thread-only

MozReview-Commit-ID: 5Sc820Jh494

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8848165 [details] [diff] [review]
AURORA Make the IPC serialization/deserialization time probes main-thread-only

See previous request

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8848169 [details] [diff] [review]
BETA Make the IPC serialization/deserialization time probes main-thread-only

See previous request

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8848165 [details] [diff] [review]
AURORA Make the IPC serialization/deserialization time probes main-thread-only

For better collecting telemetry data. Aurora54+.

Comment 14

[Carsten Book [:Tomcat]]

the aurora patch has problems:

renamed 1346866 -> Bug-1346866---Make-the-IPC-serializationdeserializ.patch
applying Bug-1346866---Make-the-IPC-serializationdeserializ.patch
patching file ipc/chromium/src/base/pickle.cc
Hunk #1 FAILED at 452
1 out of 1 hunks FAILED -- saving rejects to file ipc/chromium/src/base/pickle.cc.rej
patching file ipc/glue/MessageChannel.cpp
Hunk #1 FAILED at 780
1 out of 1 hunks FAILED -- saving rejects to file ipc/glue/MessageChannel.cpp.rej
patching file toolkit/components/telemetry/Histograms.json
Hunk #1 FAILED at 11073
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/telemetry/Histograms.json.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh Bug-1346866---Make-the-IPC-serializationdeserializ.patch

could you take a look, thanks!

Comment 15

[Nika Layzell [:nika] (ni? for response)]

(In reply to Carsten Book [:Tomcat] from comment #14)
> could you take a look, thanks!

Did you make sure to uplift bug 1342635 first? These patches won't apply unless that has been uplifted already.

I can take a closer look if it still fails after that.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ede05196d436

Comment 17

[Liz Henry (:lizzard)]

Comment on attachment 8848169 [details] [diff] [review]
BETA Make the IPC serialization/deserialization time probes main-thread-only

Needs work from bug 1342635 to uplift first. OK for beta.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9a13e249c5e5

Comment 19

[Cornel Ionce [:noni]]

Setting qe-verify- based on comment 8.