Closed Bug 1343729 Opened 3 years ago Closed 3 years ago

Ignore non-main-thread IPCs in the IPC_SYNC_LATENCY_MS probe

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: ehsan, Assigned: Nika)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Having things like the GMP IPC messages show up is confusing.  If we want to track the performance of OMT code we can add specialized probes for them later.
Assignee: nobody → michael
This super short patch should make us stop collecting this telemetry off of the main thread.

I'm not sure if there is any other work I should be avoiding. The latencyMs seems to be needed for a log anyways so I didn't make computing it conditional on being on the main thread.

MozReview-Commit-ID: GtsujcVJNtW
Attachment #8844688 - Flags: review?(wmccloskey)
Comment on attachment 8844688 [details] [diff] [review]
Only collect IPC_SYNC_LATENCY_MS on the main thread

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

::: build/clang-plugin/.#Utils.h
@@ +1,1 @@
> +mlayzell@skyrocket.2554:1488295635
\ No newline at end of file

Oops.

::: ipc/glue/MessageChannel.cpp
@@ +1284,1 @@
>        Telemetry::Accumulate(Telemetry::IPC_SYNC_LATENCY_MS,

We should rename this probe.
Attachment #8844688 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #2)
> ::: ipc/glue/MessageChannel.cpp
> @@ +1284,1 @@
> >        Telemetry::Accumulate(Telemetry::IPC_SYNC_LATENCY_MS,
> 
> We should rename this probe.

If we're going to rename the probe we should do bug 1337073 at the same time. I'll hold off on landing this until we figure out what we want the new parameters to be.
Yeah let's rename this once!
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bbcd60ba4e7
Only collect IPC_SYNC_LATENCY_MS on the main thread, r=billm
Comment on attachment 8844688 [details] [diff] [review]
Only collect IPC_SYNC_LATENCY_MS on the main thread

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: Less useful sync IPC telemetry on beta/aurora
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Just landed.
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]: bug 1333489 must be uplifted to beta. Also depends on bug 1337073.
[Is the change risky?]: No
[Why is the change risky/not risky?]: Renames and updates parameters for a telemetry probe.
[String changes made/needed]: None
Attachment #8844688 - Flags: approval-mozilla-beta?
Attachment #8844688 - Flags: approval-mozilla-aurora?
Comment on attachment 8844688 [details] [diff] [review]
Only collect IPC_SYNC_LATENCY_MS on the main thread

Improvement for sync telemetry, let's take this for aurora + beta 2. 
On beta, we need the patch from bug 1333489 first.
Attachment #8844688 - Flags: approval-mozilla-beta?
Attachment #8844688 - Flags: approval-mozilla-beta+
Attachment #8844688 - Flags: approval-mozilla-aurora?
Attachment #8844688 - Flags: approval-mozilla-aurora+
Setting qe-verify- based on Michael's assessment on manual testing needs (see Comment 6).
Flags: qe-verify-
https://hg.mozilla.org/mozilla-central/rev/9bbcd60ba4e7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.