1256366 - Remove linear and exponential stats collection from histogram.cc

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P4)

Description

[Georg Fritzsche [:gfritzsche]]

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

Comment 1

[Georg Fritzsche [:gfritzsche]]

(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

Comment 2

[Georg Fritzsche [:gfritzsche]]

malayaleecoder, are you interested in taking this follow-up bug to bug 1252481?

Comment 3

[Vishnu (:malayaleecoder)]

Sure Georg, I am taking this :)

Comment 4

[Georg Fritzsche [:gfritzsche]]

Hey malayaeecoder, did you have success working on this?
Let me know if you're stuck, i'm happy to help.

Comment 5

[Vishnu (:malayaleecoder)]

Attachment: Bug1256366_v1

Hello Georg,

Please have a look at the patch. I smell something wrong even though the build went through fine.

Comment 6

[Vishnu (:malayaleecoder)]

Attachment: Bug1256366_v1.diff

Please ignore the previous patch.

Comment 7

[Georg Fritzsche [:gfritzsche]]

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?

Comment 8

[Julian Seward [: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.

Comment 9

[Georg Fritzsche [:gfritzsche]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad4579d1c3c6

Comment 10

[Vishnu (:malayaleecoder)]

Georg the try push seems good. Adding checkin-needed , hope it is fine.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/36519b116903

Comment 12

[Georg Fritzsche [:gfritzsche]]

Sure, thanks!

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/36519b116903