1428893 - Extend the JSHistogram APIs to allow Javascript to accumulate multiple samples to a non-keyed histogram in one call

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P4)

Description

[Aditya Bharti [:adbugger]]

In bug 1364043, the C++ Telemetry::Accumulate function was extended to allow adding multiple samples to a histogram in one call.

Bug 1428885 tracks implementing the functionality for keyed histograms.

Bug 1428888 tracks implementing the same functionality for categorical histograms.

This bug is for supporting the JSHistogram APIs so that Javascript can make use of this extended functionality. See comment 27 on bug 1364043 for more information.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Chris, is this something you would mentor?

Comment 2

[Chris H-C :chutten]

Oh yes. I was giving background information to :adbugger on #telemetry earlier today.

Comment 3

[Aditya Bharti [:adbugger]]

Alright so according to the discussions with :chutten from #telemetry , the first step would be to modify the internal_JS_Histogram_Add function here [1] and check whether the incoming JS::Value *vp is a pointer to a JS array. 

So I looked at the definition of the JS::Value class here at [2] and it appears it only supports a single JS Engine value. There are functions to check for various types (such as isDouble and isString) but no convenient isArray(). I'm not really sure how to proceed from here since JS::Value seems geared for single values only. 

Maybe I could check if the JS::Value isObject(), then check if that object is of type Array and try to get the values from there? In any case, I need some more help. Actually reading the values from the JS Array was step 2 of the plan; I guess that would be pre-empted if we were to follow this approach.

Needinfoing :chutten, any help would be greatly appreciated.

[1] https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/TelemetryHistogram.cpp#1129
[2] https://searchfox.org/mozilla-central/source/js/public/Value.h#315

Side note: (may / may not be related)
---------
The closest thing I found to the isArray() function was here [3]. It seems that the JSON_API::Value can contain an array. So we definitely have support for JS arrays somewhere, just not in the JS::Value class that the internal_JS_Histogram_Add function accepts as argument.

[3] https://searchfox.org/mozilla-central/source/toolkit/components/jsoncpp/include/json/value.h#377

Comment 4

[Chris H-C :chutten]

You are exactly on the correct track. Good work!

You may find the JS documentation on MDN to be helpful. The page on JS::Value specifically identifies that it objects include arrays (and functions): https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_Reference/JS::Value 

If you search through the page for "array" you could find a link to JS_IsArrayObject[1] which is a JS API call which ought to do exactly what we need it to.

Search for others who use JS_IsArrayObject for convention on how to call it and how to access the array's values when we know it's an array.

[1]: https://searchfox.org/mozilla-central/source/js/src/jsapi.cpp#3475

Comment 5

[Aditya Bharti [:adbugger]]

Attachment: 1428893_v1.diff

First version of patch. Figured out how to make everything work: checking if the value passed in is an object, checking if the object is an array, getting individual values from the array and accumulating them.

Since this is an initial version of the patch, I've only supported an array of integers for now. The next steps will be:

1. To figure out how to most efficiently perform all the type checks that we require (possibly depending on the type of histogram) and extend to support more types of accumulations.

2. Ensure that all the elements in the array have the same type.

3. Possibly structure the code better? As of now, I'm repeating the type checks per element in a loop. Adding all the checks required will add a huge block in a loop. Plus, the function is essentially doing the same checks lower down for a single value. I wonder if putting that code into another function will help.

I've tried to catch some errors and printed them to the BrowserConsole but I'm sure I've missed a lot of type checks and error handling cases. Please call those out so I'll have a good idea of how to proceed from here. I think the basic stuff about coaxing values out from the JS::Value is done.

P.S. Those extra comments and LogToBrowserConsole statements were for debugging and I'll remove them in the final patch.

Comment 6

[Chris H-C :chutten]

Comment on attachment 8949998 [details] [diff] [review]
1428893_v1.diff

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

I'm glad to see that you have it working!

To address the type concerns, maybe we could factor the existing code that concerns itself with `uint32_t value` out to its own helper method. Then we could finally unify the logic between keyed histograms and non-keyed histograms about how we turn JSValues into uint32_t.

That should address some of your other concerns as well? The histogram type will enforce the argument type, and factoring out the logic will help with structure.

Comment 7

[Aditya Bharti [:adbugger]]

Attachment: Added more type checks into a separate function

I've added more type checks for more histogram types into a separate function. Pending some more code refactoring, I think writing tests would be the next step.

Comment 8

[Chris H-C :chutten]

Comment on attachment 8950812 [details] [diff] [review]
Added more type checks into a separate function

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

When I suggested pulling out common type checking and coercion functions I was more specifically thinking there were some common behaviours in JSHistogram_Add as already written that we could take advantage of.

Maybe most/all of lines 1150-1187[1] could be pulled out so we could ensure that array values were treated the same as if those values were added in sequential "add" calls (except maybe we'd bail on the whole array if one of the values were unacceptable).

Aside from that, you have the array iteration working properly and you're checking your errors correctly. I think writing the tests will highlight a few of the things I caught (like the missing returns), and maybe they'll catch some that I missed, too.

[1]: https://searchfox.org/mozilla-central/rev/d03ad8843e3bf2e856126bc53b0475c595e5183b/toolkit/components/telemetry/TelemetryHistogram.cpp#1150-1187

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +1126,5 @@
>  };
>  
> +/* Type checking function added for bug 1428893 */
> +bool
> +typeChecksPassed(JS::Rooted<JS::Value>& element, uint32_t histogramType)

Naming this will be important. It should have "JS" in it, and should have "Histogram" in it.

@@ +1128,5 @@
> +/* Type checking function added for bug 1428893 */
> +bool
> +typeChecksPassed(JS::Rooted<JS::Value>& element, uint32_t histogramType)
> +{
> +  if ( (histogramType == nsITelemetry::HISTOGRAM_COUNT or

Using the keywords "and" and "or" are not generally done in C++ these days. Rewrite to && and ||.

@@ +1131,5 @@
> +{
> +  if ( (histogramType == nsITelemetry::HISTOGRAM_COUNT or
> +        histogramType == nsITelemetry::HISTOGRAM_LINEAR or
> +        histogramType == nsITelemetry::HISTOGRAM_EXPONENTIAL)
> +      and (!element.isInt32()) ) {

We don't need to check that the types match. Adding a boolean JS value to a linear histogram is currently supported. We ask JS to convert it to a uint32 and we just proceed.

It is a permissive approach.

@@ +1186,5 @@
> +
> +    bool isArray = false;
> +    JS_IsArrayObject(cx, arrayObj, &isArray);
> +
> +    if (isArray) {

The convention in this file (and in many places in the codebase) is to use "early returns" so we limit the depth of blocks. Specifically in this case, it would be

if (!isArray) {
  LogToBrowserconsole(nsIScriptError::errorFlag, "The argument can't be a non-array object");
  return true;
}

// then the rest of the code.

@@ +1193,5 @@
> +        LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Failed while getting length of array in first argument"));
> +        return true;
> +      }
> +
> +      LogToBrowserConsole(nsIScriptError::warningFlag,

// TODO: debug logging, remember to remove.

@@ +1201,5 @@
> +
> +      for (uint32_t arrayIdx = 0; arrayIdx < arrayLength; arrayIdx++) {
> +        JS::Rooted<JS::Value> element(cx);
> +
> +        if (NS_WARN_IF(!JS_GetElement(cx, arrayObj, arrayIdx, &element))) {

We don't need the NS_WARN_IF, since we have a LogToBrowserConsole. (no need to double-log the problem)

@@ +1210,5 @@
> +
> +        /* Check element type */
> +        if (!typeChecksPassed(element, type)) {
> +          LogToBrowserConsole(nsIScriptError::errorFlag,
> +              NS_ConvertUTF8toUTF16(nsPrintfCString("Element at index %d failed type checks", arrayIdx)));

Need a `return true;` here, I think

@@ +1214,5 @@
> +              NS_ConvertUTF8toUTF16(nsPrintfCString("Element at index %d failed type checks", arrayIdx)));
> +        }
> +
> +        /* Convert to uint32_t for accumulation */
> +        uint32_t toAppend;

We should give it an initial value of 0.

`toAppend` describes one use for what this variable is. For variables, a good convention is to name what it _is_ first, and only if that is ambiguous name how it is used. `value` works elsewhere in this function, so that's probably good to use here as well.

@@ +1234,5 @@
> +          }
> +        } else if (!JS::ToUint32(cx, element, &toAppend)) {
> +          /* For all other histogram types, we try to convert to uint32 */
> +          LogToBrowserConsole(nsIScriptError::errorFlag,
> +              NS_ConvertUTF8toUTF16(nsPrintfCString("Failed to convert element at index %d", arrayIdx)));

Need a `return true;` after the log here, too.

@@ +1257,2 @@
>    uint32_t value = 0;
>    if ((type == nsITelemetry::HISTOGRAM_COUNT) && (args.length() == 0)) {

Can this code down here now be rewritten to take advantage of your type checking function?

That was the purpose of pulling out a common approach to checking and coercing arguments: pulling out common logic to make organizing and understanding the code easier.

Comment 9

[Aditya Bharti [:adbugger]]

Attachment: Type checking logic in a separate function

Pulled out the logic for type checking into a separate function.
Marking for review. Will get started on writing tests next.

Comment 10

[Chris H-C :chutten]

Comment on attachment 8952221 [details] [diff] [review]
Type checking logic in a separate function

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

It's looking good. It seems as though we may be able to use your utility function for arguments to JSKeyedHistogram_Add as well! (later, though. We'll need a bug for that.)

I look forward to the tests. Lots to cover here. You may want to make these "xpcshell" tests in tests/unit instead of gtests. The JS API will be easier to test that way. Please let me know if you need any help with that.

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +1126,4 @@
>  };
>  
>  bool
> +JSHistogram_TypeChecksPassed(JSContext* cx, JS::Handle<JS::Value> element, HistogramID id,

This function should probably start with internal_

Also, arguments should have the prefix "a" (aCx, aElement, aId, aHistogramType, aValue) ((this helps distinguish them from locals and members))

This function checks types, but also coerces the element to a value. Maybe a name like "internal_JSHistogram_CoerceValue" would be more descriptive? Your call.

@@ +1197,5 @@
> +    }
> +    return true;
> +  }
> +
> +  if (args[0].isObject()) {

isObject is false for String args? What about string arguments that are created using `new String()`?

(this case (using `new String()`) is probably not too important, but we should probably have consistent behaviour for it and a test)

@@ +1253,5 @@
>  
>    {
>      StaticMutexAutoLock locker(gTelemetryHistogramMutex);
>      internal_Accumulate(id, value);
>    }

Leave this extra newline of whitespace to aid reading.

Comment 11

[Aditya Bharti [:adbugger]]

(In reply to Chris H-C :chutten from comment #10)

> ::: toolkit/components/telemetry/TelemetryHistogram.cpp
> @@ +1197,5 @@
> > +    }
> > +    return true;
> > +  }
> > +
> > +  if (args[0].isObject()) {
> 
> isObject is false for String args? What about string arguments that are
> created using `new String()`?
> 
> (this case (using `new String()`) is probably not too important, but we
> should probably have consistent behaviour for it and a test)

isObject() probably returns true for String args. I haven't written any tests yet but there was some behaviour I observed on the console that would be explained by this. It occurred when I was passing in an array containing both numeric and string types. Thanks for catching this one. I'll look into it further.

I'll make the other changes and start writing tests.

Comment 12

[Aditya Bharti [:adbugger]]

Attachment: Fixed failures on pre-existing xpcshell tests

So the previous patch was failing the pre-existing xpcshell tests. Found the sources of the errors:

1. The internal_JSHistogram_Add function should always return true. In case of failure, it should report failure on the console, and still return true. The previous patch was returning false in case of error. Changed the main _Add function accordingly.

2. I forgot that categorical histograms accepted integer values as arguments. I wrote code thinking that they can only accept string values. Changed the logic in the internal_JSHistogram_TypeChecksPassed function to address this issue.

xpcshell-test was run with "./mach xpcshell-test toolkit/components/telemetry/tests/unit/".

I'm still unsure about the function name describing the type coercion. The internal_JSHistogram_TypeChecksPassed does return a bool describing whether the type checks failed or passed, so that part of the functionality is there in the name. In any case, I'll change the function name after writing new xpcshell tests.

Uploading patch since this code is now in a state where my changes are not breaking any existing functionality.

Comment 13

[Chris H-C :chutten]

Comment on attachment 8952600 [details] [diff] [review]
Fixed failures on pre-existing xpcshell tests

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

Ah, yes, sorry about that. This function should never throw. There's a comment around line 1166 that explains that it should always successfully return undefined, even if it failed, but doesn't explain _why_. 

If you're interested, the short version is that it's because important Firefox code must be able to trust that using Telemetry APIs like this will never throw.

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +1191,5 @@
> +        NS_LITERAL_STRING("Need at least one argument for non count type histogram"));
> +      return true;
> +    }
> +    // If control reaches this block, it means that we got no argument with a
> +    // HISTOGRAM_COUNT type, so we can simply accumulate 1 and exit.

You can omit this comment. The code itself is pretty clear.

@@ +1249,4 @@
>  
> +  uint32_t value = 0;
> +  if (!internal_JSHistogram_TypeChecksPassed(cx, args[0], id, type, value)) {
> +    LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Invalid argument"));

This will log twice. The first log will be from the part of TypeChecksPassed that returned false. This second one doesn't add information, and can be omitted.

Comment 14

[Aditya Bharti [:adbugger]]

(In reply to Chris H-C :chutten from comment #13)

> @@ +1249,4 @@
> >  
> > +  uint32_t value = 0;
> > +  if (!internal_JSHistogram_TypeChecksPassed(cx, args[0], id, type, value)) {
> > +    LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Invalid argument"));
> 
> This will log twice. The first log will be from the part of TypeChecksPassed
> that returned false. This second one doesn't add information, and can be
> omitted.

Oh yes. I guess I accidentally changed the line when I was refactoring. I meant to write "Invalid argument at index %d", wrapped in a nsPrintfCString. It prints 2 lines, but the extra line provides information about the first index at which an error occurred. Is that extra information worth logging twice?

Comment 15

[Aditya Bharti [:adbugger]]

Ignore that.

Comment 16

[Aditya Bharti [:adbugger]]

Attachment: Preliminary xpcshell tests

Added a few xpcshell tests. All tests passing. I'll keep adding more. Do you have any specific case in mind you don't want me to miss?

Comment 17

[Chris H-C :chutten]

Comment on attachment 8953104 [details] [diff] [review]
Preliminary xpcshell tests

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

We're changing around the type coercion code a bit, so it's important to ensure we have tests covering the corners. I think we ought to already have most of those tests already, so we can probably focus on what this code adds: array support. I would test valid and invalid array accumulations to categorical, boolean, count, and linear test histograms to cover the bases.

Comment 18

[Aditya Bharti [:adbugger]]

Attachment: Added complete xpcshell tests.

Made the required changes. Wrote separate xpcshell test cases for each type of histogram. Mainly I have tests array support and 'all-or-nothing' accumulation. When there is an invalid value, nothing will be accumulated. The type checking function has also been renamed to internal_JSHistogram_CoerceValue.

Pushed to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f73a882cd26f076af3706ea0109a1e047111288

Comment 19

[Chris H-C :chutten]

Comment on attachment 8953309 [details] [diff] [review]
Added complete xpcshell tests.

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

This isn't against the most recent code. There's no clamping from bug 1438335 in the patch.

The testing looks complete, though!

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
@@ +1112,5 @@
> +  let h = Telemetry.getHistogramById("TELEMETRY_TEST_LINEAR");
> +  h.clear();
> +
> +  // At least one invalid paramater, so no accumulations.
> +  // Valid valus in front of invalid.

*values

Comment 20

[Aditya Bharti [:adbugger]]

Attachment: Code based off latest changes

This patch is based off the latest pull from the mozilla central branch. Also contains logic for clamping high values to UINT32_MAX. Passes test_TelemetryHistograms.js xpcshell tests and Telemetry* gtests locally.

Here is the try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e3e91c28443e208aebe563b747b53068bac592e

Comment 21

[Chris H-C :chutten]

Comment on attachment 8954205 [details] [diff] [review]
Code based off latest changes

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

None of the try failures have anything to do with this patch, good!

I have a few really tiny things, and then we can give this a push! Thank you for your continued contribution.

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +1176,5 @@
> +  } else if ( aElement.isNumber() && aElement.toNumber() > UINT32_MAX ) {
> +    // JS::ToUint32 does modulo UINT32_MAX before conversion. During accumulation this can lead to
> +    // artificially smaller values. Clamp value to UINT32_MAX, warn user.
> +    aValue = UINT32_MAX;
> +    LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Clamped argument at UINT32_MAX"));

Please retain the old log string and comment.

@@ +1177,5 @@
> +    // JS::ToUint32 does modulo UINT32_MAX before conversion. During accumulation this can lead to
> +    // artificially smaller values. Clamp value to UINT32_MAX, warn user.
> +    aValue = UINT32_MAX;
> +    LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Clamped argument at UINT32_MAX"));
> +    // Type checks pass since this is not an error.

No need for the comment or the return true.

@@ +1180,5 @@
> +    LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Clamped argument at UINT32_MAX"));
> +    // Type checks pass since this is not an error.
> +    return true;
> +  } else if (!JS::ToUint32(aCx, aElement, &aValue)) {
> +    // If it's not a string then we try to convert it to uint32 to accumulate

This comment is no longer accurate. There are more cases above than just "if isString". We can omit the comment.

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
@@ +1105,5 @@
> +
> +add_task(async function test_linear_multiple_samples() {
> +  // According to telemetry.mozilla.org/histogram-simulator, bucket at
> +  // index 1 of TELEMETRY_TEST_LINEAR has max value of 268.44M
> +  let valid = [0, 1, 5, 10, 268450000, 268450000];

Can you add a value of Math.pow(2, 31) + 1 to the valid array? This will test that clamping works in the array case and should add a count to the highest bucket.

Comment 22

[Aditya Bharti [:adbugger]]

(In reply to Chris H-C :chutten from comment #21)
> Comment on attachment 8954205 [details] [diff] [review]
> ::: toolkit/components/telemetry/TelemetryHistogram.cpp
> @@ +1176,5 @@
> > +  } else if ( aElement.isNumber() && aElement.toNumber() > UINT32_MAX ) {
> > +    // JS::ToUint32 does modulo UINT32_MAX before conversion. During accumulation this can lead to
> > +    // artificially smaller values. Clamp value to UINT32_MAX, warn user.
> > +    aValue = UINT32_MAX;
> > +    LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Clamped argument at UINT32_MAX"));
> 
> Please retain the old log string and comment.

Replaced value with aValue and arg[0] with aElement in old comment and made this change.
 
> ::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
> @@ +1105,5 @@
> > +
> > +add_task(async function test_linear_multiple_samples() {
> > +  // According to telemetry.mozilla.org/histogram-simulator, bucket at
> > +  // index 1 of TELEMETRY_TEST_LINEAR has max value of 268.44M
> > +  let valid = [0, 1, 5, 10, 268450000, 268450000];
> 
> Can you add a value of Math.pow(2, 31) + 1 to the valid array? This will
> test that clamping works in the array case and should add a count to the
> highest bucket.

1. Trying to add a value of Math.pow(2, 31) + 1 ( = INT32_MAX + 2) accumulates a value of Math.pow(2, 31) - 2 ( = INT32_MAX - 1) into the sum property of the linear histogram. I don't think this is due to the array code because the same thing happens when I'm adding a single value. In fact, based on some xpcshell-test experiments, any value >= INT32_MAX is clamped to INT32_MAX - 1. Is this expected behaviour?

2. I don't understand why Math.pow(2, 31) + 1 ( = INT_MAX + 2 ) should trigger the clamping code, when we only clamp values above UINT32_MAX ( = Math.pow(2, 32) - 1 ). What am I missing?

Comment 23

[Chris H-C :chutten]

There are two stages to the clamping code. The first is the code you interacted with that clamps it to uint32_t (since that's what internal_Accumulate takes). 

The second is deeper, in internal_HistogramAdd, where we clamp it to INT_MAX.

This behaviour that your test exposes is exactly what I'm hoping to see, in other words :)

I'm sorry for the confusion. Is it clear now?

Comment 24

[Aditya Bharti [:adbugger]]

Attachment: Patch updated

Updated the test cases to account for the off-by-one behaviour.
Changed the comments and log strings appropriately.

Comment 25

[Chris H-C :chutten]

Comment on attachment 8954772 [details] [diff] [review]
Patch updated

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

Thank you! Let's get this in.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a20d57bbd1b5
Allow JSHistogram APIs to accumulate multiple values in one call. r=chutten

Comment 27

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/a20d57bbd1b5

Comment 28

[Aditya Bharti [:adbugger]]

Changing the bug summary to better describe the actual changes to the code. When I filed the bug, I mistakenly assumed that JSHistogram API would call the C++ Telemetry::Accumulate function under the hood.