Refine WinSAT histogram bounds and rescale so the results are integers.

Refine WinSAT histogram bounds and rescale so the results are integers.


BUG=None.
TEST=chrome://histograms on Windows show valid results for the WinSAT histograms.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150260

[piman, 2012-08-03 22:58:54]

OWNERS-wise LGTM, but please have it reviewed by someone who knows the code /
expectations here (I added zmo).

[Zhenyao Mo, 2012-08-03 23:06:07]

I think you need to define new histograms because the values changed their
meanings (a scale of 10 difference) so the old/new values will mess up the
histogram.

[dtu, 2012-08-03 23:10:55]

On 2012/08/03 23:06:07, Zhenyao Mo wrote:
> I think you need to define new histograms because the values changed their
> meanings (a scale of 10 difference) so the old/new values will mess up the
> histogram.

The histograms are not in histograms.xml yet. Does that still hold true? Also,
histograms have to be integers, right? I'm not being completely ridiculous here?

[Zhenyao Mo, 2012-08-03 23:18:02]

On 2012/08/03 23:10:55, Dave Tu wrote:
> On 2012/08/03 23:06:07, Zhenyao Mo wrote:
> > I think you need to define new histograms because the values changed their
> > meanings (a scale of 10 difference) so the old/new values will mess up the
> > histogram.
> 
> The histograms are not in histograms.xml yet. Does that still hold true? Also,
> histograms have to be integers, right? I'm not being completely ridiculous
here?

The data are still there, histograms.xml only allows you to view them.

I think the float numbers are rounded to the nearest integers, but jar can
confirm.

[jar (doing other things), 2012-08-04 02:04:46]

https://chromiumcodereview.appspot.com/10828165/diff/1/content/gpu/gpu_info_c...
File content/gpu/gpu_info_collector_win.cc (right):

https://chromiumcodereview.appspot.com/10828165/diff/1/content/gpu/gpu_info_c...
content/gpu/gpu_info_collector_win.cc:139:
UMA_HISTOGRAM_ENUMERATION("GPU.WinSAT.OverallScore", stats.overall * 10, 200);
In addition to my comment below....

I was surprised first of all to see that you wanted 200 buckets.  That is
generally larger than most folks would find useful.... but I don't fully
understand your data, and your expected distribution.

Looking at the old code, it seemed to suggest that teh interesting values of
"stats.overall" was going to be values between 0 and 50.  In addition, since you
said you had a total of 50 buckets... you got pretty close to one bucket per
unit interval (subtle reason why it was not exact... and you probably had a top
end interval like 48-50 as one bucket).

The new range suggests that you want to see values where stats.overall * 10 is
in the range of 0-200.  Hence you're now only interested in stats.overall in the
range of 0 to 20 (rather than your previous interest up to 50).  ...and in
addition, you wanted that smaller range broken up into a super fine grain list
of 200 separate buckets.

I took a look at the collected data, and can see that it is it is indeed super
rare to even go beyond 7.

As a result, it probably makes sense that you are now looking only at the range
of 0-20 (scaled by a factor of 10).

...however... the "cool" way to do this is not to have a giant linear histogram
(re: using HISTOGRAM_ENUMERATION), but instead let the magic of the exponential
histogram help you.

I'd suggest using:

UMA_HISTOGRAM_CUSTOM_COUNTS("name", overall * 10, 1, 200, 50);

That will give you a LOT of resolution down near the lower end of the range
1-200, but not waste buckets looking at narrow details in the upper range.

YMMV... but 200 buckets is a lot of buckets... and probably not needed (and the
suggestion above will only gather 50 buckets).

https://chromiumcodereview.appspot.com/10828165/diff/1/content/gpu/gpu_info_c...
content/gpu/gpu_info_collector_win.cc:142:
UMA_HISTOGRAM_ENUMERATION("GPU.WinSAT.GamingScore", stats.gaming * 10, 200);
Best practice, when changing the parameters by which you defined a histogram, is
to change the name to a new name.  This way, if you look at "All" results from
all versions, you won't get a confusing conflation of the two histograms.

Common practice is just to add a trailing "2" or such to the "old" name to
define a new name.

You should indeed land the old names in our XML file, so that we can see old
data.  I've temporarily landed GPU.WiinSat_OverallScore so you can now glance at
what data we've already collected.

[dtu, 2012-08-04 03:17:34]

On 2012/08/04 02:04:46, jar wrote:
>
https://chromiumcodereview.appspot.com/10828165/diff/1/content/gpu/gpu_info_c...
> File content/gpu/gpu_info_collector_win.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10828165/diff/1/content/gpu/gpu_info_c...
> content/gpu/gpu_info_collector_win.cc:139:
> UMA_HISTOGRAM_ENUMERATION("GPU.WinSAT.OverallScore", stats.overall * 10, 200);
> In addition to my comment below....
> 
> I was surprised first of all to see that you wanted 200 buckets.  That is
> generally larger than most folks would find useful.... but I don't fully
> understand your data, and your expected distribution.
> 
> Looking at the old code, it seemed to suggest that teh interesting values of
> "stats.overall" was going to be values between 0 and 50.  In addition, since
you
> said you had a total of 50 buckets... you got pretty close to one bucket per
> unit interval (subtle reason why it was not exact... and you probably had a
top
> end interval like 48-50 as one bucket).
> 
> The new range suggests that you want to see values where stats.overall * 10 is
> in the range of 0-200.  Hence you're now only interested in stats.overall in
the
> range of 0 to 20 (rather than your previous interest up to 50).  ...and in
> addition, you wanted that smaller range broken up into a super fine grain list
> of 200 separate buckets.
> 
> I took a look at the collected data, and can see that it is it is indeed super
> rare to even go beyond 7.
> 
> As a result, it probably makes sense that you are now looking only at the
range
> of 0-20 (scaled by a factor of 10).
> 
> ...however... the "cool" way to do this is not to have a giant linear
histogram
> (re: using HISTOGRAM_ENUMERATION), but instead let the magic of the
exponential
> histogram help you.
> 
> I'd suggest using:
> 
> UMA_HISTOGRAM_CUSTOM_COUNTS("name", overall * 10, 1, 200, 50);
> 
> That will give you a LOT of resolution down near the lower end of the range
> 1-200, but not waste buckets looking at narrow details in the upper range.
> 
> YMMV... but 200 buckets is a lot of buckets... and probably not needed (and
the
> suggestion above will only gather 50 buckets).
> 
>
https://chromiumcodereview.appspot.com/10828165/diff/1/content/gpu/gpu_info_c...
> content/gpu/gpu_info_collector_win.cc:142:
> UMA_HISTOGRAM_ENUMERATION("GPU.WinSAT.GamingScore", stats.gaming * 10, 200);
> Best practice, when changing the parameters by which you defined a histogram,
is
> to change the name to a new name.  This way, if you look at "All" results from
> all versions, you won't get a confusing conflation of the two histograms.
> 
> Common practice is just to add a trailing "2" or such to the "old" name to
> define a new name.
> 
> You should indeed land the old names in our XML file, so that we can see old
> data.  I've temporarily landed GPU.WiinSat_OverallScore so you can now glance
at
> what data we've already collected.

Thanks for adding that temporary histogram to the dashboard! I actually didn't
know what the data would look like, so that's very helpful. Is there a way to
visualize the data without landing a patch?

About the actual distribution of the data: it's a number representing the
performance of the machine, gathered when Chrome is launched. Therefore, it
should slowly go up over time, with no theoretical limit, so some futureproofing
is needed.

There are, however, artificial limits in Windows, that also go up, with each
Windows release. These give a sense of how we expect the numbers to go up over
time. They are 5.9 on Windows VIsta, 7.9 on Windows 7, and 9.9 on Windows 8. I'm
guessing the ones over 9 are people who manually changed their scores, or
employees at Microsoft testing the next Windows release.

The numbers are expressed with one decimal place, so my first thought was to
just make a bucket for each possible value; but you're right; that leads to too
many buckets. Your suggestion is a good one. But it's also possible to get a
sample of 0, if we failed to get the numbers. Should we just ignore those? Note
there is another histogram here that checks for it, GPU.WinSAT.HasResults.

[jar (doing other things), 2012-08-06 18:37:08]

Sample of zero is not a problem.  We always have an "underflow" bucket that
lists as [0-k), where k is the declared minimum.  In truth, it actually picks up
any negative samples as well (in case your metric is really horked... so that we
don't discard "data."

Jim

On 2012/08/04 03:17:34, Dave Tu wrote:
> On 2012/08/04 02:04:46, jar wrote:
> >
>
https://chromiumcodereview.appspot.com/10828165/diff/1/content/gpu/gpu_info_c...
> > File content/gpu/gpu_info_collector_win.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10828165/diff/1/content/gpu/gpu_info_c...
> > content/gpu/gpu_info_collector_win.cc:139:
> > UMA_HISTOGRAM_ENUMERATION("GPU.WinSAT.OverallScore", stats.overall * 10,
200);
> > In addition to my comment below....
> > 
> > I was surprised first of all to see that you wanted 200 buckets.  That is
> > generally larger than most folks would find useful.... but I don't fully
> > understand your data, and your expected distribution.
> > 
> > Looking at the old code, it seemed to suggest that teh interesting values of
> > "stats.overall" was going to be values between 0 and 50.  In addition, since
> you
> > said you had a total of 50 buckets... you got pretty close to one bucket per
> > unit interval (subtle reason why it was not exact... and you probably had a
> top
> > end interval like 48-50 as one bucket).
> > 
> > The new range suggests that you want to see values where stats.overall * 10
is
> > in the range of 0-200.  Hence you're now only interested in stats.overall in
> the
> > range of 0 to 20 (rather than your previous interest up to 50).  ...and in
> > addition, you wanted that smaller range broken up into a super fine grain
list
> > of 200 separate buckets.
> > 
> > I took a look at the collected data, and can see that it is it is indeed
super
> > rare to even go beyond 7.
> > 
> > As a result, it probably makes sense that you are now looking only at the
> range
> > of 0-20 (scaled by a factor of 10).
> > 
> > ...however... the "cool" way to do this is not to have a giant linear
> histogram
> > (re: using HISTOGRAM_ENUMERATION), but instead let the magic of the
> exponential
> > histogram help you.
> > 
> > I'd suggest using:
> > 
> > UMA_HISTOGRAM_CUSTOM_COUNTS("name", overall * 10, 1, 200, 50);
> > 
> > That will give you a LOT of resolution down near the lower end of the range
> > 1-200, but not waste buckets looking at narrow details in the upper range.
> > 
> > YMMV... but 200 buckets is a lot of buckets... and probably not needed (and
> the
> > suggestion above will only gather 50 buckets).
> > 
> >
>
https://chromiumcodereview.appspot.com/10828165/diff/1/content/gpu/gpu_info_c...
> > content/gpu/gpu_info_collector_win.cc:142:
> > UMA_HISTOGRAM_ENUMERATION("GPU.WinSAT.GamingScore", stats.gaming * 10, 200);
> > Best practice, when changing the parameters by which you defined a
histogram,
> is
> > to change the name to a new name.  This way, if you look at "All" results
from
> > all versions, you won't get a confusing conflation of the two histograms.
> > 
> > Common practice is just to add a trailing "2" or such to the "old" name to
> > define a new name.
> > 
> > You should indeed land the old names in our XML file, so that we can see old
> > data.  I've temporarily landed GPU.WiinSat_OverallScore so you can now
glance
> at
> > what data we've already collected.
> 
> Thanks for adding that temporary histogram to the dashboard! I actually didn't
> know what the data would look like, so that's very helpful. Is there a way to
> visualize the data without landing a patch?
> 
> About the actual distribution of the data: it's a number representing the
> performance of the machine, gathered when Chrome is launched. Therefore, it
> should slowly go up over time, with no theoretical limit, so some
futureproofing
> is needed.
> 
> There are, however, artificial limits in Windows, that also go up, with each
> Windows release. These give a sense of how we expect the numbers to go up over
> time. They are 5.9 on Windows VIsta, 7.9 on Windows 7, and 9.9 on Windows 8.
I'm
> guessing the ones over 9 are people who manually changed their scores, or
> employees at Microsoft testing the next Windows release.
> 
> The numbers are expressed with one decimal place, so my first thought was to
> just make a bucket for each possible value; but you're right; that leads to
too
> many buckets. Your suggestion is a good one. But it's also possible to get a
> sample of 0, if we failed to get the numbers. Should we just ignore those?
Note
> there is another histogram here that checks for it, GPU.WinSAT.HasResults.

[dtu, 2012-08-06 21:41:47]

Cool! SGTM

https://chromiumcodereview.appspot.com/10828165/diff/1/content/gpu/gpu_info_c...
File content/gpu/gpu_info_collector_win.cc (right):

https://chromiumcodereview.appspot.com/10828165/diff/1/content/gpu/gpu_info_c...
content/gpu/gpu_info_collector_win.cc:139:
UMA_HISTOGRAM_ENUMERATION("GPU.WinSAT.OverallScore", stats.overall * 10, 200);
On 2012/08/04 02:04:46, jar wrote:
> In addition to my comment below....
> 
> I was surprised first of all to see that you wanted 200 buckets.  That is
> generally larger than most folks would find useful.... but I don't fully
> understand your data, and your expected distribution.
> 
> Looking at the old code, it seemed to suggest that teh interesting values of
> "stats.overall" was going to be values between 0 and 50.  In addition, since
you
> said you had a total of 50 buckets... you got pretty close to one bucket per
> unit interval (subtle reason why it was not exact... and you probably had a
top
> end interval like 48-50 as one bucket).
> 
> The new range suggests that you want to see values where stats.overall * 10 is
> in the range of 0-200.  Hence you're now only interested in stats.overall in
the
> range of 0 to 20 (rather than your previous interest up to 50).  ...and in
> addition, you wanted that smaller range broken up into a super fine grain list
> of 200 separate buckets.
> 
> I took a look at the collected data, and can see that it is it is indeed super
> rare to even go beyond 7.
> 
> As a result, it probably makes sense that you are now looking only at the
range
> of 0-20 (scaled by a factor of 10).
> 
> ...however... the "cool" way to do this is not to have a giant linear
histogram
> (re: using HISTOGRAM_ENUMERATION), but instead let the magic of the
exponential
> histogram help you.
> 
> I'd suggest using:
> 
> UMA_HISTOGRAM_CUSTOM_COUNTS("name", overall * 10, 1, 200, 50);
> 
> That will give you a LOT of resolution down near the lower end of the range
> 1-200, but not waste buckets looking at narrow details in the upper range.
> 
> YMMV... but 200 buckets is a lot of buckets... and probably not needed (and
the
> suggestion above will only gather 50 buckets).
> 

Done.

https://chromiumcodereview.appspot.com/10828165/diff/1/content/gpu/gpu_info_c...
content/gpu/gpu_info_collector_win.cc:142:
UMA_HISTOGRAM_ENUMERATION("GPU.WinSAT.GamingScore", stats.gaming * 10, 200);
On 2012/08/04 02:04:46, jar wrote:
> Best practice, when changing the parameters by which you defined a histogram,
is
> to change the name to a new name.  This way, if you look at "All" results from
> all versions, you won't get a confusing conflation of the two histograms.
> 
> Common practice is just to add a trailing "2" or such to the "old" name to
> define a new name.
> 
> You should indeed land the old names in our XML file, so that we can see old
> data.  I've temporarily landed GPU.WiinSat_OverallScore so you can now glance
at
> what data we've already collected.

Done.

[piman, 2012-08-06 21:49:29]

lgtm

[jar (doing other things), 2012-08-06 22:30:35]

lgtm

[commit-bot: I haz the power, 2012-08-06 22:55:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtu@chromium.org/10828165/6002

[commit-bot: I haz the power, 2012-08-06 23:25:27]

Try job failure for 10828165-6002 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-07 01:18:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtu@chromium.org/10828165/6002

[commit-bot: I haz the power, 2012-08-07 04:00:02]

Change committed as 150260