Last Comment Bug 661574 - Telemetry: Create a histogram directory
: Telemetry: Create a histogram directory
Status: RESOLVED FIXED
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: General (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: mozilla7
Assigned To: (dormant account)
:
Mentors:
Depends on:
Blocks: 657297 659396 661573 661881 663105 663920 665805 666522
  Show dependency treegraph
 
Reported: 2011-06-02 10:28 PDT by (dormant account)
Modified: 2012-01-01 11:29 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
move nsITelemetry to toolkit (2.04 KB, patch)
2011-06-02 15:49 PDT, (dormant account)
no flags Details | Diff | Splinter Review
telemetry directory wip (4.69 KB, patch)
2011-06-02 15:49 PDT, (dormant account)
no flags Details | Diff | Splinter Review
telemetry directory c++ side(this compiles) (7.74 KB, patch)
2011-06-02 16:30 PDT, (dormant account)
no flags Details | Diff | Splinter Review
js side (110 bytes, patch)
2011-06-02 17:10 PDT, (dormant account)
no flags Details | Diff | Splinter Review
telemetry directory(almost done) (18.61 KB, patch)
2011-06-03 16:21 PDT, (dormant account)
no flags Details | Diff | Splinter Review
part1: move nsITelemetry to toolkit (2.90 KB, patch)
2011-06-08 10:15 PDT, (dormant account)
no flags Details | Diff | Splinter Review
Telemetry directory (22.00 KB, patch)
2011-06-08 14:52 PDT, (dormant account)
no flags Details | Diff | Splinter Review
update zip probes (2.43 KB, patch)
2011-06-09 11:14 PDT, (dormant account)
mwu.code: review+
Details | Diff | Splinter Review
part1: move nsITelemetry to toolkit (3.32 KB, patch)
2011-06-09 11:21 PDT, (dormant account)
dtownsend: review+
Details | Diff | Splinter Review
part 2: Telemetry directory (21.95 KB, patch)
2011-06-09 11:23 PDT, (dormant account)
mwu.code: review+
mh+mozilla: superreview+
Details | Diff | Splinter Review
part 2: Telemetry directory (22.51 KB, patch)
2011-06-15 12:40 PDT, (dormant account)
taras.mozilla: review+
Details | Diff | Splinter Review
Part 3: Use a hashtable for id lookup (2.92 KB, patch)
2011-06-15 12:41 PDT, (dormant account)
mh+mozilla: review+
Details | Diff | Splinter Review
part 4: Get rid of unique telemetry names in favour of ids (4.05 KB, patch)
2011-06-15 12:43 PDT, (dormant account)
mh+mozilla: review+
Details | Diff | Splinter Review
Part 3: Use a hashtable for id lookup (3.05 KB, patch)
2011-06-16 15:26 PDT, (dormant account)
taras.mozilla: review+
Details | Diff | Splinter Review
part 2: Telemetry directory (22.29 KB, patch)
2011-06-17 18:11 PDT, (dormant account)
taras.mozilla: review+
Details | Diff | Splinter Review
Part 3: Use a hashtable for id lookup (3.04 KB, patch)
2011-06-17 18:13 PDT, (dormant account)
taras.mozilla: review+
Details | Diff | Splinter Review

Description (dormant account) 2011-06-02 10:28:26 PDT
Currently telemetry histograms are instantiated in an ad-hoc manner. Having a centralized directory would make it easier to document and do security review of new data points.
Comment 1 (dormant account) 2011-06-02 15:49:12 PDT
Created attachment 537007 [details] [diff] [review]
move nsITelemetry to toolkit
Comment 2 (dormant account) 2011-06-02 15:49:45 PDT
Created attachment 537008 [details] [diff] [review]
telemetry directory wip
Comment 3 (dormant account) 2011-06-02 16:30:51 PDT
Created attachment 537024 [details] [diff] [review]
telemetry directory c++ side(this compiles)
Comment 4 (dormant account) 2011-06-02 17:10:35 PDT
Created attachment 537033 [details] [diff] [review]
js side

another wip
Comment 5 (dormant account) 2011-06-02 17:11:56 PDT
Comment on attachment 537033 [details] [diff] [review]
js side

nevermind, messed up my mq
Comment 6 (dormant account) 2011-06-03 16:21:36 PDT
Created attachment 537267 [details] [diff] [review]
telemetry directory(almost done)
Comment 7 (dormant account) 2011-06-08 10:15:02 PDT
Created attachment 538047 [details] [diff] [review]
part1: move nsITelemetry to toolkit
Comment 8 (dormant account) 2011-06-08 14:52:03 PDT
Created attachment 538122 [details] [diff] [review]
Telemetry directory

This patch ended up a bit bigger than I hoped, but most of it mechanical changes. I renamed Telemetry -> TelemetryImpl, introduced a Telemetry namespace so telemetry can be used from C++ code(as opposed to using the chrome macros directly).
I added Telemetry::Accumulate(id, value) for C++ and a getHistogramById(id) to do telemetry stuff from js side. The APIs diverge slightly because this allows me to not leak chrome classes/headers into the C++ side, making it easier to utilize telemetry. The JS side is the way it is to be consistent with existing APIs.

Once this lands using chrome macros directly will be forbidden to ease security/privacy reviews(ie so they only have to look at the centralized list). In the future, I plan to add a way to iterate the histogram list for about:telemetry.
Comment 9 (dormant account) 2011-06-09 11:14:46 PDT
Created attachment 538293 [details] [diff] [review]
update zip probes
Comment 10 (dormant account) 2011-06-09 11:21:09 PDT
Created attachment 538296 [details] [diff] [review]
part1: move nsITelemetry to toolkit

This passes try
Comment 11 (dormant account) 2011-06-09 11:23:17 PDT
Created attachment 538297 [details] [diff] [review]
part 2: Telemetry directory

This passes try
Comment 12 Michael Wu [:mwu] 2011-06-09 19:11:28 PDT
Comment on attachment 538293 [details] [diff] [review]
update zip probes

This is nice.
Comment 13 Dave Townsend [:mossop] 2011-06-10 16:05:04 PDT
Comment on attachment 538297 [details] [diff] [review]
part 2: Telemetry directory

To my eye it looks ok but I'd prefer to have someone with more working C++ than me to take a look over. Shawn, would you have some time to look at this reasonably soon?

A couple of comments, don't think any of these are really required though:

* Seems like you could use a nsDataHashtable mHistogramMap which can handle nsCString keys rather than needing to jump to a char*
* Having both ID and name map to a histogram becomes a little confusing, any strong reason not to just always map from name?
Comment 14 (dormant account) 2011-06-13 10:12:29 PDT
(In reply to comment #13)
> Comment on attachment 538297 [details] [diff] [review] [review]
> part 2: Telemetry directory
> 
> To my eye it looks ok but I'd prefer to have someone with more working C++
> than me to take a look over. Shawn, would you have some time to look at this
> reasonably soon?
> 
> A couple of comments, don't think any of these are really required though:
> 
> * Seems like you could use a nsDataHashtable mHistogramMap which can handle
> nsCString keys rather than needing to jump to a char*

I chose char* so I could point at static strings. I don't see much benefit from switching to an xpcom datastructure here. The one string convertion pales in comparison to overhead of xpconnect call.

> * Having both ID and name map to a histogram becomes a little confusing, any
> strong reason not to just always map from name?

Performance. Doing probes from C++ is just a few function calls and an array lookup ie O(1), vs logN for js lookup.

Dave/Shawn, is it ok if I get glandium/mwu to review the C++ bits if Shawn can't get to this soon?
Comment 15 Dave Townsend [:mossop] 2011-06-13 10:20:02 PDT
(In reply to comment #14)
> > * Having both ID and name map to a histogram becomes a little confusing, any
> > strong reason not to just always map from name?
> 
> Performance. Doing probes from C++ is just a few function calls and an array
> lookup ie O(1), vs logN for js lookup.
> 
> Dave/Shawn, is it ok if I get glandium/mwu to review the C++ bits if Shawn
> can't get to this soon?

Yeah that'd be fine
Comment 16 (dormant account) 2011-06-13 12:15:49 PDT
Comment on attachment 538297 [details] [diff] [review]
part 2: Telemetry directory

I'll aim for 2/3 r+s for this.
Comment 17 Michael Wu [:mwu] 2011-06-14 17:41:42 PDT
Comment on attachment 538297 [details] [diff] [review]
part 2: Telemetry directory

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

Looks fine overall. Only thing I really care about is the use of abstract strings. The rest are nits.

::: toolkit/components/telemetry/Telemetry.cpp
@@ +58,3 @@
>  
> +struct ltstr
> +{

Put { on the same line as struct for consistency.

@@ +72,5 @@
>  public:
> +  static TelemetryImpl* GetSingleton();
> +
> +private:
> +  // This is used to cache JS string->TelemetryImpl::ID conversions

Telemetry::ID?

@@ +88,5 @@
> +  const char *name;
> +  PRUint32 min;
> +  PRUint32 max;
> +  PRUint32 bucket_count;
> +  PRUint32 histogram_type;

Maybe camelCase here for consistency with other mozilla things?

@@ +93,5 @@
> +};
> +
> +const TelemetryHistogram gHistograms[] = {
> +#define HISTOGRAM(id, name, min, max, bucket_count, histogram_type, b) \
> +  {NULL, NS_STRINGIFY(id), name, min, max, bucket_count, nsITelemetry::HISTOGRAM_ ## histogram_type},

Space after { and before }

@@ +102,5 @@
> +};
> +
> +nsresult
> +HistogramGet(const char *name, PRUint32 min, PRUint32 max, PRUint32 bucket_count,
> +             PRUint32 histogram_type, Histogram **aResult)

This is sorta weird mixing camelCase and c_style. Also odd that aResult is the only arg that's prefixed.

@@ +134,5 @@
> +}
> +
> +// O(1) histogram lookup by numeric id
> +nsresult
> +GetHistogramByEnumId(Telemetry::ID id, Histogram **ret)

Looks like you can just return Histogram *?

@@ +248,2 @@
>    Histogram *h;
> +  nsresult rv = HistogramGet(name.BeginReading(), min, max, bucket_count, histogram_type, &h);

PromiseFlatCString(name).get()

@@ +281,5 @@
> +{
> +  // Cache names for log(N) lookup
> +  // Note the histogram names are statically allocated
> +  if (Telemetry::HistogramCount && !mHistogramMap.size()) {
> +    for (PRUint32 i = 0;i < Telemetry::HistogramCount;i++) {

Space after the ;s

@@ +286,5 @@
> +      mHistogramMap[gHistograms[i].id] = (Telemetry::ID) i;
> +    }
> +  }
> +
> +  NameHistogramMap::iterator it = mHistogramMap.find(name.BeginReading());

PromiseFlatCString(name).get()

::: toolkit/components/telemetry/Telemetry.h
@@ +1,1 @@
> +//* -*-  Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2; -*- */

//*?

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +1,1 @@
> +//* -*-  Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2; -*- */

//*?

::: toolkit/components/telemetry/nsITelemetry.idl
@@ +80,5 @@
>    [implicit_jscontext]
>    jsval newHistogram(in ACString name, in PRUint32 min, in PRUint32 max, in PRUint32 bucket_count, in unsigned long histogram_type);
> +
> +  /**
> +   * Same as newHistogram above, but for histograms registed in TelemetryHistograms.h.

registered
Comment 18 Mike Hommey [:glandium] 2011-06-14 18:43:07 PDT
Comment on attachment 538297 [details] [diff] [review]
part 2: Telemetry directory

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

Just a few nits:

- It would be nicer if the addition of the boolean histogram type was an entirely separate patch, but either way is fine for me.

::: toolkit/components/telemetry/Telemetry.cpp
@@ +52,2 @@
>  
>  namespace {

why not under mozilla::Telemetry here too?

@@ +73,5 @@
> +  static TelemetryImpl* GetSingleton();
> +
> +private:
> +  // This is used to cache JS string->TelemetryImpl::ID conversions
> +  typedef map<const char*, Telemetry::ID, ltstr> NameHistogramMap;

Any particular reason not to use nsTHashtable?

@@ +93,5 @@
> +};
> +
> +const TelemetryHistogram gHistograms[] = {
> +#define HISTOGRAM(id, name, min, max, bucket_count, histogram_type, b) \
> +  {NULL, NS_STRINGIFY(id), name, min, max, bucket_count, nsITelemetry::HISTOGRAM_ ## histogram_type},

This adds relocations for the strings. OTOH, any alternative would be ugly :(

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +38,5 @@
> +
> +/**
> + * This file lists Telemetry histograms collected by Firefox.
> + *  Format is HISTOGRAM(id, histogram name, minium, maximum, bucket count,
> + * histogram kind, human-readable description for about:telemetry)

I'm wondering. Do we really need the histogram name ? Shouldn't the id be enough, with the human-readable description?
Comment 19 (dormant account) 2011-06-15 12:11:07 PDT
Thanks for the quick reviews.

> > +nsresult
> > +GetHistogramByEnumId(Telemetry::ID id, Histogram **ret)
> 
> Looks like you can just return Histogram *?

It's more convenient for one of the callers to do nsresult and as it passes slightly better error messages to JS.

(In reply to comment #18)
> 
> - It would be nicer if the addition of the boolean histogram type was an
> entirely separate patch, but either way is fine for me.

Sorry.

> 
> ::: toolkit/components/telemetry/Telemetry.cpp
> @@ +52,2 @@
> >  
> >  namespace {
> 
> why not under mozilla::Telemetry here too?

Because toplevel anonymous namespace helps the compiler do "static"-like things

> 
> @@ +73,5 @@
> > +  static TelemetryImpl* GetSingleton();
> > +
> > +private:
> > +  // This is used to cache JS string->TelemetryImpl::ID conversions
> > +  typedef map<const char*, Telemetry::ID, ltstr> NameHistogramMap;
> 
> Any particular reason not to use nsTHashtable?

Other than the awkward interface, no reason. Updated patch coming up.


> ::: toolkit/components/telemetry/TelemetryHistograms.h
> @@ +38,5 @@
> > +
> > +/**
> > + * This file lists Telemetry histograms collected by Firefox.
> > + *  Format is HISTOGRAM(id, histogram name, minium, maximum, bucket count,
> > + * histogram kind, human-readable description for about:telemetry)
> 
> I'm wondering. Do we really need the histogram name ? Shouldn't the id be
> enough, with the human-readable description?

You raise a really good point. Using the id gives us a benefit of guaranteeing uniqueness by the compiler(since they have to be valid enum values). I think I'll do that in a follow up. I wanted to do fancy things with formatting in the name, but the description field should be better for that anyway.
Comment 20 (dormant account) 2011-06-15 12:40:18 PDT
Created attachment 539622 [details] [diff] [review]
part 2: Telemetry directory

Updated part 2 according to review comments. By the way, really appreciate the quality of comments in this review round. Carrying over r+.

This also includes a fix for an issue mwu spotted on irc. It renames GetSingleton to GetInstance and makes it alreadyAddrefed. This avoids accidental over-addrefing that I was doing in bug 661573.
Comment 21 (dormant account) 2011-06-15 12:41:28 PDT
Created attachment 539623 [details] [diff] [review]
Part 3: Use a hashtable for id lookup
Comment 22 (dormant account) 2011-06-15 12:43:47 PDT
Created attachment 539624 [details] [diff] [review]
part 4: Get rid of unique telemetry names in favour of ids

might as well get rid of probe names in this bug
Comment 23 Michael Wu [:mwu] 2011-06-15 15:35:14 PDT
Comment on attachment 539622 [details] [diff] [review]
part 2: Telemetry directory

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

::: toolkit/components/telemetry/Telemetry.cpp
@@ +307,3 @@
>  }
>  
> +already_AddRefed<TelemetryImpl>

NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR does this for you afaict - http://hg.mozilla.org/mozilla-central/file/cab0115bbc48/xpcom/components/ModuleUtils.h#l101
Comment 24 (dormant account) 2011-06-15 16:10:34 PDT
(In reply to comment #23)
> Comment on attachment 539622 [details] [diff] [review] [review]
> part 2: Telemetry directory
> 
> Review of attachment 539622 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/Telemetry.cpp
> @@ +307,3 @@
> >  }
> >  
> > +already_AddRefed<TelemetryImpl>
> 
> NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR does this for you afaict -
> http://hg.mozilla.org/mozilla-central/file/cab0115bbc48/xpcom/components/
> ModuleUtils.h#l101

The problem is that if one returns a naked pointer in GetSingleton, then one is tempted to do GetSingleton()->foo which is normally a completely sane C++ pattern, but in this case GetSingleton() isn't a simple getter and is addrefing stuff. Wrapping it in already_AddRefed documents that and eliminates accidental misuse. Having said that, if you have any suggestions to GetSingleton/GetInstance dance in bug 661573, I'm all ears.
Comment 25 Mike Hommey [:glandium] 2011-06-15 18:35:09 PDT
Comment on attachment 539623 [details] [diff] [review]
Part 3: Use a hashtable for id lookup

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

Looks good. Note this is the typical case where it would be better to use something like gperf, but afaik, we don't do that.
Comment 26 (dormant account) 2011-06-15 18:36:05 PDT
> Looks good. Note this is the typical case where it would be better to use
> something like gperf, but afaik, we don't do that.

gperf?
Comment 27 Mike Hommey [:glandium] 2011-06-15 18:39:45 PDT
(In reply to comment #25)
> Comment on attachment 539623 [details] [diff] [review] [review]
> Part 3: Use a hashtable for id lookup
> 
> Review of attachment 539623 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Note this is the typical case where it would be better to use
> something like gperf, but afaik, we don't do that.

Actually, there is something missing: clearing the hashtable when destroying the TelemetryImpl instance.
Comment 28 Mike Hommey [:glandium] 2011-06-15 18:41:03 PDT
(In reply to comment #26)
> > Looks good. Note this is the typical case where it would be better to use
> > something like gperf, but afaik, we don't do that.
> 
> gperf?

http://www.gnu.org/software/gperf/ a perfect hash generator. It generates the hashtable at build time instead of runtime, which is good for static hashtables like that one.
Comment 29 Mike Hommey [:glandium] 2011-06-15 18:52:45 PDT
Comment on attachment 539624 [details] [diff] [review]
part 4: Get rid of unique telemetry names in favour of ids

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

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +46,5 @@
> +HISTOGRAM(TELEMETRY_PING, 1, 3000, 10, EXPONENTIAL, "Time(ms) taken to submit telemetry info")
> +HISTOGRAM(TELEMETRY_SUCCESS, 0, 1, 2, BOOLEAN,  "Success(No, Yes) rate of telemetry submissions")
> +HISTOGRAM(MEMORY_JS_GC_HEAP, 1024, 512 * 1024, 10, EXPONENTIAL, "Memory(MB) used by the JavaScript GC")
> +HISTOGRAM(MEMORY_RESIDENT, 32 * 1024, 1024 * 1024, 10, EXPONENTIAL, "Resident memory(MB) reported by OS")
> +HISTOGRAM(MEMORY_LAYOUT_ALL, 1024, 64 * 1024, 10, EXPONENTIAL, "Memory(MB) reported used by layout")

I'd put the units at the end.

::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
@@ +72,5 @@
>      do_check_eq(payload.info[f], expected_info[f]);
>    }
>  
> +  const TELEMETRY_PING = "TELEMETRY_PING";
> +  const TELEMETRY_SUCCESS = "TELEMETRY_SUCCESS";

You could just get rid of these entirely and use strings directly (especially considering they're only used once).

You also need to modify toolkit/components/telemetry/TelemetryPing.js.

Other than that, lgtm.
Comment 30 Michael Wu [:mwu] 2011-06-15 18:54:00 PDT
(In reply to comment #24)
> The problem is that if one returns a naked pointer in GetSingleton, then one
> is tempted to do GetSingleton()->foo which is normally a completely sane C++
> pattern, but in this case GetSingleton() isn't a simple getter and is
> addrefing stuff. Wrapping it in already_AddRefed documents that and
> eliminates accidental misuse. Having said that, if you have any suggestions
> to GetSingleton/GetInstance dance in bug 661573, I'm all ears.

But.. there are no users who are doing GetSingleton()->foo. Are you expecting users who will? It's fine if there will be, but I'd lean towards consistency with other GetSingleton impls if there won't be.
Comment 31 (dormant account) 2011-06-15 18:55:34 PDT
(In reply to comment #30)

> But.. there are no users who are doing GetSingleton()->foo. Are you
> expecting users who will? It's fine if there will be, but I'd lean towards
> consistency with other GetSingleton impls if there won't be.

bug 661573
Comment 32 Michael Wu [:mwu] 2011-06-15 18:58:19 PDT
(In reply to comment #20)
> This also includes a fix for an issue mwu spotted on irc. It renames
> GetSingleton to GetInstance and makes it alreadyAddrefed. This avoids
> accidental over-addrefing that I was doing in bug 661573.

The issue I was referring to was the Telemetry code adding its own reference when creating the object. Other code like the cookie service - http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#571 only addref once even if the object is being created for the first time.
Comment 33 Mike Hommey [:glandium] 2011-06-15 19:41:42 PDT
I'm not convinced we need to make it alreadyAddrefed either. And if we don't, the dance in bug 661573 is not necessary.
Comment 34 (dormant account) 2011-06-16 15:26:52 PDT
Created attachment 539904 [details] [diff] [review]
Part 3: Use a hashtable for id lookup

Added Clear to the destructor. Also initialize the hashtable to the number of elements in the enum.
Comment 35 (dormant account) 2011-06-16 15:32:42 PDT
(In reply to comment #29)

> You also need to modify toolkit/components/telemetry/TelemetryPing.js.

Are you sure? That was already using ids, unless I'm missing something
Comment 36 Mike Hommey [:glandium] 2011-06-16 16:57:29 PDT
(In reply to comment #35)
> (In reply to comment #29)
> 
> > You also need to modify toolkit/components/telemetry/TelemetryPing.js.
> 
> Are you sure? That was already using ids, unless I'm missing something

You're right, I missed the change in part 2. (you could still remove the consts from there, though)
Comment 37 (dormant account) 2011-06-17 18:11:50 PDT
Created attachment 540194 [details] [diff] [review]
part 2: Telemetry directory

Got rid of the pointless global that mwu/glandium pointed out.
Comment 38 (dormant account) 2011-06-17 18:13:11 PDT
Created attachment 540195 [details] [diff] [review]
Part 3: Use a hashtable for id lookup

Updated for changes in part 2

Note You need to log in before you can comment on or make changes to this bug.