844146 - accumulating into boolean histograms from C++ can produce bogus histograms

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Nathan Froyd [:froydnj]]

See bug 811507 for the gory details.  The division between C++ codepaths and JS codepaths may have made sense once upon a time (they might have even been the same at one point), but the histogram code has evolved enough that we can fix this at the lowest levels.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: add BooleanHistogram::Accumulate to properly canonicalize values for boolean histograms

The only trickiness is explained by the comment in ::Accumulate.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: make JS and C++ codepaths for adding histogram values identical

...and now that the histogram bits handle this for us, we don't need
this extra checking.

Comment 3

[(dormant account)]

Comment on attachment 717219 [details] [diff] [review]
add BooleanHistogram::Accumulate to properly canonicalize values for boolean histograms

Really seems like the underlying vector should be of <bool> of kind.

Comment 4

[(dormant account)]

Comment on attachment 717219 [details] [diff] [review]
add BooleanHistogram::Accumulate to properly canonicalize values for boolean histograms

Forgot to mention this: !!value is the standard mozilla way of booleanification. Please change this before landing.

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Taras Glek (:taras) from comment #3)
> Really seems like the underlying vector should be of <bool> of kind.

That would be nice.  We talked about this once in the context of saving space and it just wasn't worth doing (IIRC, it was going to save 1-2K of memory).

(In reply to Taras Glek (:taras) from comment #4)
> Forgot to mention this: !!value is the standard mozilla way of
> booleanification. Please change this before landing.

Good point.  I will fix the booleanification of |sample|.

Comment 6

[Nathan Froyd [:froydnj]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/79db8bedd0aa
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e1a74396082e

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/79db8bedd0aa
https://hg.mozilla.org/mozilla-central/rev/e1a74396082e