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)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: Yoric, Unassigned)
Details
Attachments
(3 files, 2 obsolete files)
3.12 KB,
patch
|
vladan
:
review+
|
Details | Diff | Splinter Review |
1.73 KB,
patch
|
vladan
:
review+
|
Details | Diff | Splinter Review |
6.33 KB,
patch
|
vladan
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•10 years ago
|
||
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)
Updated•10 years ago
|
Summary: Compress Telemetry comments → don't include histogram comments in the generated data structures
Comment 3•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8343937 -
Flags: review?(vdjeric) → review+
Comment 4•10 years ago
|
||
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)
Comment 5•10 years ago
|
||
(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.
Comment 6•10 years ago
|
||
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)
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8347350 -
Flags: review?(vdjeric) → review+
Comment 9•10 years ago
|
||
Btw, will the histogram name strings get copied twice, once in Telemetry::GetRegisteredHistograms and again when being converted to JS strings?
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0579da7f7f4 https://hg.mozilla.org/integration/mozilla-inbound/rev/2050afa859b4 https://hg.mozilla.org/integration/mozilla-inbound/rev/991c995ce483
Flags: in-testsuite+
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f0579da7f7f4 https://hg.mozilla.org/mozilla-central/rev/2050afa859b4 https://hg.mozilla.org/mozilla-central/rev/991c995ce483
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•