Closed Bug 932285 Opened 11 years ago Closed 10 years ago

don't include histogram comments in the generated data structures

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: Yoric, Unassigned)

Details

Attachments

(3 files, 2 obsolete files)

We blow up our data structures (see bug 932281). It should be possible to compress the comments, though. Note in particular that we now have lz4 support on m-c (see bug 888658), which might come in handy.
I didn't convert this to an array because we depend on fast lookups into
registeredHistograms here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryPing.js#287

Making the values undefined seemed like the path of least resistance.
Attachment #8343935 - Flags: review?(vdjeric)
Saves 50K of space (!).
Attachment #8343937 - Flags: review?(vdjeric)
Summary: Compress Telemetry comments → don't include histogram comments in the generated data structures
Updated patch.  Defining things to |undefined| is ineffective, and if
we're going to define them to null, we have to update a few things in
TelemetryPing to be aware of that.
Attachment #8343935 - Attachment is obsolete: true
Attachment #8343935 - Flags: review?(vdjeric)
Attachment #8345499 - Flags: review?(vdjeric)
Attachment #8343937 - Flags: review?(vdjeric) → review+
Comment on attachment 8345499 [details] [diff] [review]
part 1 - make values in nsITelemetry.registeredHistograms null

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

::: toolkit/components/telemetry/TelemetryPing.js
@@ +296,3 @@
>            ret[startup_name] = this.packHistogram(hls[startup_name]);
>        }
>      }

Instead of making Telemetry.registeredHistograms into a map with all values set to null, why not make it into an array and rewrite getHistograms() to iterate over Telemetry.registeredHistograms and then check for the presence of each item in Telemetry.histogramSnapshots instead?
Attachment #8345499 - Flags: review?(vdjeric)
(In reply to Vladan Djeric (:vladan) from comment #4)
> Instead of making Telemetry.registeredHistograms into a map with all values
> set to null, why not make it into an array and rewrite getHistograms() to
> iterate over Telemetry.registeredHistograms and then check for the presence
> of each item in Telemetry.histogramSnapshots instead?

1. Because it is more work. :p
2. Because there used to be some reason that the loops were ordered thus.  But it might have been related to how we saved pings, which has changed quite a bit.

I can't think of a good reason that inverting the checking order shouldn't work.  Will try to work on that tomorrow.
Adjust TelemetryPing to do things in an efficient order before we turn
things into arrays.
Attachment #8345499 - Attachment is obsolete: true
Attachment #8347350 - Flags: review?(vdjeric)
I chose to do things through IDL rather than through JSAPI because raw
JSAPI usage is a footgun.  Let's let XPConnect handle all the details,
even if the calling sequence is not quite as nice.
Attachment #8347352 - Flags: review?(vdjeric)
Comment on attachment 8347352 [details] [diff] [review]
part 2 - convert nsITelemetry.registeredHistograms to a function returning an array

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

::: toolkit/components/telemetry/Telemetry.cpp
@@ +1958,5 @@
>  
>    for (size_t i = 0; i < count; ++i) {
> +    const char* h = gHistograms[i].id();
> +    size_t len = strlen(h);
> +    histograms[i] = static_cast<char*>(nsMemory::Clone(h, len+1));

Technically (len + 1) * sizeof(char)

::: toolkit/components/telemetry/TelemetryPing.js
@@ +289,3 @@
>      let ret = {};
>  
> +    for (name of registered) {

Nit: for (let name ...) {
Attachment #8347352 - Flags: review?(vdjeric) → review+
Attachment #8347350 - Flags: review?(vdjeric) → review+
Btw, will the histogram name strings get copied twice, once in Telemetry::GetRegisteredHistograms and again when being converted to JS strings?
You need to log in before you can comment on or make changes to this bug.