Issue 1394963002: Adds a couple more measures to the benchmarking app

Issue 1394963002: Adds a couple more measures to the benchmarking app (Closed)

Created:
5 years, 2 months ago by zra

Modified:
5 years, 2 months ago

Reviewers:
ppi

CC:
mojo-reviews_chromium.org, gregsimon, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, tonyg, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org

Base URL:
git@github.com:domokit/mojo.git@master

Target Ref:
refs/heads/master

Project:
mojo

Visibility:
Public.

More Reviews

Description

Adds a couple more measures to the benchmarking app BUG= R=ppi@chromium.org Committed: https://chromium.googlesource.com/external/mojo/+/0d891ea1f48e9f630d1cd524ba60499b0e5760a9

Patch Set 1 #

Patch Set 2 : Format #

Total comments: 6

Patch Set 3 : #

Total comments: 15

Patch Set 4 : Address comments #

Patch Set 5 : #

Total comments: 4

Patch Set 6 : #

Patch Set 7 : #

Total comments: 2

Patch Set 8 : #

Created: 5 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+141 lines, -19 lines)			Patch
M	apps/benchmark/README.md	View	1 2 3	1 chunk	+4 lines, -0 lines	0 comments	Download
M	apps/benchmark/measurements.h	View	1 2 3	3 chunks	+14 lines, -6 lines	0 comments	Download
M	apps/benchmark/measurements.cc	View	1 2 3 4 5 6	8 chunks	+39 lines, -6 lines	0 comments	Download
M	apps/benchmark/measurements_unittest.cc	View	1 2 3 4 5	2 chunks	+42 lines, -5 lines	0 comments	Download
M	apps/benchmark/run_args.cc	View	1 2 3 4 5 6 7	1 chunk	+14 lines, -0 lines	0 comments	Download
M	examples/dart/mojo_rtt_benchmark/lib/main.dart	View	1 2 3	2 chunks	+2 lines, -2 lines	0 comments	Download
M	mojo/devtools/common/mojo_benchmark	View	1 2	1 chunk	+2 lines, -0 lines	0 comments	Download
M	mojo/tools/data/rtt_benchmarks	View	1 2	8 chunks	+24 lines, -0 lines	0 comments	Download

Messages

Total messages: 13 (1 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

ppi

As discussed offline and after chatting with Tony, I think that we should eventually have ...

5 years, 2 months ago (2015-10-08 22:29:47 UTC) #3

zra

https://codereview.chromium.org/1394963002/diff/20001/apps/benchmark/benchmark_app.cc File apps/benchmark/benchmark_app.cc (right): https://codereview.chromium.org/1394963002/diff/20001/apps/benchmark/benchmark_app.cc#newcode122 apps/benchmark/benchmark_app.cc:122: for (double result : results) { On 2015/10/08 22:29:47, ...

5 years, 2 months ago (2015-10-09 14:51:15 UTC) #4

ppi

https://codereview.chromium.org/1394963002/diff/40001/apps/benchmark/measurements.cc File apps/benchmark/measurements.cc (right): https://codereview.chromium.org/1394963002/diff/40001/apps/benchmark/measurements.cc#newcode123 apps/benchmark/measurements.cc:123: if ((percentile < 0.0) || (percentile > 1.0)) { ...

5 years, 2 months ago (2015-10-12 19:19:19 UTC) #5

zra

5 years, 2 months ago (2015-10-12 20:38:19 UTC) #6

ppi

https://codereview.chromium.org/1394963002/diff/40001/apps/benchmark/measurements.cc File apps/benchmark/measurements.cc (right): https://codereview.chromium.org/1394963002/diff/40001/apps/benchmark/measurements.cc#newcode141 apps/benchmark/measurements.cc:141: static_cast<size_t>(static_cast<double>(durations.size()) * percentile); On 2015/10/12 20:38:19, zra wrote: > ...

5 years, 2 months ago (2015-10-12 20:53:55 UTC) #7

zra

https://codereview.chromium.org/1394963002/diff/40001/apps/benchmark/measurements.cc File apps/benchmark/measurements.cc (right): https://codereview.chromium.org/1394963002/diff/40001/apps/benchmark/measurements.cc#newcode141 apps/benchmark/measurements.cc:141: static_cast<size_t>(static_cast<double>(durations.size()) * percentile); On 2015/10/12 20:53:54, ppi wrote: > ...

5 years, 2 months ago (2015-10-12 22:59:33 UTC) #8

https://codereview.chromium.org/1394963002/diff/40001/apps/benchmark/measurem...
File apps/benchmark/measurements.cc (right):

https://codereview.chromium.org/1394963002/diff/40001/apps/benchmark/measurem...
apps/benchmark/measurements.cc:141:
static_cast<size_t>(static_cast<double>(durations.size()) * percentile);
On 2015/10/12 20:53:54, ppi wrote:
> On 2015/10/12 20:38:19, zra wrote:
> > On 2015/10/12 19:19:19, ppi wrote:
> > > I'd expect that we want index in the range [0, size - 1], not [0, size].
Or
> > does
> > > that API want [0, size]?
> > 
> > You're right. Needed to be more careful here. Changed to ceil(size *
> percentile)
> > - 1.
> 
> Why not use the simple formula of percentile * (size-1) ? This would map full
> range of indices [0, size-1]. In particular, this would handle 0.0 (which
> Sheet's PERCENTILE is happy to accept).

I think we should follow the "Nearest Rank Method" described here:

https://en.wikipedia.org/wiki/Percentile

The problem with the simple formula is that the double to int conversion
truncates. E.g.:

90th percentile of [1, 2, 3] should be 3, but:

.9 * (3 - 1) = 1.8, which is truncated to 1.

Value at index 1 is 2. We can't just add 1 to the result, because that will
screw up other examples, e.g. 0.5.

But handling 0.0 sounds better to me, so I just added a special case for it.

https://codereview.chromium.org/1394963002/diff/40001/apps/benchmark/measurem...
File apps/benchmark/measurements_unittest.cc (right):

https://codereview.chromium.org/1394963002/diff/40001/apps/benchmark/measurem...
apps/benchmark/measurements_unittest.cc:157: EXPECT_DOUBLE_EQ(0.012,
regular.Measure(measurement));
On 2015/10/12 20:53:55, ppi wrote:
> On 2015/10/12 20:38:19, zra wrote:
> > On 2015/10/12 19:19:19, ppi wrote:
> > > Please test edge cases (0.0 and 1.0) too.
> > 
> > Added for 1.0. On further reflection 0.0 doesn't make sense, so it is
> excluded.
> > (There is no element >= none of the elements).
> 
> As noted above, I think that we should handle 0.0 (meaning the lowest element)
> and 1.0 (meaning the highest element), as long as it is not an abomination of
> some universally held definition of percentile.

Added test for 0.0.

ppi

https://codereview.chromium.org/1394963002/diff/40001/apps/benchmark/measurements.cc File apps/benchmark/measurements.cc (right): https://codereview.chromium.org/1394963002/diff/40001/apps/benchmark/measurements.cc#newcode141 apps/benchmark/measurements.cc:141: static_cast<size_t>(static_cast<double>(durations.size()) * percentile); On 2015/10/12 22:59:33, zra wrote: > ...

5 years, 2 months ago (2015-10-12 23:15:20 UTC) #9

https://codereview.chromium.org/1394963002/diff/40001/apps/benchmark/measurem...
File apps/benchmark/measurements.cc (right):

https://codereview.chromium.org/1394963002/diff/40001/apps/benchmark/measurem...
apps/benchmark/measurements.cc:141:
static_cast<size_t>(static_cast<double>(durations.size()) * percentile);
On 2015/10/12 22:59:33, zra wrote:
> On 2015/10/12 20:53:54, ppi wrote:
> > On 2015/10/12 20:38:19, zra wrote:
> > > On 2015/10/12 19:19:19, ppi wrote:
> > > > I'd expect that we want index in the range [0, size - 1], not [0, size].
> Or
> > > does
> > > > that API want [0, size]?
> > > 
> > > You're right. Needed to be more careful here. Changed to ceil(size *
> > percentile)
> > > - 1.
> > 
> > Why not use the simple formula of percentile * (size-1) ? This would map
full
> > range of indices [0, size-1]. In particular, this would handle 0.0 (which
> > Sheet's PERCENTILE is happy to accept).
> 
> I think we should follow the "Nearest Rank Method" described here:
> 
> https://en.wikipedia.org/wiki/Percentile
> 
> The problem with the simple formula is that the double to int conversion
> truncates. E.g.:
> 
> 90th percentile of [1, 2, 3] should be 3, but:
> 
> .9 * (3 - 1) = 1.8, which is truncated to 1.
> 
> Value at index 1 is 2. We can't just add 1 to the result, because that will
> screw up other examples, e.g. 0.5.
> 
> But handling 0.0 sounds better to me, so I just added a special case for it.

Ack. Thanks for the reference!

https://codereview.chromium.org/1394963002/diff/80001/apps/benchmark/measurem...
File apps/benchmark/measurements.cc (right):

https://codereview.chromium.org/1394963002/diff/80001/apps/benchmark/measurem...
apps/benchmark/measurements.cc:139: if (percentile == 0.0) {
This probably does what we want, but I'd suggest to skip the "if" and do index =
max(0, index) later (to avoid reasoning about double imprecision).

https://codereview.chromium.org/1394963002/diff/80001/apps/benchmark/measurem...
File apps/benchmark/measurements_unittest.cc (right):

https://codereview.chromium.org/1394963002/diff/80001/apps/benchmark/measurem...
apps/benchmark/measurements_unittest.cc:167: EXPECT_DOUBLE_EQ(0.012,
regular.Measure(measurement));
Please make the measurements on the reversed list too.

zra

https://codereview.chromium.org/1394963002/diff/80001/apps/benchmark/measurements.cc File apps/benchmark/measurements.cc (right): https://codereview.chromium.org/1394963002/diff/80001/apps/benchmark/measurements.cc#newcode139 apps/benchmark/measurements.cc:139: if (percentile == 0.0) { On 2015/10/12 23:15:20, ppi ...

5 years, 2 months ago (2015-10-13 03:35:39 UTC) #10

ppi

lgtm % comment https://codereview.chromium.org/1394963002/diff/120001/apps/benchmark/run_args.cc File apps/benchmark/run_args.cc (right): https://codereview.chromium.org/1394963002/diff/120001/apps/benchmark/run_args.cc#newcode67 apps/benchmark/run_args.cc:67: LOG(ERROR) << "Expected '" << result->param ...

5 years, 2 months ago (2015-10-13 03:55:00 UTC) #11

zra

https://codereview.chromium.org/1394963002/diff/120001/apps/benchmark/run_args.cc File apps/benchmark/run_args.cc (right): https://codereview.chromium.org/1394963002/diff/120001/apps/benchmark/run_args.cc#newcode67 apps/benchmark/run_args.cc:67: LOG(ERROR) << "Expected '" << result->param << "' to ...

5 years, 2 months ago (2015-10-13 05:09:26 UTC) #12

zra

5 years, 2 months ago (2015-10-13 05:09:43 UTC) #13

Message was sent while issue was closed.

Committed patchset #8 (id:140001) manually as
0d891ea1f48e9f630d1cd524ba60499b0e5760a9 (presubmit successful).

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages