Calling TelemetryHistogram::Accumulate() even by HistogramID goes through hashtable lookup

RESOLVED DUPLICATE of bug 1366294

Status

()

Toolkit
Telemetry
P2
normal
RESOLVED DUPLICATE of bug 1366294
8 months ago
5 months ago

People

(Reporter: Away for a while, Assigned: chutten)

Tracking

(Blocks: 1 bug)

unspecified
Points:
3
Dependency tree / graph

Firefox Tracking Flags

(firefox55 affected)

Details

(Whiteboard: [qf:p3] [measurement:client])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 months ago
See this profile: https://perfht.ml/2nqsk1w

This is due to <http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/toolkit/components/telemetry/TelemetryHistogram.cpp#598>.
(Reporter)

Comment 1

8 months ago
Created attachment 8851437 [details] [diff] [review]
Avoid recomputing the histogram ID when it's available in the caller
Attachment #8851437 - Flags: review?(chutten)
(Reporter)

Updated

8 months ago
Assignee: nobody → ehsan
(Assignee)

Comment 2

8 months ago
Comment on attachment 8851437 [details] [diff] [review]
Avoid recomputing the histogram ID when it's available in the caller

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

Saving cycles is fine by me!
Attachment #8851437 - Flags: review?(chutten) → review+

Comment 3

8 months ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b21aa70d85f
Avoid recomputing the histogram ID when it's available in the caller; r=chutten
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b68e1c411057 because test_TelemetryHistograms.js and test_TelemetrySession.js would rather you not do that, https://treeherder.mozilla.org/logviewer.html#?job_id=86810740&repo=mozilla-inbound
Priority: -- → P3
(Assignee)

Comment 5

8 months ago
Ack, yes, I forgot about addon-added histograms. They don't have IDs, which means that the control flow where getting an id fails, but you can still accumulate is indeed deliberate.

:Ehsan, you good to rework the patch (eventually)?
Flags: needinfo?(ehsan)
(Reporter)

Comment 6

8 months ago
(In reply to Chris H-C :chutten from comment #5)
> Ack, yes, I forgot about addon-added histograms. They don't have IDs, which
> means that the control flow where getting an id fails, but you can still
> accumulate is indeed deliberate.

Ugh, I didn't even know add-ons get to add their own histograms.  :-)

> :Ehsan, you good to rework the patch (eventually)?

Yeah, I presume we just want to preserve the previous behavior (which based on reading the code seems to be silently pretending that things are fine, but I'd like to run this under the debugger before judging too quickly) right?

I'll take a look at this again possibly next week though, I'm at a work week this week.  I'll let the needinfo linger for now.  Sorry for the lag in responding.
(Reporter)

Comment 7

8 months ago
In order to fix this, we'd need to maintain the old code path that supports adding the histogram without having an ID for it in the parent process, but instead of having a conditional branch based on the hashtable lookup which can be slow, we can do that with some code duplication, which is a bit sucky but at least not as slow.
Flags: needinfo?(ehsan)
(Reporter)

Comment 8

8 months ago
Created attachment 8853756 [details] [diff] [review]
Avoid recomputing the histogram ID when it's available in the caller
Attachment #8853756 - Flags: review?(chutten)
(Reporter)

Updated

8 months ago
Attachment #8851437 - Attachment is obsolete: true
(Assignee)

Comment 9

8 months ago
Comment on attachment 8853756 [details] [diff] [review]
Avoid recomputing the histogram ID when it's available in the caller

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

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +704,3 @@
>  internal_HistogramAdd(Histogram& histogram, int32_t value)
>  {
> +  // We only really care about the dataset of the histogram if we are not recording

The corresponding conditional for this comment has been removed.

(And, as I discovered later, the conditional is required)

@@ +1394,2 @@
>      internal_RemoteAccumulate(id, aSample);
> +  } else if (XRE_IsParentProcess()) {

In the event we have a runtime-defined histogram but extended telemetry isn't enabled, I think this will allow recording. (which it shouldn't, see "bail out" case). Need a bail in internal_HistogramAdd(Histogram, int32_t) if !CanRecordExtended.

Sadly, the tests appear to be missing this particular case (runtime-defined addons with !canRecordExtended). Please add that case to test_addons.
Attachment #8853756 - Flags: review?(chutten) → review-
(In reply to :Ehsan Akhgari (busy) from comment #6)
> (In reply to Chris H-C :chutten from comment #5)
> > Ack, yes, I forgot about addon-added histograms. They don't have IDs, which
> > means that the control flow where getting an id fails, but you can still
> > accumulate is indeed deliberate.
> 
> Ugh, I didn't even know add-ons get to add their own histograms.  :-)

We intend to remove the code for this, we should be able to remove the current code (see bug 1353295 for next steps).
It might help to wait with the patch here until after cleaning this up.
(Reporter)

Comment 11

8 months ago
(In reply to Georg Fritzsche [:gfritzsche] from comment #10)
> (In reply to :Ehsan Akhgari (busy) from comment #6)
> > (In reply to Chris H-C :chutten from comment #5)
> > > Ack, yes, I forgot about addon-added histograms. They don't have IDs, which
> > > means that the control flow where getting an id fails, but you can still
> > > accumulate is indeed deliberate.
> > 
> > Ugh, I didn't even know add-ons get to add their own histograms.  :-)
> 
> We intend to remove the code for this, we should be able to remove the
> current code (see bug 1353295 for next steps).
> It might help to wait with the patch here until after cleaning this up.

That sounds promising!  I'll wait.
Depends on: 1353295
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p1]
(Reporter)

Comment 12

7 months ago
Now thanks to bug 1353295 the original patch can be relanded! \o/

Comment 13

7 months ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d31299b9b88e
Avoid recomputing the histogram ID when it's available in the caller; r=chutten
had to back this out because this conflict with https://hg.mozilla.org/integration/mozilla-inbound/rev/df0936e009668093aee2ad44b5764e4fbcb816c6 that got backout from m-c

After this backout we had x7 failures like https://treeherder.mozilla.org/logviewer.html#?job_id=91230487&repo=mozilla-inbound so we had to back this out
Flags: needinfo?(ehsan)

Comment 15

7 months ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cd8e3b73309
Backed out changeset d31299b9b88e for conflicting with the backout in df0936e00966
(Reporter)

Comment 16

7 months ago
Yeah I think the test failure when bug 1353295 got backed out is to be expected.  :-(
Flags: needinfo?(ehsan)

Comment 17

7 months ago
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b7da41c6b3a
Avoid recomputing the histogram ID when it's available in the caller; r=chutten

Comment 18

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1b7da41c6b3a
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1358042
Depends on: 1358259
Ehsan, i don't see an r+ here. Did this get sign-off on IRC?
Flags: needinfo?(ehsan)
(Assignee)

Comment 20

7 months ago
Oops, yes, sorry. The version that was submitted was r+ but failed because of addonHistograms. With the removal of addonHistograms it is that original patch that was pushed.
Flags: needinfo?(ehsan)
(Reporter)

Comment 21

7 months ago
This is causing endless pain and misery.  Now there is bug 1358259.  There is also the possible Talos regression.  I really don't have more time to spend on this.  :-(  Declaring defeat.

Also [qf:p1] was probably a tough sell in the first place.  This does inflate the cost of telemetry collection for all places where people call Telemetry::Accumulate() where they pass a numeric ID but most of these probes are opt-in, so I'm hoping that somewhere in this code we are bailing out early in the release channel.  It may be a good idea if someone can verify that at least...
Assignee: ehsan → nobody
Whiteboard: [qf:p1] → [qf:p3]
(Reporter)

Comment 22

7 months ago
I'm backing this out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 23

7 months ago
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #22)
> I'm backing this out.

https://hg.mozilla.org/integration/mozilla-inbound/rev/51ce1ce423df854f36b89a48ab96d98b09f881de
I'm picking this up for our work planning.
Thanks for getting this as far, we'll check into the fallout.
Priority: P3 → P2
Whiteboard: [qf:p3] → [qf:p3] [measurement:client]
(Reporter)

Comment 25

7 months ago
Thank you!

Comment 26

7 months ago
backoutbugherder
Merged backout
https://hg.mozilla.org/mozilla-central/rev/51ce1ce423df
Status: REOPENED → RESOLVED
Last Resolved: 7 months ago7 months ago
Resolution: --- → FIXED
Ugh, stupid bugherder...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

7 months ago
status-firefox55: fixed → affected
Target Milestone: mozilla55 → ---
(Assignee)

Updated

7 months ago
Assignee: nobody → chutten
Priority: P2 → P1
Points: --- → 3
Depends on: 1366294
(Assignee)

Updated

6 months ago
Priority: P1 → P2
With the current state of bug 1366294, can we just dupe this bug against it?
Flags: needinfo?(chutten)
(Assignee)

Comment 29

5 months ago
Sounds fair.
Status: REOPENED → RESOLVED
Last Resolved: 7 months ago5 months ago
Flags: needinfo?(chutten)
Resolution: --- → DUPLICATE
Duplicate of bug: 1366294
You need to log in before you can comment on or make changes to this bug.