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

Status: RESOLVED DUPLICATE of bug 1366294

(Toolkit :: Telemetry, defect, P2)

Description

[(no longer active)]

See this profile: https://perfht.ml/2nqsk1w

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

Comment 1

[(no longer active)]

Attachment: Avoid recomputing the histogram ID when it's available in the caller

Comment 2

[Chris H-C :chutten]

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!

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b21aa70d85f
Avoid recomputing the histogram ID when it's available in the caller; r=chutten

Comment 4

[Phil Ringnalda (:philor)]

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

Comment 5

[Chris H-C :chutten]

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)?

Comment 6

[(no longer active)]

(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.

Comment 7

[(no longer active)]

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.

Comment 8

[(no longer active)]

Attachment: Avoid recomputing the histogram ID when it's available in the caller

Comment 9

[Chris H-C :chutten]

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.

Comment 10

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 11

[(no longer active)]

(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.

Comment 12

[(no longer active)]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d31299b9b88e
Avoid recomputing the histogram ID when it's available in the caller; r=chutten

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cd8e3b73309
Backed out changeset d31299b9b88e for conflicting with the backout in df0936e00966

Comment 16

[(no longer active)]

Yeah I think the test failure when bug 1353295 got backed out is to be expected.  :-(

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1b7da41c6b3a

Comment 19

[Georg Fritzsche [:gfritzsche]]

Ehsan, i don't see an r+ here. Did this get sign-off on IRC?

Comment 20

[Chris H-C :chutten]

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.

Comment 21

[(no longer active)]

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...

Comment 22

[(no longer active)]

I'm backing this out.

Comment 23

[(no longer active)]

(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

Comment 24

[Georg Fritzsche [:gfritzsche]]

I'm picking this up for our work planning.
Thanks for getting this as far, we'll check into the fallout.

Comment 25

[(no longer active)]

Thank you!

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Merged backout
https://hg.mozilla.org/mozilla-central/rev/51ce1ce423df

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Ugh, stupid bugherder...

Comment 28

[Georg Fritzsche [:gfritzsche]]

With the current state of bug 1366294, can we just dupe this bug against it?

Comment 29

[Chris H-C :chutten]

Sounds fair.