1261052 - Move C++ histogram implementation code into a separate module

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Georg Fritzsche [:gfritzsche]]

Telemetry.cpp is a pretty big and historically grown source file.
It accumulated different Telemetry-related tasks, making it hard to keep track of what is going on.

We should break this down into more specific modules, keeping Telemetry.cpp as the central modules that ties things together and implements the nsITelemetry interface etc.

To start this off, we should move out the code that manages histograms into a separate module - that code is semantically really separate from the rest of the tasks handled in Telemetry.cpp.
This will help with dealing with bug 1258183 (which will deal with races around histogram accumulation code) by making the histogram code more contained and easier to reason about.

Comment 1

[Georg Fritzsche [:gfritzsche]]

I think the clearest name for the new module is TelemetryHistograms.h/.cpp.

We have basically three layers for the histogram logic here:
(1) public interfaces via Telemetry.h, nsITelemetry and JSHistogram_* / JSKeyedHistogram_* functions
(2) histogram implementation logic in Telemetry.cpp
(3) base histogram code forked from chromium in histogram.h/.cc

(3) should only be used internally. Checking through dxr:
https://dxr.mozilla.org/mozilla-central/search?q=base%2Fhistogram.h&redirect=false&case=true
* there are some files using it that should not, see bug 1261063
* some includes are in chromium ipc code, we won't touch that (if needed, we could look at how to disabled those)
* some files just need it for the static initializer (base::StatisticsRecorder): XPCShellImpl.cpp, nsAppRunner.cpp, nsEmbedFunctions.cpp
* ... and of course the implementation in Telemetry.cpp uses it of course internally
We could look into making base/histogram.h not publically exported everywhere to make unintended use harder, but that can be looked into later.

For (2), we want to move it into TelemetryHistograms.cpp, some refactoring for this.

For (1):
* we should move the public functions into TelemetryHistograms.h to not cause confusion by having declaration & implementation spread around
* we can include that in Telemetry.h to not break existing code
* the JSHistogram_* & JSKeyedHistograms_* could move to TelemetryHistograms.cpp
* the nsITelemetry implementation functions would stay in Telemetry.cpp and call the TelemetryHistograms interface

Comment 2

[Georg Fritzsche [:gfritzsche]]

Note that with (3) being internal, we can move the locking from histogram.h/.cc up to the TelemetryHistograms.cpp level and increase thread safety basically without performance costs.

Comment 3

[Julian Seward [:jseward]]

Attachment: bug1261052-move-Histogram-2.cset

WIP patch.  Is ugly but builds and I think it works (haven't really
tested).  It needs tidying up.

* Creates TelemetryHistogram.{cpp,h}

* Creates TelemetryID.h, for easy access to enum TelemetryID
  and makes it public (I suspect this is wrong)

* Creates singleton class TelemetryHistogramImpl as a direct analogue
  of TelemetryImpl

* Moves mCanRecordBase, mCanRecordExtended and mHistogramMap
  from class TelemetryImpl to class TelemetryHistogramImpl

* Moves all JSHistogram_*, JSKeyedHistogram_* functions to
  TelemetryHistogram.cpp

* Moves class KeyedHistogram to TelemetryHistogram.{cpp,h}

* Moves misc other minor functions into TelemetryHistogram.cpp

There are almost certainly other functions that can be moved into 
TelemetryHistogram.cpp.

I find the class name TelemetryImpl and now TelemetryHistogramImpl 
to be confusing, since they are basically singleton global variables
that hold the telemetry state.  Can we rename them to TelemetryGlobals
and TelemetryHistogramGlobals, or some such?  I think that would be
clearer.

Comment 4

[Julian Seward [:jseward]]

Georg, is this what you had in mind?  If so, what other stuff
do you think should be moved across?  Other comments?

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8742884 [details] [diff] [review]
bug1261052-move-Histogram-2.cset

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

I've been doing a first high-level pass without worrying about naming and structure yet.

From a high-level look, i think we can contain this more.
I think we can keep all usage of base::histogram and KeyedHistogram internal to TelemetryHistogram.cpp.
Keeping the implementation of JSHistogram_*() and JSKeyedHistogram_*() internal to TelemetryHistogram.cpp definitely makes sense.

With that, we could (in the succeeding bugs) keep the histogram-specific locking inside TelemetryHistogram.cpp and can drop the locking in histogram.cc.
It would also mean that a few functions in Telemetry.cpp just forward calls to functions in TelemetryHistogram.

To be more specific, we would probably need a public interface like this in TelemetryHistogram.h:
* SetHistogramRecordingEnabled()
* getters & setters for CanRecordBase/CanRecordExtended
* Accumulate() ... in different versions
* ClearHistogram()
* GetHistogramById(), GetKeyedHistogramById()
* NewHistogram(), NewKeyedHistogram()
* HistogramFrom()
* Snapshot functions for histograms / keyed histograms
* RegisteredHistograms(), RegisteredKeyedHistograms()

I don't think we should expose:
* base::Histogram
* class KeyedHistogram
* enum reflectStatus
* struct TelemetryHistogram
* KEYED_HISTOGRAM_NAME_SEPARATOR
* SUBSESSION_HISTOGRAM_PREFIX
* HistogramGet()
* GetSubsessionHistogram()
* WrapAndReturnHistogram(), WrapAndReturnKeyedHistogram()
* HistogramAdd()
* CloneHistogram()
* ReflectHistogramSnapshot()
* CheckHistogramArguments()

::: toolkit/components/telemetry/TelemetryID.h
@@ +5,5 @@
> +
> +#ifndef TelemetryID_h__
> +#define TelemetryID_h__
> +
> +// Makes Telemetry::ID available in a convenient way.

This effectively only wraps the histogram enums in the right namespace.
If we need that, lets just make gen-histogram-enum.py put the namespace into TelemetryHistogramEnums.h directly.

Comment 6

[Julian Seward [:jseward]]

Attachment: bug1261052-c6.cset

Patch which provides an interface as described in comment 5.

* Browser still appears to start and quit ok, and
  ./mach xpcshell-test toolkit/components/telemetry/tests/unit
  reports 20 passes and no failures.

* There had to be a few extra functions in the interface, that are not
  listed in comment 5.  These are mostly to do with addon histograms
  and are marked with "//!!EXTRA" in TelemetryHistogram.h.

* Code in TelemetryHistogram.cpp is divided up into sections delimited
  by comment lines and a brief description, for clarity.

* The functions exported by TelemetryHistogram (as listed in the .h
  file) are in namespace TelemetryHistogramX.  All other functions
  (that are private) are in anonymous namespace.  The trailing X is
  a temporary hack, because "TelemetryHistogram" is also the name of
  a type, confusingly.

* The singleton class TelemetryHistogramImpl in the previous version
  of the patch has been removed.  I found it excessively confusing and
  so I replaced it with a few C style global variables near the top of
  TelemetryHistogram.cpp.  This can be undone if necessary, but for
  now the simplication makes the work easier.  IMO Telemetry.cpp would
  benefit from a similar change (removal of singleton class
  TelemetryImpl.)

* The wrapping of TelemetryID.h as discussed at the end of comment 5
  is unchanged -- should be easy to fix.

* Patch requires some cleanup -- mostly, removal of commented out
  code, and fixing some FIXMEs.

Comment 7

[Julian Seward [:jseward]]

One tricky thing about this is that the change is now so massive
that hg has no chance of rebasing it in any sane way.  So it's
vulnerable to bit-rot.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8746031 [details] [diff] [review]
bug1261052-c6.cset

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

Looking mostly at the interface in TelemetryHistogram.h, this looks good.
I can read more through Telemetry.cpp & TelemetryHistogram.cpp tomorrow, but the interface mostly seems fine so far.

We should not expose the |base::Histogram| type at all though - as commented below, bug 1261063 should allow us to get rid of that.

::: toolkit/components/telemetry/Telemetry.cpp
@@ +2674,4 @@
>  }
>  
>  base::Histogram*
>  GetHistogramById(ID id)

Basing this on bug 1261063 should mean we can just remove this function.

::: toolkit/components/telemetry/Telemetry.h
@@ +17,3 @@
>  namespace base {
>    class Histogram;
>  } // namespace base

We should remove this forward declaration.
Maybe you can just base this patch on the ones in bug 1261063?
Those get rid of the exposing of base::Histogram.

::: toolkit/components/telemetry/TelemetryHistogram.h
@@ +5,5 @@
> +
> +#ifndef TelemetryHistogram_h__
> +#define TelemetryHistogram_h__
> +
> +#include "base/histogram.h"

Let's move this and the using declarations into TelemetryHistogram.cpp.

@@ +39,5 @@
> +* ReflectHistogramSnapshot()
> +* CheckHistogramArguments()
> +*/
> +
> +namespace TelemetryHistogramX {

To avoid naming conflicts we can just rename TelemetryHistogram to, say, HistogramInfo.

@@ +71,5 @@
> +                      JS::MutableHandle<JS::Value> ret);
> +
> +//!!EXTRA
> +base::Histogram*
> +GetHistogramById(mozilla::Telemetry::ID id);

We shouldn't need to export this.
All the use of base::histogram should be internal to TelemetryHistogram.cpp (bug 1261063 should enable this).

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8746031 [details] [diff] [review]
bug1261052-c6.cset

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

This looks good overall.
I'll leave more detailed looks etc. to future feedback/review iterations.

::: toolkit/components/telemetry/Telemetry.cpp
@@ -921,1 @@
>  InitHistogramRecordingEnabled()

Lets move this and kRecordingInitiallyDisabledIDs[] over to TelemetryHistogram.cpp as well.

@@ +1150,4 @@
>    // Setup of the initial recording-enabled state (for not-recording-by-default
>    // histograms) using InitHistogramRecordingEnabled() will happen after instantiating
>    // sTelemetry since it depends on the static GetKeyedHistogramById(...) - which
>    // uses the singleton instance at sTelemetry.

Lets update the comment here to what happens.

@@ +1153,5 @@
>    // uses the singleton instance at sTelemetry.
>  
>    // Some Telemetry histograms depend on the value of C++ constants and hardcode
>    // their values in Histograms.json.
>    // We add static asserts here for those values to match so that future changes

Lets move the comment and the static asserts to TelemetryHistogram InitializeGlobalState() or so.
They are histogram specific.

@@ +2522,5 @@
>  {
>    size_t n = aMallocSizeOf(this);
>  
>    // Ignore the hashtables in mAddonMap; they are not significant.
> +  n += TelemetryHistogramX::GetMapShallowSizesOfExcludingThis(aMallocSizeOf);

Is it good practice to collect this here for non-member data?
Lets check with someone who knows.

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +99,5 @@
> +   
> +bool gCanRecordBase;
> +bool gCanRecordExtended;
> +
> +HistogramMapType gHistogramMap;

I'm not super-happy how all the state is now visible to all the code in this file, i want this to be contained a bit more.
I'm fine with deferring this to other bugs though, as the scope of this bug is already large enough.

@@ +1465,5 @@
> +  gCanRecordExtended = canRecordExtended;
> +
> +  // FIXME sewardj: actually what we want is something like
> +  // gHistogramMap(Telemetry::HistogramCount) -- does that mean,
> +  // pre-set it to that size?

That is the idea, reserving space for |HistogramCount| entries.

@@ +1479,5 @@
> +#ifdef DEBUG
> +  gHistogramMap.MarkImmutable();
> +#endif
> +
> +  gKeyedHistograms.Clear();

We shouldn't need to clear gKeyedHistograms & gAddonMap.

Comment 10

[Julian Seward [:jseward]]

Attachment: bug1261052-c10.cset

Rebased, and made to depend on bug 1261063.  Addresses some of the
review comments in comments 8 and 9 and in particular removes
base::Histogram from TelemetryHistogram.h.  Not-yet done are:


::: toolkit/components/telemetry/TelemetryHistogram.h
@@ +5,5 @@
> +
> +#ifndef TelemetryHistogram_h__
> +#define TelemetryHistogram_h__
> +
> +#include "base/histogram.h"

Let's move this and the using declarations into TelemetryHistogram.cpp.


@@ +1150,4 @@
>    // Setup of the initial recording-enabled state (for not-recording-by-default
>    // histograms) using InitHistogramRecordingEnabled() will happen after instantiating
>    // sTelemetry since it depends on the static GetKeyedHistogramById(...) - which
>    // uses the singleton instance at sTelemetry.

Lets update the comment here to what happens.


@@ +1153,5 @@
>    // uses the singleton instance at sTelemetry.
>  
>    // Some Telemetry histograms depend on the value of C++ constants and hardcode
>    // their values in Histograms.json.
>    // We add static asserts here for those values to match so that future changes

Lets move the comment and the static asserts to TelemetryHistogram
InitializeGlobalState() or so.  They are histogram specific.


@@ +2522,5 @@
>  {
>    size_t n = aMallocSizeOf(this);
>  
>    // Ignore the hashtables in mAddonMap; they are not significant.
> +  n += TelemetryHistogramX::GetMapShallowSizesOfExcludingThis(aMallocSizeOf);

Is it good practice to collect this here for non-member data?
Lets check with someone who knows.


::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +99,5 @@
> +   
> +bool gCanRecordBase;
> +bool gCanRecordExtended;
> +
> +HistogramMapType gHistogramMap;

I'm not super-happy how all the state is now visible to all the code
in this file, i want this to be contained a bit more.  I'm fine with
deferring this to other bugs though, as the scope of this bug is
already large enough.


@@ +1465,5 @@
> +  gCanRecordExtended = canRecordExtended;
> +
> +  // FIXME sewardj: actually what we want is something like
> +  // gHistogramMap(Telemetry::HistogramCount) -- does that mean,
> +  // pre-set it to that size?

That is the idea, reserving space for |HistogramCount| entries.

** sewardj: I couldn't figure out how to resize an existing
HistogramMapType (which is a nsTHashtable, really).  nsTHashtable
doesn't seem to have a "resize" or "reserve" method.

Comment 11

[Julian Seward [:jseward]]

Attachment: bug1261052-c11.cset

Comment 12

[Julian Seward [:jseward]]

(In reply to Julian Seward [:jseward] from comment #11)
> Created attachment 8750294 [details] [diff] [review]
> bug1261052-c11.cset


Rebased.  Addresses all the review comments above, except the following:

> @@ +2522,5 @@
> >  {
> >    size_t n = aMallocSizeOf(this);
> >  
> >    // Ignore the hashtables in mAddonMap; they are not significant.
> > +  n += TelemetryHistogramX::GetMapShallowSizesOfExcludingThis(aMallocSizeOf);
> 
> Is it good practice to collect this here for non-member data?
> Lets check with someone who knows.

I didn't check.  But this doesn't actually change what is measured, as
far as I can see, because the previous code was measuring the size of
two global variables in the static class TelemetryImpl (mAddonMap,
mHistogramMap), and this just changes it to measure the equivalent
globals directly (gAddonMap, gHistogramMap in TelemetryHistogram.cpp).
So the returned numbers should be unchanged.


> ::: toolkit/components/telemetry/TelemetryHistogram.cpp
> @@ +99,5 @@
> > +   
> > +bool gCanRecordBase;
> > +bool gCanRecordExtended;
> > +
> > +HistogramMapType gHistogramMap;
> 
> I'm not super-happy how all the state is now visible to all the code
> in this file, i want this to be contained a bit more.  I'm fine with
> deferring this to other bugs though, as the scope of this bug is
> already large enough.

This still needs to be done.  Shall I file a followup?


> @@ +1465,5 @@
> > +  gCanRecordExtended = canRecordExtended;
> > +
> > +  // FIXME sewardj: actually what we want is something like
> > +  // gHistogramMap(Telemetry::HistogramCount) -- does that mean,
> > +  // pre-set it to that size?
> 
> That is the idea, reserving space for |HistogramCount| entries.
> 
> ** sewardj: I couldn't figure out how to resize an existing
> HistogramMapType (which is a nsTHashtable, really).  nsTHashtable
> doesn't seem to have a "resize" or "reserve" method.

.. and I still can't figure out how to do this.  Any ideas?

Comment 13

[Georg Fritzsche [:gfritzsche]]

(In reply to Julian Seward [:jseward] from comment #12)
> (In reply to Julian Seward [:jseward] from comment #11)
> > Created attachment 8750294 [details] [diff] [review]
> > bug1261052-c11.cset
> 
> 
> Rebased.  Addresses all the review comments above, except the following:
> 
> > @@ +2522,5 @@
> > >  {
> > >    size_t n = aMallocSizeOf(this);
> > >  
> > >    // Ignore the hashtables in mAddonMap; they are not significant.
> > > +  n += TelemetryHistogramX::GetMapShallowSizesOfExcludingThis(aMallocSizeOf);
> > 
> > Is it good practice to collect this here for non-member data?
> > Lets check with someone who knows.
> 
> I didn't check.  But this doesn't actually change what is measured, as
> far as I can see, because the previous code was measuring the size of
> two global variables in the static class TelemetryImpl (mAddonMap,
> mHistogramMap), and this just changes it to measure the equivalent
> globals directly (gAddonMap, gHistogramMap in TelemetryHistogram.cpp).
> So the returned numbers should be unchanged.

I think it's a bit surprising that data internal to TelemetryHistogram.cpp is counted as part of the size of the Telemetry instance from Telemetry.cpp.
That said, as the numbers won't change maybe we shouldn't worry about it for now.

> > ::: toolkit/components/telemetry/TelemetryHistogram.cpp
> > @@ +99,5 @@
> > > +   
> > > +bool gCanRecordBase;
> > > +bool gCanRecordExtended;
> > > +
> > > +HistogramMapType gHistogramMap;
> > 
> > I'm not super-happy how all the state is now visible to all the code
> > in this file, i want this to be contained a bit more.  I'm fine with
> > deferring this to other bugs though, as the scope of this bug is
> > already large enough.
> 
> This still needs to be done.  Shall I file a followup?

Yes, thanks.

> > @@ +1465,5 @@
> > > +  gCanRecordExtended = canRecordExtended;
> > > +
> > > +  // FIXME sewardj: actually what we want is something like
> > > +  // gHistogramMap(Telemetry::HistogramCount) -- does that mean,
> > > +  // pre-set it to that size?
> > 
> > That is the idea, reserving space for |HistogramCount| entries.
> > 
> > ** sewardj: I couldn't figure out how to resize an existing
> > HistogramMapType (which is a nsTHashtable, really).  nsTHashtable
> > doesn't seem to have a "resize" or "reserve" method.
> 
> .. and I still can't figure out how to do this.  Any ideas?

I don't see that exposed from nsTHashtable, only on the internal PLDHashtable (ClearAndPrepareForLength).
Can't we just do something like |gHistogramMap = HistogramMapType(HistogramCount)|?

Comment 14

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8750294 [details] [diff] [review]
bug1261052-c11.cset

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

::: toolkit/components/telemetry/Telemetry.cpp
@@ +80,5 @@
>  
>  using namespace mozilla;
>  using namespace mozilla::HangMonitor;
>  
>  using base::BooleanHistogram;

We should now remove the "base/histogram.h" include and the "base::*" types used from it.

@@ +671,5 @@
>                             sTelemetryIOObserver);
>    sTelemetryIOObserver = nullptr;
>  }
>  
> +//class KeyedHistogram;

You still need to remove the commented out information.

@@ +1028,3 @@
>  {
> +  // We expect TelemetryHistogram::InitializeGlobalState() to have been
> +  // called before we get to this point.

Should we assert that?

@@ +1903,5 @@
>    return NS_OK;
>  }
>  
> +/* static */ bool
> +TelemetryImpl::IsInitialized()

Nit: This seems to be placed a bit randomly between the CanRecordBase and CanRecordExtended functions.
Lets just move it after SetCanRecordExtended() or so.

@@ +2186,5 @@
>  {
>    MOZ_ASSERT(!sql.IsEmpty());
>    MOZ_ASSERT(!dbName.IsEmpty());
>  
> +  if (!sTelemetry || !TelemetryHistogram::CanRecordExtended())

When we add the API-level locking in TelemetryHistograms, we'd block all those CanRecordBase() & CanRecordExtended() checks on locking that is specific to histograms.
I think we should probably track the recording state here as well so we can avoid that.
Happy to take this to a follow-up bug.

@@ +2346,5 @@
>  {
>    size_t n = aMallocSizeOf(this);
>  
>    // Ignore the hashtables in mAddonMap; they are not significant.
> +  n += TelemetryHistogram::GetMapShallowSizesOfExcludingThis(aMallocSizeOf);

We should also move the later StatisticsRecorder::Histograms code etc. over to TelemetryHistogram.
This is also required to remove the "base/histogram.h" include here.

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +74,5 @@
> +};
> +
> +typedef StatisticsRecorder::Histograms::iterator HistogramIterator;
> +
> +struct AddonHistogramInfo {

Nit: Lets put this right below |struct HistogramInfo|.

@@ +104,5 @@
> +namespace {
> +
> +// Set to true once this global state has been initialized
> +bool gInitDone = false;
> +   

Nit: Trailing whitespace here and in other locations.

@@ +1492,5 @@
> +  gCanRecordExtended = canRecordExtended;
> +
> +  // jseward: we actually what we want is to pre-size the map to
> +  // Telemetry::HistogramCount, but I can't see how to do that.
> +  gHistogramMap.Clear();

Per IRC, we should probably just initialize gHistogramMap directly with the HistogramCount.
It won't actually allocate memory based on that number until the first insert into the map.

::: toolkit/components/telemetry/TelemetryHistogram.h
@@ +7,5 @@
> +#define TelemetryHistogram_h__
> +
> +#include "mozilla/TelemetryHistogramEnums.h"
> +
> +namespace TelemetryHistogram {

While i don't think we need to document all the functions here, we should add a comment documenting that:
* this module is internal to Telemetry and should not be used outside, see Telemetry.h for public functions
* it encapsulates our histogram accumulation and storage logic

@@ +25,5 @@
> +void Accumulate(mozilla::Telemetry::ID aHistogram, uint32_t aSample);
> +void Accumulate(mozilla::Telemetry::ID aID, const nsCString& aKey,
> +                                            uint32_t aSample);
> +void Accumulate(const char* name, uint32_t sample);
> +void Accumulate(const char *name, const nsCString& key, uint32_t sample);

Nit: Lets stay with one pointer style here, |const char* name|.

Comment 15

[Julian Seward [:jseward]]

Attachment: bug1261052-final-1.cset

Addresses all points in comment #14, except:

> @@ +25,5 @@
> > +void Accumulate(mozilla::Telemetry::ID aHistogram, uint32_t aSample);
> > +void Accumulate(mozilla::Telemetry::ID aID, const nsCString& aKey,
> > +                                            uint32_t aSample);
> > +void Accumulate(const char* name, uint32_t sample);
> > +void Accumulate(const char *name, const nsCString& key, uint32_t sample);
>
> Nit: Lets stay with one pointer style here, |const char* name|.

Can do, but this interface (TelemetryHistogram.h) is already mostly
done in the |T *x| rather than the |T* x| style (also |T &x| rather
than |T& x|) -- I only moved existing code around; I didn't mess with
the interface declarations.  I fixed up this particular one
(Accumulate) as you say, though.

--------

Also: per comments on irc yesterday, I checked some initialisation-sequence
details, and they look OK (unchanged from the original).

Comment 16

[Georg Fritzsche [:gfritzsche]]

(In reply to Julian Seward [:jseward] from comment #15)
> > @@ +25,5 @@
> > > +void Accumulate(mozilla::Telemetry::ID aHistogram, uint32_t aSample);
> > > +void Accumulate(mozilla::Telemetry::ID aID, const nsCString& aKey,
> > > +                                            uint32_t aSample);
> > > +void Accumulate(const char* name, uint32_t sample);
> > > +void Accumulate(const char *name, const nsCString& key, uint32_t sample);
> >
> > Nit: Lets stay with one pointer style here, |const char* name|.
> 
> Can do, but this interface (TelemetryHistogram.h) is already mostly
> done in the |T *x| rather than the |T* x| style (also |T &x| rather
> than |T& x|) -- I only moved existing code around; I didn't mess with
> the interface declarations.

Right, consistency with the rest is better. It just stood out to me there.

Comment 17

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8751278 [details] [diff] [review]
bug1261052-final-1.cset

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

There is nothing that stands out to me anymore, thanks!

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +1378,5 @@
> +
> +////////////////////////////////////////////////////////////////////////
> +////////////////////////////////////////////////////////////////////////
> +//
> +// PRIVATE: functions related to Addons

Nit: "... to addon histograms."

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bf8922cd1b7

Comment 19

[Carsten Book [:Tomcat]]

Hi Julian,

sorry had to back this out for bustage in pgo builds like https://treeherder.mozilla.org/logviewer.html#?job_id=27839523&repo=mozilla-inbound#L20228

seems the line that cause the error is 
 05:06:57 INFO - Unified_cpp_dom_base5.obj : error LNK2001: unresolved external symbol "char const * __cdecl mozilla::Telemetry::GetHistogramName(enum mozilla::Telemetry::ID)" (?GetHistogramName@Telemetry@mozilla@@YAPBDW4ID@12@@Z)

Comment 20

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d28e974d0a32

Comment 21

[Nicholas Nethercote [inactive]]

> ** sewardj: I couldn't figure out how to resize an existing
> HistogramMapType (which is a nsTHashtable, really).  nsTHashtable
> doesn't seem to have a "resize" or "reserve" method.

It doesn't. The only thing like that is you can specify an initial length. E.g. if you expect to have N entries, you pass in N as the length.

However, nsTHashtable will always resize if you remove an element and it's underloaded. So if you do a mixture of adding and removing on your way to N entries, the reserving at construction time won't have the effect you desire. But if you don't do any removes you'll be fine.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d421496a9e3

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/4d421496a9e3