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

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Nika, Assigned: Nika)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

MozReview-Commit-ID: 5Sc820Jh494
Attachment #8846749 - Flags: review?(wmccloskey)
Please update the telemetry descriptions so that it's clear from reading them that they are main-thread only.
MozReview-Commit-ID: 5Sc820Jh494
Attachment #8846776 - Flags: review?(wmccloskey)
Attachment #8846749 - Attachment is obsolete: true
Attachment #8846749 - Flags: review?(wmccloskey)
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.
Attachment #8846776 - Flags: review?(wmccloskey) → review+
Attachment #8846776 - Attachment is obsolete: true
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/722daec5cc3e
Make the IPC serialization/deserialization time probes main-thread-only, r=billm
https://hg.mozilla.org/mozilla-central/rev/722daec5cc3e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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
Attachment #8846879 - Flags: approval-mozilla-beta?
Attachment #8846879 - Flags: approval-mozilla-aurora?
Comment on attachment 8848165 [details] [diff] [review]
AURORA Make the IPC serialization/deserialization time probes main-thread-only

See previous request
Attachment #8848165 - Flags: approval-mozilla-aurora?
Comment on attachment 8848169 [details] [diff] [review]
BETA Make the IPC serialization/deserialization time probes main-thread-only

See previous request
Attachment #8848169 - Flags: approval-mozilla-beta?
Attachment #8846879 - Flags: approval-mozilla-beta?
Attachment #8846879 - Flags: approval-mozilla-aurora?
Comment on attachment 8848165 [details] [diff] [review]
AURORA Make the IPC serialization/deserialization time probes main-thread-only

For better collecting telemetry data. Aurora54+.
Attachment #8848165 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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!
Flags: needinfo?(michael)
(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.
Flags: needinfo?(michael)
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.
Attachment #8848169 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Setting qe-verify- based on comment 8.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.