Closed Bug 1256366 Opened 4 years ago Closed 4 years ago

Remove linear and exponential stats collection from histogram.cc

Categories

(Toolkit :: Telemetry, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: gfritzsche, Assigned: malayaleecoder, Mentored)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [measurement:client] [lang=c++])

Attachments

(1 file, 1 obsolete file)

Bug 1252481 removes the "sum_squares_lo" and "sum_squares_hi" properties from Telemetry.

Without those, the additional stats collection in histogram.cc actually never gets used.
This is basically all the additional code that was added here:
https://hg.mozilla.org/mozilla-central/rev/4e8386f13286

To verify this did not break anything, we should run the tests using:
mach test toolkit/components/telemetry/tests/unit
(In reply to Georg Fritzsche [:gfritzsche] from comment #0)
> Without those, the additional stats collection in histogram.cc actually
> never gets used.
> This is basically all the additional code that was added here:
> https://hg.mozilla.org/mozilla-central/rev/4e8386f13286

To add a bit of detail:
For histogram.h & histogram.cc, we can:
* drop all the sum_squares, log_sum, log_sum_squares occurrences and their calculations
* we don't need AccumulateWithLinearStats(), we can just use Accumulate() directly
* note that current code looks a bit difference now than in the revision linked above
malayaleecoder, are you interested in taking this follow-up bug to bug 1252481?
Flags: needinfo?(malayaleecoder)
Sure Georg, I am taking this :)
Flags: needinfo?(malayaleecoder)
Assignee: nobody → malayaleecoder
Hey malayaeecoder, did you have success working on this?
Let me know if you're stuck, i'm happy to help.
Flags: needinfo?(malayaleecoder)
Attached patch Bug1256366_v1 (obsolete) — Splinter Review
Hello Georg,

Please have a look at the patch. I smell something wrong even though the build went through fine.
Flags: needinfo?(malayaleecoder) → needinfo?(gfritzsche)
Please ignore the previous patch.
Attachment #8737700 - Attachment is obsolete: true
Comment on attachment 8737701 [details] [diff] [review]
Bug1256366_v1.diff

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

This looks good to me, thanks!

Julian, would you mind taking a quick look at the mutex usage for correctness?
Attachment #8737701 - Flags: review+
Attachment #8737701 - Flags: feedback?(jseward)
Comment on attachment 8737701 [details] [diff] [review]
Bug1256366_v1.diff

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

Yes, that looks fine to me.  Does not look as if it will change the locking
situation at all.
Attachment #8737701 - Flags: feedback?(jseward) → feedback+
Georg the try push seems good. Adding checkin-needed , hope it is fine.
Status: NEW → ASSIGNED
Flags: needinfo?(gfritzsche)
Keywords: checkin-needed
Sure, thanks!
Flags: needinfo?(gfritzsche)
https://hg.mozilla.org/mozilla-central/rev/36519b116903
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.