Copy the Histogram details while we hold the lock in internal_JSHistogram_Snapshot

RESOLVED INVALID

Status

()

enhancement
RESOLVED INVALID
2 years ago
Last year

People

(Reporter: chutten, Assigned: alejandro, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++])

In internal_JSHistogram_Snapshot (as of bug 1278821) we hold the lock to copy out the histogram's SampleSet (the data, basically). We release the lock so we can then call internal_ReflectHistogramAndSamples which uses the JS Engine (which may access Telemetry APIs and require the lock itself) to build the response. 

However, internal_ReflectHistogramAndSamples doesn't just use the SampleSet, it also uses the passed-in Histogram to find out immutable things like ranges and histogram_type... but also mutable things like min, max, and sum.

min, max, and sum may have changed between the copying of the sampleset and the call into internal_ReflectHistogramAndSamples. So we should take local copies of those three values and use them to build the JS snapshot.
Chris, when you have time, is this something you'd want to setup for mentoring?
Flags: needinfo?(chutten)
Mentor: chutten
Flags: needinfo?(chutten)
Whiteboard: [lang=c++]
Assignee: nobody → alexrs95
Status: NEW → ASSIGNED
Hey Chris, I have a doubt regarding this comment:
> min, max, and sum may have changed
If I understand correctly, `min` and `max` are accessed through a pointer to a histogram, but `sum` is part of the SampleSet (https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/TelemetryHistogram.cpp#652) and its value can't change.

What we may want to copy is the bucket count (if this value is mutable), which is accessed through the pointer to the histogram. (https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/TelemetryHistogram.cpp#656)

Am I right?
Flags: needinfo?(chutten)
You're right, sum is handled appropriately. Bucket count shouldn't change, as it is set at compilation. Min and Max are still a problem...

...oh wait, min and max are not what I thought they were. I thought they were the minimum and maximum values recorded into the histogram, but they appear to be equivalent to the "low" and "high" values from the definition in Histograms.json.

If you confirm that this is true, we can go ahead and close out this bug as being INVALID (aka, Wrong).
Flags: needinfo?(chutten)
Yeah, they are exactly what you say. From here (https://searchfox.org/mozilla-central/source/ipc/chromium/src/base/histogram.h#303) we can see that the comment next to `declared_min_` says:
> Less than this goes into counts_[0]
and if we go to `histograms.rst` we can see that it matches the description of `low` (https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/docs/collection/histograms.rst#158).

The same with `declare_max_` and `high`.

Should we consider renaming those parameters to avoid confusion in the future?
Flags: needinfo?(chutten)
I think that might be too little benefit for a not-insignificant amount of work.

Thank you for looking into this! Sorry for the noise.
Status: ASSIGNED → RESOLVED
Closed: Last year
Flags: needinfo?(chutten)
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.