Closed Bug 1457127 Opened 6 years ago Closed 6 years ago

Implement histogram persistence support for GeckoView

Categories

(Toolkit :: Telemetry, enhancement, P1)

Product:
Toolkit
Component:
Telemetry
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

Details

(Whiteboard: [geckoview:klar:p1])

Attachments

(9 files)

Bug 1457127 - Create common helpers for taking histogram snapshots.
59 bytes, text/x-review-board-request
chutten
: review+
janerik
: review+
Details
Bug 1457127 - Create common helpers for taking keyed histogram snapshots.
59 bytes, text/x-review-board-request
janerik
: review+
chutten
: review+
Details
Bug 1457127 - Remove unused reflectStatus.
59 bytes, text/x-review-board-request
janerik
: review+
Details
Bug 1457127 - Do not reflect histograms that are not for the current product.
59 bytes, text/x-review-board-request
chutten
: review+
janerik
: review+
Details
Bug 1457127 - Implement histogram persistence for GeckoView.
59 bytes, text/x-review-board-request
janerik
: review+
chutten
: review+
Details
Bug 1457127 - Implement keyed histogram persistence for GeckoView.
59 bytes, text/x-review-board-request
janerik
: review+
chutten
: review+
Details
Bug 1457127 - Add basic gtest coverage for histogram persistence I/O.
59 bytes, text/x-review-board-request
chutten
: review+
janerik
: review+
Details
Bug 1457127 - Add an helper function for checking if the GPU process was used.
59 bytes, text/x-review-board-request
janerik
: review+
Details
[DO NOT LAND] - test_persistence.patch
2.73 KB, patch
Details | Diff | Splinter Review 
In bug 1453591 we landed initial support for GV persistence. In that bug we also added support for scalar probes persistence. We need to do the same for histograms:

- Persist the data to disk in GeckoViewPersistence.cpp
- Load it off the disk and update the histograms.
Blocks: 1453591
Priority: -- → P2
Assignee: nobody → alessio.placitelli
Priority: P2 → P1
Depends on: 1430531
Comment on attachment 8976077 [details]
Bug 1457127 - Create common helpers for taking histogram snapshots.

https://reviewboard.mozilla.org/r/244262/#review250252

I'm all for having smaller functions! Couple of things to clarify in this patch.

::: commit-message-3c9d6:5
(Diff revision 1)
> +Bug 1457127 - Create common helpers for taking histogram snapshots. r?chutten,janerik
> +
> +This patch factors out the code needed to take snapshots for both plain
> +and keyed histogram. These functions will be used for generating a snapshot
> +for the persistence logic as well.

This commit only extracts the non-keyed histogram snapshotting.

::: toolkit/components/telemetry/TelemetryHistogram.cpp:764
(Diff revision 1)
>  
> +/**
> + * Helper function to get a snapshot of the histograms.
> + *
> + * @param {aLock} the lock proof.
> + * @param {aDataset} the dataset for which we're requesting the snapshot.

We use several different styles for these parameter comments, sometimes addressing the user with `we`, sometimes more formal passive phrasing.
Maybe for consistency we should decide which one we prefer.

::: toolkit/components/telemetry/TelemetryHistogram.cpp:784
(Diff revision 1)
> +  if (!aOutSnapshot.resize(static_cast<uint32_t>(ProcessID::Count))) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +
> +  for (uint32_t process = 0; process < static_cast<uint32_t>(ProcessID::Count); ++process) {
> +    HistogramSnapshotsArray& hArray = aOutSnapshot[process];

You resize the array above to the required size, but does that actually ensure we have valid `HistogramSnapshotsArray` elements in there?
Attachment #8976077 - Flags: review?(jrediger) → review-
Comment on attachment 8976078 [details]
Bug 1457127 - Create common helpers for taking keyed histogram snapshots.

https://reviewboard.mozilla.org/r/244264/#review250264

::: toolkit/components/telemetry/TelemetryHistogram.cpp:781
(Diff revision 1)
>   * @param {aOutSnapshot} the container in which the snapshot data will be stored.
>   * @return {nsresult} NS_OK if the snapshot was successfully taken or
>   *         NS_ERROR_OUT_OF_MEMORY if we failed to allocate memory.
>   */
>  nsresult
> -internal_GetHistogramSnapshots(const StaticMutexAutoLock& aLock,
> +internal_GetHistogramsSnapshot(const StaticMutexAutoLock& aLock,

Maybe avoid this rename by naming it like this in the first patch.

::: toolkit/components/telemetry/TelemetryHistogram.cpp:1079
(Diff revision 1)
> +                                    unsigned int aDataset,
> +                                    bool aClearSubsession,
> +                                    bool aIncludeGPU,
> +                                    KeyedHistogramProcessSnapshotsArray& aOutSnapshot)
> +{
> +  if (!aOutSnapshot.resize(static_cast<uint32_t>(ProcessID::Count))) {

Same as before. I'd just like to know if that is actually defined behaviour.

::: toolkit/components/telemetry/TelemetryHistogram.cpp:2448
(Diff revision 1)
> -    KeyedHistogramSnapshotData data;
> -    HistogramID histogramId;
> -  };
> -
>    // Get a snapshot of all the data while holding the mutex.
> -  mozilla::Vector<mozilla::Vector<KeyedHistogramProcessInfo>> dataSnapshot;
> +  KeyedHistogramProcessSnapshotsArray dataSnapshot;

In the other function it's called `processHistArray`. Should we keep a consistent name here in both functions?
Attachment #8976078 - Flags: review?(jrediger) → review-
Comment on attachment 8976079 [details]
Bug 1457127 - Remove unused reflectStatus.

https://reviewboard.mozilla.org/r/244266/#review250266
Attachment #8976079 - Flags: review?(jrediger) → review+
Comment on attachment 8976080 [details]
Bug 1457127 - Do not reflect histograms that are not for the current product.

https://reviewboard.mozilla.org/r/244268/#review250268

As I understand it histogram flags are deprecated anyway, so we won't see them appear on GeckoView anyway, right? Just want to make sure it has no pipeline impact.
We should be careful to not whitelist histogram flags.
Attachment #8976080 - Flags: review?(jrediger) → review+
Comment on attachment 8976077 [details]
Bug 1457127 - Create common helpers for taking histogram snapshots.

https://reviewboard.mozilla.org/r/244262/#review250278

::: toolkit/components/telemetry/TelemetryHistogram.cpp:764
(Diff revision 1)
>  
> +/**
> + * Helper function to get a snapshot of the histograms.
> + *
> + * @param {aLock} the lock proof.
> + * @param {aDataset} the dataset for which we're requesting the snapshot.

Good point. I switched to formal passive phrasing and filed bug 1461976 for creating a linter.

::: toolkit/components/telemetry/TelemetryHistogram.cpp:784
(Diff revision 1)
> +  if (!aOutSnapshot.resize(static_cast<uint32_t>(ProcessID::Count))) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +
> +  for (uint32_t process = 0; process < static_cast<uint32_t>(ProcessID::Count); ++process) {
> +    HistogramSnapshotsArray& hArray = aOutSnapshot[process];

It does! Here it's using `Mozilla::Vector`, which mimicks `std::vector` that has the same behaviour. Moreover, code comment [says so](https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/mfbt/Vector.h#661,667) and it didn't crash :)
Comment on attachment 8976078 [details]
Bug 1457127 - Create common helpers for taking keyed histogram snapshots.

https://reviewboard.mozilla.org/r/244264/#review250284

::: toolkit/components/telemetry/TelemetryHistogram.cpp:1079
(Diff revision 1)
> +                                    unsigned int aDataset,
> +                                    bool aClearSubsession,
> +                                    bool aIncludeGPU,
> +                                    KeyedHistogramProcessSnapshotsArray& aOutSnapshot)
> +{
> +  if (!aOutSnapshot.resize(static_cast<uint32_t>(ProcessID::Count))) {

Yes, that's actually defined behaviour (see the other comment).
Comment on attachment 8976080 [details]
Bug 1457127 - Do not reflect histograms that are not for the current product.

https://reviewboard.mozilla.org/r/244268/#review250268

They are, as in we're not allowing the creation of new flag histograms. Legacy flag histograms can still be whitelisted and sent, if they are allowed on the product. If they are not, then we don't expect them to flow in regardless of their value. For this reason, this should have no pipeline impact. Moreover, this should be no-op on Desktop and Fennec.
Comment on attachment 8976077 [details]
Bug 1457127 - Create common helpers for taking histogram snapshots.

https://reviewboard.mozilla.org/r/244262/#review250302

Seems fairly straightforward.

::: toolkit/components/telemetry/TelemetryHistogram.cpp:776
(Diff revision 2)
> + */
> +nsresult
> +internal_GetHistogramsSnapshot(const StaticMutexAutoLock& aLock,
> +                               unsigned int aDataset,
> +                               bool aClearSubsession,
> +                               bool aIncludeGPU,

*sigh* (if it weren't for all the flag histograms cluttering up every process everywhere, we wouldn't need this nonsense)

::: toolkit/components/telemetry/TelemetryHistogram.cpp:804
(Diff revision 2)
> +      if (!IsInDataset(info.dataset, aDataset)) {
> +        continue;
> +      }
> +
> +      bool shouldInstantiate =
> +        info.histogramType == nsITelemetry::HISTOGRAM_FLAG;

*sigh*

::: toolkit/components/telemetry/TelemetryHistogram.cpp:2330
(Diff revision 2)
>                             processObject, JSPROP_ENUMERATE)) {
>        return NS_ERROR_FAILURE;
>      }
>  
> -    const mozilla::Vector<HistogramProcessInfo>& hArray = processHistArray[process];
> +    const HistogramSnapshotsArray& hArray = processHistArray[process];
>      for (size_t i = 0; i < hArray.length(); ++i) {

While we're in the neighbourhood, how about a new-style forloop? We don't need `i` if all we use it for is getting `hData`.
Attachment #8976077 - Flags: review?(chutten) → review+
Comment on attachment 8976081 [details]
Bug 1457127 - Implement histogram persistence for GeckoView.

https://reviewboard.mozilla.org/r/244270/#review250300

This looks suprisingly clean after the previous histogram snapshot refactor (if we ignore the boiler plate the JS API forces on us).
Couple of small issues remaining, but overall it looks good.

::: toolkit/components/telemetry/TelemetryHistogram.cpp:2549
(Diff revision 2)
>    return 0;
>  }
> +
> +#if defined(MOZ_TELEMETRY_GECKOVIEW)
> +namespace base {
> +class DummySampleSet : public Histogram::SampleSet

Dummy sounds like it's not for serious use.
We apparently need it to properly track the counts. 
Can we find a better name?
`PersistedSampleSet` maybe?

::: toolkit/components/telemetry/TelemetryHistogram.cpp:2573
(Diff revision 2)
> +};
> +} // base (from ipc/chromium/src/base)
> +
> +namespace {
> +void
> +internal_ReflectHistogramToJSON(mozilla::JSONWriter& aWriter,

For scalars you passed the writer as the last argument. Here it's the other way around.
It makes sense to keep it consistent.

::: toolkit/components/telemetry/TelemetryHistogram.cpp:2573
(Diff revision 2)
> +};
> +} // base (from ipc/chromium/src/base)
> +
> +namespace {
> +void
> +internal_ReflectHistogramToJSON(mozilla::JSONWriter& aWriter,

For this function to work correctly it requires that a new object is already started. This should either be documented or the object opening/closing moved into it.

::: toolkit/components/telemetry/TelemetryHistogram.cpp:2733
(Diff revision 2)
> +      if (!histogramName.init(aCx, histogram)) {
> +        JS_ClearPendingException(aCx);
> +        continue;
> +      }
> +
> +      // Get the data for this histogram.)

nit. Trailing ).
Attachment #8976081 - Flags: review?(jrediger) → review-
Comment on attachment 8976078 [details]
Bug 1457127 - Create common helpers for taking keyed histogram snapshots.

https://reviewboard.mozilla.org/r/244264/#review250298

::: toolkit/components/telemetry/TelemetryHistogram.cpp:2473
(Diff revision 2)
>      }
>      if (!JS_DefineProperty(aCx, obj, GetNameForProcessID(ProcessID(process)),
>                             processObject, JSPROP_ENUMERATE)) {
>        return NS_ERROR_FAILURE;
>      }
>      for (size_t i = 0; i < hArray.length(); ++i) {

We can use a new-style for loop here, too, if we want to be classy.
Attachment #8976078 - Flags: review?(chutten) → review+
Comment on attachment 8976080 [details]
Bug 1457127 - Do not reflect histograms that are not for the current product.

https://reviewboard.mozilla.org/r/244268/#review250322
Attachment #8976080 - Flags: review?(chutten) → review+
Comment on attachment 8976082 [details]
Bug 1457127 - Implement keyed histogram persistence for GeckoView.

https://reviewboard.mozilla.org/r/244272/#review250320

Only some nits remaining here.

::: toolkit/components/telemetry/TelemetryHistogram.cpp:1106
(Diff revision 2)
>   * @param {aDataset} the dataset for which the snapshot is being requested.
>   * @param {aClearSubsession} whether or not to clear the data after
>   *        taking the snapshot.
>   * @param {aIncludeGPU} whether or not to include data for the GPU.
>   * @param {aOutSnapshot} the container in which the snapshot data will be stored.
> + * @param {aSkipEmpty} whether or not we can skip empty keyed histograms from the

`we`. With the previous switch this needs a change as well.

::: toolkit/components/telemetry/TelemetryHistogram.cpp:2649
(Diff revision 2)
> +  // Include the GPU process in histogram snapshots only if we actually tried
> +  // to launch a process for it.
> +  bool includeGPUProcess = false;
> +  if (auto gpm = mozilla::gfx::GPUProcessManager::Get()) {
> +    includeGPUProcess = gpm->AttemptedGPUProcess();
> +  }

We now have the same code snippet multiple times. Is it time for a helper function?

::: toolkit/components/telemetry/TelemetryHistogram.cpp:3165
(Diff revision 2)
> +                                            numCounts).get());
> +          continue;
> +        }
> +
> +        // Update the data for the histogram.
> +        h->AddSampleSet(base::DummySampleSet(mozilla::Move(mozilla::Get<2>(histogramData)),

`dummy` name as before.

::: toolkit/components/telemetry/geckoview/TelemetryGeckoViewPersistence.cpp:327
(Diff revision 2)
> +  if (JS_GetProperty(jsapi.cx(), dataObj, "keyedHistograms", &keyedHistogramData)) {
> +    // If the data is an object, try to parse its properties. If not,
> +    // silently skip and try to load the other sections.
> +    if (!keyedHistogramData.isObject()
> +        || NS_FAILED(TelemetryHistogram::DeserializeKeyedHistograms(jsapi.cx(), keyedHistogramData))) {
> +      ANDROID_LOG("MainThreadParsePersistedProbes - Failed to parse 'keyedHistogramData'.");

Should be "Failed to parse 'keyedHistograms'"
Attachment #8976082 - Flags: review?(jrediger) → review+
Comment on attachment 8976083 [details]
Bug 1457127 - Add basic gtest coverage for histogram persistence I/O.

https://reviewboard.mozilla.org/r/244274/#review250332

Before the first release of GV we definitely need a better test story. This is a good start, but nowhere near sufficient imo.
Unfortunately the mocking takes out most of the interesting code and the failure path is never tested.

I know it's tedious but at least having one test with invalid values which are skipped over on deserialization might be a good idea.

::: toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp:44
(Diff revision 2)
> +        "sum": 6,
> +        "counts": [
> +          3, 5, 7
> +        ]
> +      }
> +    }

We should have at least one sample with multiple processes to check that this works as well.
Attachment #8976083 - Flags: review?(jrediger) → review+
Comment on attachment 8976081 [details]
Bug 1457127 - Implement histogram persistence for GeckoView.

https://reviewboard.mozilla.org/r/244270/#review250324

::: toolkit/components/telemetry/TelemetryHistogram.cpp:460
(Diff revision 2)
>    MOZ_ASSERT(internal_IsHistogramEnumId(id));
>    return !gHistogramRecordingDisabled[id];
>  }
>  
> +bool
> +internal_CanRecordHistogram(const HistogramID id,

I am amazed we didn't already have a function by this name.

::: toolkit/components/telemetry/TelemetryHistogram.cpp:2549
(Diff revision 2)
>    return 0;
>  }
> +
> +#if defined(MOZ_TELEMETRY_GECKOVIEW)
> +namespace base {
> +class DummySampleSet : public Histogram::SampleSet

Or we could add an Init method or something directly to Histogram::SampleSet and use that.

::: toolkit/components/telemetry/TelemetryHistogram.cpp:2591
(Diff revision 2)
> +} // Anonymous namespace
> +
> +nsresult
> +TelemetryHistogram::SerializeHistograms(mozilla::JSONWriter& aWriter)
> +{
> +  // Include the GPU process in histogram snapshots only if we actually tried

No XRE_IsParentProcess() assert here?

::: toolkit/components/telemetry/TelemetryHistogram.cpp:2602
(Diff revision 2)
> +
> +  // Take a snapshot of the histograms.
> +  HistogramProcessSnapshotsArray processHistArray;
> +  {
> +    StaticMutexAutoLock locker(gTelemetryHistogramMutex);
> +    // We always request the "opt-in"/"prerelease" dataset: we internally

optin is the release dataset, not prerelease dataset (see https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/toolkit/components/telemetry/TelemetryCommon.cpp#40 )

::: toolkit/components/telemetry/TelemetryHistogram.cpp:2604
(Diff revision 2)
> +  HistogramProcessSnapshotsArray processHistArray;
> +  {
> +    StaticMutexAutoLock locker(gTelemetryHistogramMutex);
> +    // We always request the "opt-in"/"prerelease" dataset: we internally
> +    // record the right subset, so this will only return "prerelease" if
> +    // it was recorded.

Thank goodness we locked canRecordExtended, otherwise we'd have to deal with data recorded before the user was able to disable extended collection...

::: toolkit/components/telemetry/TelemetryHistogram.cpp:2616
(Diff revision 2)
> +    }
> +  }
> +
> +
> +
> +  // Make the JS calls on the stashed histograms for every process

*JSON calls

::: toolkit/components/telemetry/TelemetryHistogram.cpp:2621
(Diff revision 2)
> +  // Make the JS calls on the stashed histograms for every process
> +  for (uint32_t process = 0; process < processHistArray.length(); ++process) {
> +    aWriter.StartObjectProperty(GetNameForProcessID(ProcessID(process)));
> +
> +    const HistogramSnapshotsArray& hArray = processHistArray[process];
> +    for (size_t i = 0; i < hArray.length(); ++i) {

new style forloop

::: toolkit/components/telemetry/geckoview/TelemetryGeckoViewPersistence.cpp:311
(Diff revision 2)
> +  if (JS_GetProperty(jsapi.cx(), dataObj, "histograms", &histogramData)) {
> +    // If the data is an object, try to parse its properties. If not,
> +    // silently skip and try to load the other sections.
> +    if (!histogramData.isObject()
> +        || NS_FAILED(TelemetryHistogram::DeserializeHistograms(jsapi.cx(), histogramData))) {
> +      ANDROID_LOG("MainThreadParsePersistedProbes - Failed to parse 'histograms'.");

We don't return failure codes for most failures here. Do we want to log the minor ones we currently try to recover from?
Attachment #8976081 - Flags: review?(chutten)
Comment on attachment 8976082 [details]
Bug 1457127 - Implement keyed histogram persistence for GeckoView.

https://reviewboard.mozilla.org/r/244272/#review250340

::: toolkit/components/telemetry/TelemetryHistogram.cpp:2644
(Diff revision 2)
>  }
>  
>  nsresult
> +TelemetryHistogram::SerializeKeyedHistograms(mozilla::JSONWriter& aWriter)
> +{
> +  // Include the GPU process in histogram snapshots only if we actually tried

We should only serialize in the parent process as well (the other processes' storages should be empty (aside from flags))

::: toolkit/components/telemetry/TelemetryHistogram.cpp:2655
(Diff revision 2)
> +
> +  // Take a snapshot of the keyed histograms.
> +  KeyedHistogramProcessSnapshotsArray processHistArray;
> +  {
> +    StaticMutexAutoLock locker(gTelemetryHistogramMutex);
> +    // We always request the "opt-in"/"prerelease" dataset: we internally

*release

::: toolkit/components/telemetry/TelemetryHistogram.cpp:3030
(Diff revision 2)
> +        JS_ClearPendingException(aCx);
> +        continue;
> +      }
> +
> +      JS::RootedId key(aCx);
> +      for (auto& keyVal : keys) {

There's code in here we should share with the non-keyed deserialize. At the very least we should be able to extract sum and counts generically.
Attachment #8976082 - Flags: review?(chutten)
Comment on attachment 8976083 [details]
Bug 1457127 - Add basic gtest coverage for histogram persistence I/O.

https://reviewboard.mozilla.org/r/244274/#review250346

::: toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp:49
(Diff revision 2)
> +    }
> +  },
> +  "keyedHistograms": {
> +    "content": {
> +      "TELEMETRY_TEST_MULTIPRODUCT_KEYED": {
> +        "niceKey": {

We should test that multiple keys restore properly.
Attachment #8976083 - Flags: review?(chutten) → review+
Comment on attachment 8976081 [details]
Bug 1457127 - Implement histogram persistence for GeckoView.

https://reviewboard.mozilla.org/r/244270/#review250324

> Or we could add an Init method or something directly to Histogram::SampleSet and use that.

I'd rather keep changes local to toolkit/telemetry, rather than touch our fork of `Histogram.cc`

> optin is the release dataset, not prerelease dataset (see https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/toolkit/components/telemetry/TelemetryCommon.cpp#40 )

Mh, interesting. I think that common is unclear or wrong: shouldn't it be "opt-in" from "prerelease" and just "opt out" from release?
Comment on attachment 8976081 [details]
Bug 1457127 - Implement histogram persistence for GeckoView.

https://reviewboard.mozilla.org/r/244270/#review250324

> Mh, interesting. I think that common is unclear or wrong: shouldn't it be "opt-in" from "prerelease" and just "opt out" from release?

You're right, that comment is misleading.
Comment on attachment 8976083 [details]
Bug 1457127 - Add basic gtest coverage for histogram persistence I/O.

https://reviewboard.mozilla.org/r/244274/#review250332

I agree this is blatantly insufficient for test coverage. I'm afraid gtest is not the solution in our case: we're mocking all the serialization/deserialization functions and all we're covering is that I/O works and that the serialization/deserialization functions are being called as expected. I filed bug 1461965 to create a better testing story for GV, I'll tackle it after this bug. There's also bug 1459144 to try to make this gtest coverage more useful by linking the real modules to the gtest-xul, instead of the mocking one.

> We should have at least one sample with multiple processes to check that this works as well.

This is true. Unfortunately, the parsing logic in the TelemetryHistogram.cpp is never called from this test, as the functions are mocked up in here. I can't simply link that to the gtest to have proper API testing, as there's some compiler magic weirdness going on. I filed bug 1459144 to investigate this.
Comment on attachment 8976083 [details]
Bug 1457127 - Add basic gtest coverage for histogram persistence I/O.

https://reviewboard.mozilla.org/r/244274/#review250346

> We should test that multiple keys restore properly.

That's exactly right! However all this gtest madness does is making sure that the persistence functions are called on the right schedule. Tha serialization/deserialization functions are mocked up, the code in TelemetryHistogram.cpp is never called in these tests. Unfortunately I wasn't able to link TelemetryScalar.cpp/TelemetryHisgoram.cpp to this gtest suite. See the comment I left to Jan-Erik.
Comment on attachment 8976077 [details]
Bug 1457127 - Create common helpers for taking histogram snapshots.

https://reviewboard.mozilla.org/r/244262/#review250608
Attachment #8976077 - Flags: review?(jrediger) → review+
Comment on attachment 8976078 [details]
Bug 1457127 - Create common helpers for taking keyed histogram snapshots.

https://reviewboard.mozilla.org/r/244264/#review250610
Attachment #8976078 - Flags: review?(jrediger) → review+
Comment on attachment 8976449 [details]
Bug 1457127 - Add an helper function for checking if the GPU process was used.

https://reviewboard.mozilla.org/r/244576/#review250612
Attachment #8976449 - Flags: review?(jrediger) → review+
Comment on attachment 8976081 [details]
Bug 1457127 - Implement histogram persistence for GeckoView.

https://reviewboard.mozilla.org/r/244270/#review250616
Attachment #8976081 - Flags: review?(jrediger) → review+
Comment on attachment 8976081 [details]
Bug 1457127 - Implement histogram persistence for GeckoView.

https://reviewboard.mozilla.org/r/244270/#review251066

::: toolkit/components/telemetry/TelemetryHistogram.cpp:2662
(Diff revisions 2 - 4)
> +    JS::RootedValue elementValue(aCx);
> +    int countAsInt = 0;
> +    if (!JS_GetElement(aCx, countsArrayObj, arrayIdx, &elementValue)
> +        || !JS::ToInt32(aCx, elementValue, &countAsInt)) {
> +      JS_ClearPendingException(aCx);
> +      continue;

This means aOutCountArray won't necessarily be the same length as coutnsLen. Will that cause a problem to the caller?

It looks like this will be caught just before AddSampleSet, but we can probably just return NS_ERROR_FAILURE here and make that decision earlier.
Attachment #8976081 - Flags: review?(chutten) → review+
Comment on attachment 8976082 [details]
Bug 1457127 - Implement keyed histogram persistence for GeckoView.

https://reviewboard.mozilla.org/r/244272/#review251072
Attachment #8976082 - Flags: review?(chutten) → review+
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/440073e5e466
Create common helpers for taking histogram snapshots. r=chutten,janerik
https://hg.mozilla.org/integration/autoland/rev/ae169019f9e4
Create common helpers for taking keyed histogram snapshots. r=chutten,janerik
https://hg.mozilla.org/integration/autoland/rev/164e7e5c55eb
Remove unused reflectStatus. r=janerik
https://hg.mozilla.org/integration/autoland/rev/f97a89b40a5f
Do not reflect histograms that are not for the current product. r=chutten,janerik
https://hg.mozilla.org/integration/autoland/rev/b49beaf72b1d
Add an helper function for checking if the GPU process was used. r=janerik
https://hg.mozilla.org/integration/autoland/rev/8fcea2ebf26e
Implement histogram persistence for GeckoView. r=chutten,janerik
https://hg.mozilla.org/integration/autoland/rev/b07106556b62
Implement keyed histogram persistence for GeckoView. r=chutten,janerik
https://hg.mozilla.org/integration/autoland/rev/8850728602d6
Add basic gtest coverage for histogram persistence I/O. r=chutten,janerik
Blocks: 1452550
Adding whiteboard tag [geckoview:klar:p1] because Alessio says this bug should blocker Klar+GeckoView release.
Whiteboard: [geckoview:klar:p1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: