819418 - don't record or reflect sum/sum_squares information for exponential histograms

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Nathan Froyd [:froydnj]]

Per bug 816166 comment 23, sum and sum_squares aren't going to be used for exponential histograms.  In the interest of slimming the telemetry code, we should dispense with useless work.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 0 - Histogram::SampleSet:Add needs to transfer all values, not just sum_

This is actually a bugfix; our STARTUP_* histogram numbers are totally wrong
after bug 816166 because we weren't transferring over our new statistics to
the cloned histograms.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 1 - modify exponential histograms to only record log_sum{,_squares}

This is the interesting part; I didn't try separating things out because
there's way too many ways to break things through interdependencies.

Comment 3

[Vladan Djeric (:vladan)]

Comment on attachment 689837 [details] [diff] [review]
part 1 - modify exponential histograms to only record log_sum{,_squares}

># HG changeset patch
># User Nathan Froyd <[e-mail]>
>
>part 1 - modify exponential histograms to only record log_sum{,_squares}

Initial impressions review

>+void Histogram::SampleSet::Accumulate(Sample value, Count count,
>+				      size_t index) {
>+  DCHECK(count == 1 || count == -1);
>+  counts_[index] += count;
>+  redundant_count_ += count;
>+  DCHECK_GE(counts_[index], 0);
>+  DCHECK_GE(redundant_count_, 0);
>+}

We don't need "value" in the parameter list, it's never used. I'd also suggest changing the function name since it's not really accumulating.


>diff --git a/ipc/chromium/src/base/histogram.h b/ipc/chromium/src/base/histogram.h

>+    void Accumulate(Sample value, Count count, size_t index);
>+

Add description of function or use more descriptive name


>diff --git a/toolkit/components/telemetry/Telemetry.cpp b/toolkit/components/telemetry/Telemetry.cpp


> bool
> IsEmpty(const Histogram *h)
> {
>   Histogram::SampleSet ss;
>   h->SnapshotSample(&ss);
> 
>-  return ss.counts(0) == 0 && ss.sum() == 0;
>+  return ss.counts(0) == 0 && 
>+    (h->histogram_type() == Histogram::HISTOGRAM
>+     ? ss.log_sum() == 0
>+     : ss.sum() == 0);
> }

You don't need the conditional expression: "return ss.counts(0) == 0 && ss.sum() == 0 && ss.log_sum() == 0;"

If we decide to go ahead with this change, we might need to update the "sum" field in the about:telemetry histograms section.

Comment 4

[Nathan Froyd [:froydnj]]

First part committed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d462bea24982

Comment 5

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/d462bea24982

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: part 1 - modify exponential histograms to only record log_sum{,_squares}

Revised patch according to review comments.

Comment 7

[Vladan Djeric (:vladan)]

Comment on attachment 691949 [details] [diff] [review]
part 1 - modify exponential histograms to only record log_sum{,_squares}

>+    // Export |sum_squares| as two separate 32-bit properties so that we
>+    // can accurately reconstruct it on the analysis side.
>+    uint64_t sum_squares = ss.sum_squares();
>+    uint32_t lo = sum_squares;

Do we need an explicit cast to remove compiler warnings?

>+    if (hgram.histogram_type == Telemetry.HISTOGRAM_EXPONENTIAL) {
>+      retgram.log_sum = hgram.log_sum;
>+      retgram.log_sum_squares = hgram.log_sum_squares;
>+    } else {
>+      retgram.sum = hgram.sum;
>+      retgram.sum_squares_lo = hgram.sum_squares_lo;
>+      retgram.sum_squares_hi = hgram.sum_squares_hi;
>+    }

We should co-ordinate with Metrics to make sure we don't land this if their parsing code can't handle an absent "sum" field.

>+  if (s1.histogram_type == Telemetry.HISTOGRAM_EXPONENTIAL) {
>+    do_check_eq(s1.log_sum, s2.log_sum);
>+    do_check_eq(s1.log_sum_squares, s2.log_sum_squares);
>+  } else {
>+    do_check_eq(s1.sum, s2.sum);
>+    do_check_eq(s1.sum_squares_lo, s2.sum_squares_lo);
>+    do_check_eq(s1.sum_squares_hi, s2.sum_squares_hi);
>+  }

Might as well add a checks that "log_sum" etc doesn't exist in flag histograms and same for exponential histograms

Comment 8

[Vladan Djeric (:vladan)]

Comment on attachment 691949 [details] [diff] [review]
part 1 - modify exponential histograms to only record log_sum{,_squares}

I think we should give a bit more thought to the impact of removing the "sum" field from exponential histograms. By chance, I ran into a user of this field and it's reasonable to assume there would be more: https://builder.addons.mozilla.org/package/162308/

Also, did you test whether removing this field breaks about:telemetry?

Comment 9

[Nathan Froyd [:froydnj]]

I didn't check about:telemetry, but I wouldn't be too surprised if things did break.

If we're concerned about not breaking external consumers, then let's just drop part 1 on the floor.  We'd get more benefit, speed-wise, from making the log_sum accumulation compute in single precision rather than double precision.  If you agree, then let's close this bug and I'll open a new one for changing the computation precision.

Comment 10

[Vladan Djeric (:vladan)]

I'd like to keep most of the "part 1" patch. i.e. all histograms have "sum" but only exponential has log_sum & log_sum_squares + only linear has sum_squares

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: part 1 - be more selective in exporting aggregate statistics

Here's a version that continues to export sum everywhere, but is more
selective about when log_sum{,_squares} and sum_squares gets exported.

Comment 12

[Nathan Froyd [:froydnj]]

Attachment: part 1 - be more selective in exporting aggregate statistics

This time with the explicit truncations and not including experiments.

Comment 13

[Vladan Djeric (:vladan)]

Comment on attachment 692453 [details] [diff] [review]
part 1 - be more selective in exporting aggregate statistics

Did you file a bug for changing log computation to single-precision? Or were you planning to land that change in this bug?

>+void Histogram::SampleSet::Accumulate(Sample value, Count count,
>+				      size_t index) {
>   DCHECK(count == 1 || count == -1);
>   counts_[index] += count;
>-  int64_t amount = static_cast<int64_t>(count) * value;
>-  sum_ += amount;
>-  sum_squares_ += amount * value;
>   redundant_count_ += count;
>+  sum_+= static_cast<int64_t>(count) * value;

Space between "sum_" and "+=". 
I don't think we need the static_casts for 32-bit to 64-bit conversions.

>   DCHECK_GE(counts_[index], 0);
>-  DCHECK_GE(sum_, 0);
>   DCHECK_GE(redundant_count_, 0);
> }

Put the "DCHECK_GE(sum_, 0)" back in Histogram::SampleSet::Accumulate

>   // Check that clearing works.
>   h.clear();
>   var s = h.snapshot();
>   for each(var i in s.counts) {
>     do_check_eq(i, 0);
>   }
>-  do_check_eq(s.sum, 0);
>+  do_check_eq(histogram_type == Telemetry.HISTOGRAM_EXPONENTIAL
>+	      ? s.log_sum : s.sum, 0);

Why change this?

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Vladan Djeric (:vladan) from comment #13)
> Did you file a bug for changing log computation to single-precision? Or were
> you planning to land that change in this bug?

I haven't.  I'll file another bug for that.

> >+void Histogram::SampleSet::Accumulate(Sample value, Count count,
> >+				      size_t index) {
> >   DCHECK(count == 1 || count == -1);
> >   counts_[index] += count;
> >-  int64_t amount = static_cast<int64_t>(count) * value;
> >-  sum_ += amount;
> >-  sum_squares_ += amount * value;
> >   redundant_count_ += count;
> >+  sum_+= static_cast<int64_t>(count) * value;
> 
> Space between "sum_" and "+=". 
> I don't think we need the static_casts for 32-bit to 64-bit conversions.

We do!  Otherwise, we get a int x int -> int multiply and an extension of the result to 64 bits, instead of an int x int -> int64_t multiply.  That was a pre-existing bug; I'd be happy to split that out into a separate fix (and may in fact do that).  Compilation of:

#include <stdint.h>

int64_t
madd(int64_t a, int b, int c)
{
  a += b * c;
  return a;
}

int64_t
madd2(int64_t a, int b, int c)
{
  a += ((int64_t)b)*c;
  return a;
}

gives (both GCC and clang, -m32 -O2):

madd:
.LFB0:
	.cfi_startproc
	movl	16(%esp), %eax
	imull	12(%esp), %eax
	movl	%eax, %edx
	sarl	$31, %edx
	addl	4(%esp), %eax
	adcl	8(%esp), %edx
	ret
	.cfi_endproc
.LFE0:
	.size	madd, .-madd
	.p2align 4,,15
	.globl	madd2
	.type	madd2, @function
madd2:
.LFB1:
	.cfi_startproc
	movl	16(%esp), %eax
	imull	12(%esp)
	addl	4(%esp), %eax
	adcl	8(%esp), %edx
	ret

The rules on IMULL are a little peculiar, but we really do want the second version, not the first.

> >   DCHECK_GE(counts_[index], 0);
> >-  DCHECK_GE(sum_, 0);
> >   DCHECK_GE(redundant_count_, 0);
> > }
> 
> Put the "DCHECK_GE(sum_, 0)" back in Histogram::SampleSet::Accumulate

Will do.

> >   // Check that clearing works.
> >   h.clear();
> >   var s = h.snapshot();
> >   for each(var i in s.counts) {
> >     do_check_eq(i, 0);
> >   }
> >-  do_check_eq(s.sum, 0);
> >+  do_check_eq(histogram_type == Telemetry.HISTOGRAM_EXPONENTIAL
> >+	      ? s.log_sum : s.sum, 0);
> 
> Why change this?

Whoops, forgot to remove this bit.  I'll add a do_check_eq for s.log_sum == 0 too.

Comment 15

[Nathan Froyd [:froydnj]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b8fd4a16afc8

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b8fd4a16afc8