Last Comment Bug 649502 - Expose histograms to JS
: Expose histograms to JS
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: ---
Assigned To: (dormant account)
:
Mentors:
Depends on: 651262
Blocks: 585196 657709
  Show dependency treegraph
 
Reported: 2011-04-12 14:45 PDT by (dormant account)
Modified: 2011-06-14 06:43 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (12.05 KB, patch)
2011-04-13 21:28 PDT, (dormant account)
no flags Details | Diff | Splinter Review
histogram support for telemetry (12.67 KB, patch)
2011-04-19 17:36 PDT, (dormant account)
no flags Details | Diff | Splinter Review
Sample visualization (61.99 KB, image/jpeg)
2011-04-20 13:06 PDT, (dormant account)
no flags Details
fix bustage on windows (1.70 KB, patch)
2011-04-25 13:45 PDT, (dormant account)
no flags Details | Diff | Splinter Review
histogram support for telemetry (14.54 KB, patch)
2011-04-26 12:34 PDT, (dormant account)
no flags Details | Diff | Splinter Review
histogram support for telemetry (14.26 KB, patch)
2011-04-26 17:18 PDT, (dormant account)
mrbkap: review+
mrbkap: superreview+
Details | Diff | Splinter Review

Description (dormant account) 2011-04-12 14:45:12 PDT
So they can be reported via telemetry
Comment 1 Nicholas Nethercote [:njn] 2011-04-13 20:02:28 PDT
Which histograms?
Comment 2 (dormant account) 2011-04-13 21:28:58 PDT
Created attachment 525925 [details] [diff] [review]
wip

Chromium histogram data structures + accompanying macros. here is a WIP.
Comment 3 (dormant account) 2011-04-19 17:36:24 PDT
Created attachment 527162 [details] [diff] [review]
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) 2011-04-20 13:06:30 PDT
Created attachment 527360 [details]
Sample visualization

Here is a picture of an addon implementing about:histogram with this api
Comment 5 (dormant account) 2011-04-25 13:45:41 PDT
Created attachment 528165 [details] [diff] [review]
fix bustage on windows

Need this to build on windows
Comment 6 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-04-26 10:36:12 PDT
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) 2011-04-26 12:34:03 PDT
Created attachment 528394 [details] [diff] [review]
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) 2011-04-26 15:58:10 PDT
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) 2011-04-26 17:18:50 PDT
Created attachment 528492 [details] [diff] [review]
histogram support for telemetry

Thanks for the quick review. I addressed your comments in this patch.
Comment 10 Blake Kaplan (:mrbkap) 2011-04-26 18:26:25 PDT
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 (⌚ UTC+1/+2) 2011-04-28 04:50:50 PDT
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) 2011-04-28 10:20:43 PDT
(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] 2011-05-11 15:33:05 PDT
(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) 2011-05-11 15:50:47 PDT
Comment on attachment 528492 [details] [diff] [review]
histogram support for telemetry

Sure!
Comment 15 (dormant account) 2011-05-13 14:20:27 PDT
I drafted up some docs at https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsITelemetry
Comment 16 Eric Shepherd [:sheppy] 2011-06-14 06:43:47 PDT
Trevor and I have tidied the doc up a bit; it's done.

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