don't record or reflect sum/sum_squares information for exponential histograms
RESOLVED
FIXED
in mozilla20
Status
()
People
(Reporter: froydnj, Assigned: froydnj)
Tracking
Firefox Tracking Flags
(Not tracked)
Details
Attachments
(2 attachments, 3 obsolete attachments)
1.56 KB,
patch

vladan
:
review+

Details  Diff  Splinter Review 
11.92 KB,
patch

vladan
:
review+

Details  Diff  Splinter Review 
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.
Assignee  
Comment 1•7 years ago


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.
Attachment #689836 
Flags: review?(vdjeric)
Assignee  
Comment 2•7 years ago


This is the interesting part; I didn't try separating things out because there's way too many ways to break things through interdependencies.
Attachment #689837 
Flags: review?(vdjeric)
Updated•7 years ago

Attachment #689836 
Flags: review?(vdjeric) → review+
Comment 3•7 years ago


Comment on attachment 689837 [details] [diff] [review] part 1  modify exponential histograms to only record log_sum{,_squares} ># HG changeset patch ># User Nathan Froyd <froydnj@gmail.com> > >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.
Attachment #689837 
Flags: review?(vdjeric)
Assignee  
Comment 4•7 years ago


First part committed: remote: https://hg.mozilla.org/integration/mozillainbound/rev/d462bea24982
Whiteboard: [leave open]
Assignee  
Comment 6•7 years ago


Revised patch according to review comments.
Attachment #689837 
Attachment is obsolete: true
Attachment #691949 
Flags: review?(vdjeric)
Comment 7•7 years ago


Comment on attachment 691949 [details] [diff] [review] part 1  modify exponential histograms to only record log_sum{,_squares} >+ // Export sum_squares as two separate 32bit 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 coordinate 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
Attachment #691949 
Flags: review?(vdjeric) → review+
Comment 8•7 years ago


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?
Attachment #691949 
Flags: review+
Assignee  
Comment 9•7 years ago


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, speedwise, 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•7 years ago


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
Assignee  
Comment 11•7 years ago


Here's a version that continues to export sum everywhere, but is more selective about when log_sum{,_squares} and sum_squares gets exported.
Attachment #691949 
Attachment is obsolete: true
Attachment #692447 
Flags: review?(vdjeric)
Assignee  
Comment 12•7 years ago


This time with the explicit truncations and not including experiments.
Attachment #692447 
Attachment is obsolete: true
Attachment #692447 
Flags: review?(vdjeric)
Attachment #692453 
Flags: review?(vdjeric)
Comment 13•7 years ago


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 singleprecision? 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 32bit to 64bit 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?
Attachment #692453 
Flags: review?(vdjeric) → review+
Assignee  
Comment 14•7 years ago


(In reply to Vladan Djeric (:vladan) from comment #13) > Did you file a bug for changing log computation to singleprecision? 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 32bit to 64bit 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 preexisting 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.
Assignee  
Comment 15•7 years ago


remote: https://hg.mozilla.org/integration/mozillainbound/rev/b8fd4a16afc8
Assignee: nobody → nfroyd
Whiteboard: [leave open]
Comment 16•7 years ago


https://hg.mozilla.org/mozillacentral/rev/b8fd4a16afc8
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: intestsuite+
Resolution:  → FIXED
Target Milestone:  → mozilla20
You need to log in
before you can comment on or make changes to this bug.
Description
•