Closed Bug 1364043 Opened 7 years ago Closed 6 years ago

Allow C++ to record multiple samples into histograms with one call

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: gfritzsche, Assigned: adbugger, Mentored)

References

Details

Attachments

(1 file, 6 obsolete files)

Bug 1322523 is looking to record multiple samples into an enumerated histogram.
It technically can do that in a loop, but that means taking the lock on TelemetryHistogram.cpp multiple times.

We should just provide a fast path for Telemetry::Accumulate() that allows passing multiple samples.
Then we can at least only take that lock once.
Maybe we could also directly add a SampleSet from the samples instead of calling Histogram::Add() repeatedly.
Mentor: gfritzsche
Blocks: 1322523
See Also: 1322523
Depends on: 1366294
I'm interested in working on this bug. Is it ok to ask to be assigned to this? 

:chutten from #telemetry has told me where to start and has given me a general overview of the Telemetry structure. I'll be doing more in-depth reading of various files on my own; is there anything specific that a beginner like me should read about in particular?
You can take a look at the patch from bug 1322523 to see how Safe Browsing is currently recording multiple samples using a for loop and Telemetry::Accumulate. 

Telemetry.h is where Telemetry::Accumulate can be found. It plumbs through to TelemetryImpl, then does the bulk of its work within TelemetryHistogram.

There are a few Telemetry::Accumulate functions. This one is mostly specific to the ones that takes a HistogramID and a single sample.

The task is to write another Telemetry::Accumulate function that takes a Histogram ID and a collection of samples (an nsTArray<uint32_t> might be the most appropriate here), plumbs that through to TelemetryHistogram where it can be implemented by taking the lock exactly once and then accumulating multiple times.

After that, we can look into Extra Credit:
 * expand to include the types that are for categorical histograms and the type that uses a key as well
 * look into the implementation of the Histogram type to see if there's an even cheaper way to pass multiple samples into storage.

But for now, start as small and as manageable as you can. (If nothing else, it'll make it easier for me to review :)
Assignee: nobody → adibhar97
Mentor: chutten
Status: NEW → ASSIGNED
Attached patch Template for further work (obsolete) — Splinter Review
Here is an initial diff of my changes. 
Few points just to check my understanding of the pipeline:

1. Define a new function header for Accumulate(id, const nsTArray<uint32_t>& samplesArray) in Telemetry.h. Here I've used a reference to a const array based on the nsTArray documentation here: 
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Arrays#Typesafe_Arrays

2. Put the function body in Telemetry.cpp. Within the body, it now uses TelemetryHistograms::Accumulate. The Telemetry::Accumulate is for external use only.

3. Add another function header in TelemetryHistograms.h. Here, it is part of the TelemetryHistograms namespace and is for internal use.

4. Similarly, put the function body in TelemetryHistograms.cpp. Acquire the lock once and use internalAccumulate to accumulate values. I've made use of the nsTArray.Length() function from the docs since sizeof will not return size of array, but the reference.

For now, I've implemented multiple samples for just the basic type of histogram. (I guess this is 'enumerated'?)
If there are no problems with this diff, will use the same approach for the other types.

Builds successfully without errors upon running ./mach toolkit/components/telemetry .
Flags: needinfo?(chutten)
The basic type of histogram can be a few things (linear, exponential, boolean, flag, categorical, count, enumerated (which is implemented as a linear histogram under the covers)), but that's not too relevant here :)

Looks good! Now to write tests :)

Take a look in toolkit/components/telemetry/tests/gtest. There is a file called TestHistograms.cpp which tests histogram accumulations by using the C++ API to accumulate to them and then using the C++ API we use to implement the JS API to check that the accumulations succeeded.

Good test cases here would include testing that Accumulate(id, samples):
1) fails appropriately if you try to accumulate to a keyed histogram
2) Behaves consistently if you try to accumulate to a categorical histogram
3) Stores the right number if we accumulate the same sample more than once (If you accumulate "2" three times to a linear histogram, the bucket that contains "2" values should have a count of "3")
4) Stores the right numbers if we accumulate different samples in the array
5) ... anything else you can think of :)

Please ask questions if you have any. A good approach to start would be to copy what another test does.
Flags: needinfo?(chutten)
I'll get started on the tests. Writing tests is completely new to me, so back to learning mode. :)

Meanwhile what do you mean by an 'appropriate' failure in this case?

(In reply to Chris H-C :chutten from comment #4)
> 1) fails appropriately if you try to accumulate to a keyed histogram

I've only implemented adding an array of samples to a simple histogram. Adding an array of samples to a keyed histogram should fail anyway because I haven't added that function signature (Accumulate(id, const nsCString& key, const nsTArray<> mSamples)) yet.
Flags: needinfo?(chutten)
(In reply to Aditya Bharti [:adbugger] from comment #5)

> Meanwhile what do you mean by an 'appropriate' failure in this case?
> 
> (In reply to Chris H-C :chutten from comment #4)
> > 1) fails appropriately if you try to accumulate to a keyed histogram

Never mind. Sorry for the spam. Just understood what you meant by that.
Flags: needinfo?(chutten)
Added a simple test for count histograms. All tests pass as of now. Where can find some detailed documentation on how to access histogram properties?

As an example, I'm trying to add a test for linear histograms. I have the following line to get the 'counts' array from the histogram:

  // Get "counts" array from histogram
  JS::RootedValue countArray(cx.GetJSContext());
  GetProperty(cx.GetJSContext(), "counts", histogram, &countArray);

How do I access individual elements (bucket values) in the counts array? Trying count[i] throws JS::RootedValue no match for operator[]. 

I also have general questions about the true/false positional arguments in functions like GetJSContext and GetAndClearHistogram. While not extremely important for progress right now, I think it'll be a good idea to understand the testing procedure better. Where can I find documentation for these?
Attachment #8938029 - Attachment is obsolete: true
Flags: needinfo?(chutten)
Comment on attachment 8939409 [details] [diff] [review]
Added a simple test for count histograms

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

To access the elements of an array by index, you'll be using a jsapi function (specifically JS_GetElement): https://searchfox.org/mozilla-central/source/js/src/jsapi.h#3022

If you'll be iterating over the array, you may also be interested in JS_GetArrayLength: https://searchfox.org/mozilla-central/source/js/src/jsapi.h#3318

As for the bool positional parameters, they are most often `is_keyed` which is `false` if the histogram is a "normal" histogram and `true` if the histogram is a "keyed" histogram. There is documentation about keyed histograms[1], but the short-form explanation is "instead of one histogram, a keyed histogram is actually many identical histograms indexed by string keys".

[1]: https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/histograms.html#keyed-histograms

::: toolkit/components/telemetry/Telemetry.h
@@ +59,5 @@
>  void Accumulate(HistogramID id, uint32_t sample);
>  
>  /**
> + * Adds an array of samples to a histogram defined in TelemetryHistograms.h
> + * TODO: Bug 1364043

Remember to remove the TODO in the final patch.

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +1874,4 @@
>    internal_Accumulate(aID, aSample);
>  }
>  
> +void // TODO: Bug 1364043

Remember to remove the TODO in the final patch.

@@ +1882,5 @@
> +    return;
> +  }
> +
> +  StaticMutexAutoLock locker(gTelemetryHistogramMutex);
> +  for(int i = 0; i < mSamples.Length(); i++){

I believe for nsTArray we support the new-style C++ for loop like

for (uint32_t sample: mSamples) { ... }

This form is preferred as it forces the compiler to write the bounds checks instead of a human.

::: toolkit/components/telemetry/TelemetryHistogram.h
@@ +36,4 @@
>  nsresult SetHistogramRecordingEnabled(const nsACString &id, bool aEnabled);
>  
>  void Accumulate(mozilla::Telemetry::HistogramID aHistogram, uint32_t aSample);
> +void Accumulate(mozilla::Telemetry::HistogramID aHistogram, const nsTArray<uint32_t>& mSamples); // TODO: Bug 1364043

Remember to remove the TODO in the final patch.

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +238,5 @@
>    JS::ToUint32(cx.GetJSContext(), otherValue, &uOtherValue);
>    ASSERT_EQ(uOtherValue, kOtherSampleExpectedValue) << "The other-sample histogram is not returning expected value";
>  }
> +
> +TEST_F(TelemetryTestFixture, AccumulateCountHistogram2)

A more descriptive name might be something like "AccumulateCountHistogram_MultipleSamples"

@@ +242,5 @@
> +TEST_F(TelemetryTestFixture, AccumulateCountHistogram2)
> +{
> +  const uint32_t nSamples = 3;
> +  const uint32_t aValue = 4;
> +  nsTArray<uint32_t> mSamples(nSamples);

nsTArray<T> has a constructor that accepts a std::initializer_list<T>, so you should be able to skip the loop and write

nsTArray<uint32_t> mSamples({4, 4, 4});
Flags: needinfo?(chutten)
The "counts" array is a JS::RootedValue whereas the JS_GetElement requires it to be a JS::Handle obj. I don't know how to convert one to the other so I tried looking at other tests.

The GetElement() function has been used in the AccumulateCategoricalHistogram test as follows:
>  GetElement(cx.GetJSContext(),
>             static_cast<uint32_t>(Telemetry::LABELS_TELEMETRY_TEST_CATEGORICAL::CommonLabel),
>             counts, &value);

Here I am assuming that the static_cast serves as an 'index' into the categorical histogram.

I created the following test on similar lines:
>TEST_F(TelemetryTestFixture, AccumulateLinearHistogram)
>{
>  nsTArray<uint32_t> mSamples({4,4,4});
>  uint32_t kExpectedCount = 3;

>  AutoJSContextWithGlobal cx(mCleanGlobal);

>  GetAndClearHistogram(cx.GetJSContext(), mTelemetry, NS_LITERAL_CSTRING("TELEMETRY_TEST_LINEAR"),
>                        false);

>  // Accumulate in the histogram
> Telemetry::Accumulate(Telemetry::TELEMETRY_TEST_COUNT, mSamples);

>  // Get a snapshot of all the histograms
>  JS::RootedValue snapshot(cx.GetJSContext());
>  GetSnapshots(cx.GetJSContext(), mTelemetry, "TELEMETRY_TEST_LINEAR", &snapshot, false);

>  // Get histogram from snapshot
>  JS::RootedValue histogram(cx.GetJSContext());
>  GetProperty(cx.GetJSContext(), "TELEMETRY_TEST_LINEAR", snapshot, &histogram);

>  // Get "counts" array from histogram
>  JS::RootedValue counts(cx.GetJSContext());
>  GetProperty(cx.GetJSContext(), "counts", histogram, &counts);

>  // Get count in bucket with index 0
>  JS::RootedValue count(cx.GetJSContext());
>  const uint32_t aIndex = 0;
>  GetElement(cx.GetJSContext(), aIndex, counts, &count);

>  // Check that this count matches with nSamples
>  uint32_t uCount = 0;
>  JS::ToUint32(cx.GetJSContext(), count, &uCount);
>  ASSERT_EQ(uCount, kExpectedCount) << "This histogram is not returning the expected value";
>}

I'm not getting an error straight out but this:
[ RUN      ] TelemetryTestFixture.AccumulateLinearHistogram
ExceptionHandler::GenerateDump cloned child 12109
ExceptionHandler::SendContinueSignalToChild sent continue signal to child
ExceptionHandler::WaitForContinueSignal waiting for continue signal...

What am I missing? Other tests seem to use the JS Helper function GetElement like this without any errors.

Also, regarding the addition of multiple samples into categorical histograms, I guess I will have to extend the AccumulateCatogorical API and create a new function that takes in an array of enumValues and calls the new Accumulate function? If that is the case, I think I should get this function right before doing so.
Flags: needinfo?(chutten)
(In reply to Aditya Bharti [:adbugger] from comment #9)
> What am I missing? Other tests seem to use the JS Helper function GetElement
> like this without any errors.

It's a puzzle, that's for sure. The ExceptionHandler means the tests crashed, which isn't very nice of them. Have you used a debugger before? You can run mach gtest with --debug to enable the default debugger. It'll attach to the test run and should hit a breakpoint just as the process crashes to let you inspect the program state.

I'm running a build to try the same thing, so I'll leave the needinfo for now.
 
> Also, regarding the addition of multiple samples into categorical
> histograms, I guess I will have to extend the AccumulateCatogorical API and
> create a new function that takes in an array of enumValues and calls the new
> Accumulate function? If that is the case, I think I should get this function
> right before doing so.

Sounds like a solid plan. Adding a variant (and tests) for AccumulateCategorical is part of the Extra Credit section of comment#2, so I don't expect it in this first patch.
Flags: needinfo?(chutten)
Flags: needinfo?(chutten)
According to my local debug session the problem is that "TELEMETRY_TEST_LINEAR" isn't in the snapshot (it fails when it tries to be coerced to an object so we can get the `counts` property out of it).

And upon further examination, it is clear why this is the case. We're not accumulating any values to TELEMETRY_TEST_LINEAR, we're accumulating to TELEMETRY_TEST_COUNT.

I think the test ought to work if you change the Telemetry::Accumulate line to accumulate to the linear test hgram.
Flags: needinfo?(chutten)
Argh. My bad. Stupid mistake. Will submit the next patch soon.

Moving on.

1. How do I check whether a histogram is keyed? Will add a check in the Telemetry::Accumulate function to ensure we don't add to a keyed histogram.
2. What should be the behaviour when we try to add to a keyed histogram? I assume there will some function like MOZ_ASSERT_UNREACHABLE and and early return.
3. Whatever the correct procedure is, how do I test for the 'appropriate' failure?

4. I just realised that some functions in TelemetryHistograms.cpp check the internal_CanRecordBase() [1] whereas some don't [2]. What is the correct procedure? I seem to remember that we removed all usages of the canRecord bool pref from telemetry.js in a previous bug, saying that telemetry will handle recording permission internally. If I understand correctly, every function should check internal_CanRecordBase() no?

[1] https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/TelemetryHistogram.cpp#1906
[2] https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/TelemetryHistogram.cpp#1865

These two seem to be doing the same thing. One takes a histogram id, while the other takes a name.
Flags: needinfo?(chutten)
Added another test. Will add test for keyed histogram failure, subject to questions from previous comment.
Attachment #8939409 - Attachment is obsolete: true
(In reply to Aditya Bharti [:adbugger] from comment #12)
> 1. How do I check whether a histogram is keyed? Will add a check in the
> Telemetry::Accumulate function to ensure we don't add to a keyed histogram.

gHistogramInfos[aId].keyed is true if aId is the HistogramID for a keyed histogram, false otherwise.

You can see it used throughout the file TelemetryHistogram.cpp in MOZ_ASSERT forms and early-return forms.

> 2. What should be the behaviour when we try to add to a keyed histogram? I
> assume there will some function like MOZ_ASSERT_UNREACHABLE and and early
> return.

You can code it to assert if it's called with a keyed histogram. Throughout the code we assume users are calling only the keyed APIs with keyed histograms and the non-keyed APIs with non-keyed histograms.

> 3. Whatever the correct procedure is, how do I test for the 'appropriate'
> failure?

If you chose to just MOZ_ASSERT it, then no test necessary. If you choose to early return, you'll need to make sure it doesn't record the value you asked it to accumulate.

> 4. I just realised that some functions in TelemetryHistograms.cpp check the
> internal_CanRecordBase() [1] whereas some don't [2]. What is the correct
> procedure? I seem to remember that we removed all usages of the canRecord
> bool pref from telemetry.js in a previous bug, saying that telemetry will
> handle recording permission internally. If I understand correctly, every
> function should check internal_CanRecordBase() no?

Don't worry. Each call of internal_Accumulate will check it for you. And then, later, internal_HistogramAdd will double-check it as part of the CanRecordDataset expression.

If you aim for the Extra Credit of more efficiently adding multiple samples directly into the histogram instance and therefore bypassing internal_Accumulate and internal_HistogramAdd, we might need to think about it. For now, existing code has you covered.
Flags: needinfo?(chutten)
Added the keyed histogram condition check, using Moz assert.
Attachment #8939643 - Attachment is obsolete: true
Needinfoing for the latest attachment in comment #15. Let me know if this looks good. The first patch is almost done.
This checks for count and linear histograms and fails when the histogram is keyed.

If this looks good, I'll add a test adding multiple different samples, remove the TODO's and upload a proper commit (after testing on try).

Then I guess on to categorical and keyed histograms.
Flags: needinfo?(chutten)
Comment on attachment 8939713 [details] [diff] [review]
Tests for linear and count histograms with keyed histogram failure

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

Just a few small things

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +1881,5 @@
> +    MOZ_ASSERT_UNREACHABLE("Histogram usage requires valid ids.");
> +    return;
> +  }
> +
> +  MOZ_ASSERT(!gHistogramInfos[keyed], "Cannot accumulate into a keyed histogram. No key given.");

This will fail to compile. !gHistogramInfos[aID].keyed, no?

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +302,5 @@
> +
> +  // Check that this count matches with nSamples
> +  uint32_t uCount = 0;
> +  JS::ToUint32(cx.GetJSContext(), count, &uCount);
> +  ASSERT_EQ(uCount, kExpectedCount) << "This histogram is not returning the expected value";

The histogram did not accumulate the correct number of values, rather.
(In reply to Chris H-C :chutten from comment #17)

> > +
> > +  MOZ_ASSERT(!gHistogramInfos[keyed], "Cannot accumulate into a keyed histogram. No key given.");
> 
> This will fail to compile. !gHistogramInfos[aID].keyed, no?

How did I miss that? Does running ./mach gtest Telemetry* not trigger a build?

Making the other change.
Flags: needinfo?(chutten)
Apologies for the last erroneous patch. This one has been compiled locally and all gtests pass.

Few things regarding the last test case AccumulateLinearHistogram_DifferentSamples :

The histogram TELEMETRY_TEST_LINEAR has low=1, high=INT_MAX, n_buckets=10

1. How are the bucket regions divided by range? When I accumulated {4,8,high/10 + 20}, all 3 values were being accumulated into the first bucket. Unless, I'm mistaken, the last value should be counted in the second bucket, right?

2. Are values greater than high accumulated at all? The documentation at [1] seems to suggest they are ('last' bucket), but when I tried to accumulate {high+3}, neither bucket index 9 nor 10 had any values in it. Right now, I'm testing by putting 2 values in first bucket and one in the last one.

Otherwise, patch seems done. Awaiting final comments before uploading proper commit. (Ignore the TODOs. Will remove before commit.)

[1] https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/histograms.html#high
Attachment #8939713 - Attachment is obsolete: true
Flags: needinfo?(chutten)
Never mind the questions in comment #19. :Dexter from #telemetry cleared that up. :)
He's helpful that way :)
Flags: needinfo?(chutten)
Comment on attachment 8940100 [details] [diff] [review]
Tests for count histogram, and linear histogram (same and different samples)

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

Prefixes in mozilla codebases are documented over here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Prefixes

I've highlighted each and every one I found that could be changed so we don't miss any, so please don't be alarmed by the number of comments :)

With your try server access set up, have you tried pushing this to try to see if it will build and run on other platforms?

::: toolkit/components/telemetry/Telemetry.cpp
@@ +1930,5 @@
>    TelemetryHistogram::Accumulate(aHistogram, aSample);
>  }
>  
> +void // TODO: Bug 1364043
> +Accumulate(HistogramID aHistogram, const nsTArray<uint32_t>& mSamples)

aSamples

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +1874,5 @@
>    internal_Accumulate(aID, aSample);
>  }
>  
> +void // TODO: Bug 1364043
> +TelemetryHistogram::Accumulate(HistogramID aID, const nsTArray<uint32_t>& mSamples)

aSamples

::: toolkit/components/telemetry/TelemetryHistogram.h
@@ +36,4 @@
>  nsresult SetHistogramRecordingEnabled(const nsACString &id, bool aEnabled);
>  
>  void Accumulate(mozilla::Telemetry::HistogramID aHistogram, uint32_t aSample);
> +void Accumulate(mozilla::Telemetry::HistogramID aHistogram, const nsTArray<uint32_t>& mSamples); // TODO: Bug 1364043

The prefix for arguments is `a` so this should be `aSamples`

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +240,5 @@
>  }
> +
> +TEST_F(TelemetryTestFixture, AccumulateCountHistogram_MultipleSamples)
> +{
> +  nsTArray<uint32_t> mSamples({4,4,4});

m is the prefix for member variables. This should be just `samples`

@@ +241,5 @@
> +
> +TEST_F(TelemetryTestFixture, AccumulateCountHistogram_MultipleSamples)
> +{
> +  nsTArray<uint32_t> mSamples({4,4,4});
> +  uint32_t kExpectedSum = 12;

k is the prefix for constants. We can make it const, or drop it to just expectedSum.

@@ +271,5 @@
> +}
> +
> +TEST_F(TelemetryTestFixture, AccumulateLinearHistogram_MultipleSamples)
> +{
> +  nsTArray<uint32_t> mSamples({4,4,4});

samples

@@ +272,5 @@
> +
> +TEST_F(TelemetryTestFixture, AccumulateLinearHistogram_MultipleSamples)
> +{
> +  nsTArray<uint32_t> mSamples({4,4,4});
> +  uint32_t kExpectedCount = 3;

make const

@@ +296,5 @@
> +  GetProperty(cx.GetJSContext(), "counts", histogram, &counts);
> +
> +  // Index 0 is only for values less than 'low'. Values within range start at index 1
> +  JS::RootedValue count(cx.GetJSContext());
> +  const uint32_t aIndex = 1;

a is the prefix for arguments to the function. This can just be `index` or you can just pas the 1 straight to GetElement.

@@ +307,5 @@
> +}
> +
> +TEST_F(TelemetryTestFixture, AccumulateLinearHistogram_DifferentSamples)
> +{
> +  const uint32_t high = 2147483646;

high what? Maybe this could be highValue? ... Actually, you can just put this inline in `samples` since it isn't referenced elsewhere in this block.

@@ +308,5 @@
> +
> +TEST_F(TelemetryTestFixture, AccumulateLinearHistogram_DifferentSamples)
> +{
> +  const uint32_t high = 2147483646;
> +  nsTArray<uint32_t> mSamples({4, 8, high});

samples

@@ +346,5 @@
> +  JS::ToUint32(cx.GetJSContext(), countLast, &uCountLast);
> +
> +  const uint32_t kExpectedCountFirst = 2;
> +  const uint32_t kExpectedCountLast = 1;
> +  ASSERT_EQ(uCountFirst, kExpectedCountFirst) << "This bucket did not accumulate the correct values";

correct number of values

@@ +347,5 @@
> +
> +  const uint32_t kExpectedCountFirst = 2;
> +  const uint32_t kExpectedCountLast = 1;
> +  ASSERT_EQ(uCountFirst, kExpectedCountFirst) << "This bucket did not accumulate the correct values";
> +  ASSERT_EQ(uCountLast, kExpectedCountLast) << "This bucket did not accumulate the correct values";

correct number of values
Here is the final patch. Please go ahead and push this to try yourself. I'm having issues with my public key (permission denied) on cellular connection, plus my college lan blocks port 22 by default. I'm guessing it'll take me a few days to get both resolved.
Attachment #8940100 - Attachment is obsolete: true
Attachment #8940407 - Flags: review?(chutten)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac00b7870eafd8ad2630f817675cebec9e66d31e

Resolved issues. Pushed to try. I did trigger a lot of new jobs but they aren't showing, plus bugherder shows no pending jobs.
There's no visual feedback when clicking the 'trigger new jobs' button so I'm pretty sure I accidentally triggered more jobs that required. In any case, here's the link, add/remove if required.
Looks like it worked to me! (treeherder can be rather slow because of how hard it works. On the positive side, we all get a free lesson in patience :) )

I've added GTest runs to Linux, Mac, Windows and an Android build to cover a few more bases on that end while I review the code.
Comment on attachment 8940407 [details] [diff] [review]
Adds multiple samples to a histogram with one call

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

Looks good to me!

There is a tiny thing that I just noticed. The two asserts at the end of the third test case have identical failure messages. To make it clear which one failed without having to check line numbers, I recommend varying the strings slightly so that they're easier to distinguish.

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +345,5 @@
> +  JS::ToUint32(cx.GetJSContext(), countLast, &uCountLast);
> +
> +  const uint32_t kExpectedCountFirst = 2;
> +  const uint32_t kExpectedCountLast = 1;
> +  ASSERT_EQ(uCountFirst, kExpectedCountFirst) << "This bucket did not accumulate the correct number of values";

The first bucket

@@ +346,5 @@
> +
> +  const uint32_t kExpectedCountFirst = 2;
> +  const uint32_t kExpectedCountLast = 1;
> +  ASSERT_EQ(uCountFirst, kExpectedCountFirst) << "This bucket did not accumulate the correct number of values";
> +  ASSERT_EQ(uCountLast, kExpectedCountLast) << "This bucket did not accumulate the correct number of values";

The last bucket
Attachment #8940407 - Flags: review?(chutten) → review+
At this point we can talk about how you want to handle the next parts of the bug. Being able to accept multiple samples for keyed histograms, supporting the JSHistogram APIs so that JavaScript can get in on this, supporting Categorical histograms... these all sound like new bugs to me.

If you're looking for more to do after you've fixed those strings and reposted the patch, you can file new bugs for our next steps and then start working on them :)
Changed lines. Squashed all changes into one commit. I guess testing this on try is redundant?

Marking the previous patch as obsolete.
Attachment #8940407 - Attachment is obsolete: true
Attachment #8940840 - Flags: review?(chutten)
See Also: → 1428885
See Also: → 1428888
Attachment #8940840 - Flags: review?(chutten) → review+
The builds and GTest runs aren't completely done yet, but the ones that are done are green, so I'm marking this for checkin.
Keywords: checkin-needed
Blocks: 1428893
See Also: → 1428893
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b820ea091f9
Allow C++ to accumulate multiple samples into histograms with one call. r=chutten
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9b820ea091f9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
See Also: → 1433998
See Also: → 1434004
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: