649502 - Expose histograms to JS

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[(dormant account)]

So they can be reported via telemetry

Comment 1

[Nicholas Nethercote [inactive]]

Which histograms?

Comment 2

[(dormant account)]

Attachment: wip

Chromium histogram data structures + accompanying macros. here is a WIP.

Comment 3

[(dormant account)]

Attachment: histogram support for telemetry

This lays the foundation for collecting interesting stats via histograms. I reflected chromium's excellent histogram datastructures into javascript.
This allows us to accumulate histogram data in C++ via histogram.h macros and via JavaScript.
I exposed histograms via xpconnect, but the actual objects returned are pure javascript objects + a few jsnatives. This means that within JavaScript histograms are a little slow to create and fast to use.

Comment 4

[(dormant account)]

Attachment: Sample visualization

Here is a picture of an addon implementing about:histogram with this api

Comment 5

[(dormant account)]

Attachment: fix bustage on windows

Need this to build on windows

Comment 6

[Benjamin Smedberg]

Comment on attachment 527162 [details] [diff] [review]
histogram support for telemetry

>diff --git a/xpcom/base/nsITelemetry.idl b/xpcom/base/nsITelemetry.idl

>+ * The Original Code is Mozilla Communicator client code, copied from
>+ * xpfe/appshell/public/nsIAppShellService.idl

Given that this is just how IDLs work, I wouldn't say this. But if there is substantive copying, then you need to leave the original copyright date, not 2011.

>+interface nsICmdLineService;

definitely don't want this... this interface hasn't existed since Firefox 1.5

>+[scriptable, uuid(5c9afdb5-0532-47f3-be31-79e13a6db642)]
>+interface nsITelemetry : nsISupports
>+{
>+  /*
>+   * Returns an object containing data from all of the currently registered histograms.
>+   * { name1: {data1}, name2:{data2}...}
>+   * where data is consists of the following properties:
>+   *   min - Minimal bucket size
>+   *   max - Maximum bucket size
>+   *   histogram_type - 0:Exponential, 1:Linear
>+   *   counts - array representing contents of the buckets in the histogram
>+   *   sum - sum of the bucket contents
>+   */
>+  void getHistograms();

As far as I can tell, all of these methods should be returning 'jsval', not void. mrbkap should confirm this, but you end up with the correct C++ signatures this way and don't need to hack into xpconnect to set proper return values.

>+
>+  /* 
>+   * Create and return a histogram where bucket sizes increase exponentially. Parameters:
>+   *   name - Unique histogram name
>+   *   min - Minimal bucket size
>+   *   max - Maximum bucket size
>+   *   bucket_count - number of buckets in the histogram.
>+   * The returned object has the following functions:
>+   *   add(int) - Adds an int value to the appropriate bucket
>+   *   ranges() - Returns an array with calculated bucket sizes
>+   *   snapshot() - Returns a snapshot of the histogram with the same data fields as in getHistograms()
>+   */

These params should use javadoc, e.g. @param name - Unique histogram name.

The rest of this is JSAPI stuff that really needs to be reviewed by mrbkap or gal or somebody who really knows JSAPI.

Comment 7

[(dormant account)]

Attachment: histogram support for telemetry

Updated patch that addresses bsmedberg's comments. 
Using jsvals in idl, changed one of the methods to a readonly property, fixed license comments. Got rid of 2 sources of .ranges in js objects.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 528394 [details] [diff] [review]
histogram support for telemetry

Review of attachment 528394 [details] [diff] [review]:

Why call a file nsTelemetryImpl.cpp? Why not just nsTelemetry.cpp?

::: xpcom/base/nsITelemetry.idl
@@ +39,5 @@
+#include "nsISupports.idl"
+
+[scriptable, uuid(5c9afdb5-0532-47f3-be31-79e13a6db642)]
+interface nsITelemetry : nsISupports
+{

All of the functions in this interface need a JSContext. We actually have an IDL marking for that now [implicit_jscontext] which takes a JSContext * as the final parameter of each function. It seems like you should use that here.

::: xpcom/base/nsTelemetryImpl.cpp
@@ +70,5 @@
+  if (!xpConnect)
+    return NS_ERROR_FAILURE;
+
+  nsresult rv = xpConnect->GetCurrentNativeCallContext(&ncc);
+  NS_ENSURE_SUCCESS(rv, rv);

Using [implicit_jscontext] allows you to get rid of this.

@@ +78,5 @@
+
+  rv = ncc->GetJSContext(cx);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  *obj = JS_NewObject(*cx, classp, NULL, NULL);

You need to check for failure here.

More generally, most JS_* functions can fail. If a JS_* function returns a boolean and isn't marked in the documentation as being infallible, then you have to propagate the failure return value to the caller. That's going to mean that all of the other functions that return void here (I'm thinking of FillRanges and ReflectHistogramSnapshot in particular) need to return bool. JSHistogram_Add also will need to propagate failure information.

It's probably also worth detecting when something failed and returning NS_ERROR_FAILURE from the IDL-defined functions.

@@ +152,5 @@
+    "JSHistogram",  /* name */
+    JSCLASS_HAS_PRIVATE, /* flags */
+    JS_PropertyStub, JS_PropertyStub, JS_PropertyStub, JS_StrictPropertyStub,
+    JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub, JS_FinalizeStub,
+    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL

I think you can use JSCLASS_NO_OPTIONAL_MEMBERS here.

Comment 9

[(dormant account)]

Attachment: histogram support for telemetry

Thanks for the quick review. I addressed your comments in this patch.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 528492 [details] [diff] [review]
histogram support for telemetry

Looks good. The style is odd, but I don't really know what xpcom style is supposed to look like.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

nsTelemetry.cpp should be called Telemetry.cpp, and everything in that file should be wrapped in namespace mozilla {}, and probably an inner namespace too.

Comment 12

[(dormant account)]

(In reply to comment #11)
> nsTelemetry.cpp should be called Telemetry.cpp, and everything in that file
> should be wrapped in namespace mozilla {}, and probably an inner namespace too.

good points. I'll address those in a follow up.

http://hg.mozilla.org/mozilla-central/rev/a4d5bcc4b85d

Comment 13

[Dave Townsend [:mossop]]

(In reply to comment #12)
> (In reply to comment #11)
> > nsTelemetry.cpp should be called Telemetry.cpp, and everything in that file
> > should be wrapped in namespace mozilla {}, and probably an inner namespace too.
> 
> good points. I'll address those in a follow up.

Did you file a bug for the follow up?

This patch also added an API but it's unclear whether it got super-review, Blake, are you happy to just count your review as a super-review?

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 528492 [details] [diff] [review]
histogram support for telemetry

Sure!

Comment 15

[(dormant account)]

I drafted up some docs at https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsITelemetry

Comment 16

[Eric Shepherd [:sheppy]]

Trevor and I have tidied the doc up a bit; it's done.