Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(34)

Issue 2089833002: Entry point for bisect sample comparison. (Closed)

Created:
4 years, 6 months ago by RobertoCN
Modified:
4 years, 2 months ago
CC:
catapult-reviews_chromium.org, tracing-review_chromium.org
Base URL:
https://github.com/catapult-project/catapult.git@mann
Target Ref:
refs/heads/master
Project:
catapult
Visibility:
Public.

Description

Entry point for bisect comparison. This is meant to enable bisect to call stats logic in catapult. The entrypoint will be tracing/bin/compare_samples, it will take paths to the output of the test runs for each sample and a metric string as given by the dashboard. This patch is meant to support chartjson, valueset and buildbot result line output formats. BUG=chromium:616932 R=eakuefner,benjhayden Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/c159bab23ac4d39b5ae9c29cf4c448f052b08c11

Patch Set 1 #

Patch Set 2 : now printing the result to stdout. #

Patch Set 3 : . #

Patch Set 4 : Added tests. #

Total comments: 2

Patch Set 5 : Adding support for valueset #

Patch Set 6 : Adding tests that pass #

Total comments: 12

Patch Set 7 : Addressing feedback. #

Patch Set 8 : A couple more changes. #

Total comments: 39

Patch Set 9 : Addressing lots of feedback, adding buildbot support, rebasing. #

Total comments: 10

Patch Set 10 : Addressing feedback, adding test for chartjsonhistogram. #

Total comments: 2

Patch Set 11 : Removed stray line. #

Total comments: 21

Patch Set 12 : Addressed feedback from Ned & Dave. #

Total comments: 3

Patch Set 13 : Rebasing fixes. #

Total comments: 6

Patch Set 14 : Addressing Ben's nits. #

Total comments: 24

Patch Set 15 : Moved compareSamples and constants to tr.b.Statistics #

Total comments: 10

Patch Set 16 : Addressing last round of feedback. #

Total comments: 2

Patch Set 17 : Closing low level file handle for temp file. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+795 lines, -9 lines) Patch
A tracing/bin/compare_samples View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +53 lines, -0 lines 0 comments Download
M tracing/tracing/base/statistics.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +21 lines, -9 lines 0 comments Download
A tracing/tracing/metrics/buildbot_output_for_compare_samples_test.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +187 lines, -0 lines 0 comments Download
A tracing/tracing/metrics/compare_samples.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +55 lines, -0 lines 0 comments Download
A tracing/tracing/metrics/compare_samples_cmdline.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +253 lines, -0 lines 0 comments Download
A tracing/tracing/metrics/compare_samples_unittest.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +225 lines, -0 lines 0 comments Download
A tracing/tracing/metrics/valueset_output_for_compare_samples_test.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 67 (11 generated)
RobertoCN
4 years, 6 months ago (2016-06-21 22:04:54 UTC) #1
RobertoCN
On 2016/06/21 22:04:54, RobertoCN wrote: Could you please review the overall approach? I expect to ...
4 years, 6 months ago (2016-06-21 22:15:47 UTC) #2
eakuefner
overall approach looks good, but i wonder if this all belongs in tracing/metrics instead. Almost ...
4 years, 5 months ago (2016-06-30 01:29:02 UTC) #3
RobertoCN
On 2016/06/30 01:29:02, eakuefner wrote: > overall approach looks good, but i wonder if this ...
4 years, 5 months ago (2016-06-30 23:21:55 UTC) #4
RobertoCN
Could you take another look at this progress. I still have to add support for ...
4 years, 5 months ago (2016-07-12 00:01:44 UTC) #5
eakuefner
Just to update this with the consensus that's been emerging in-person over the last couple ...
4 years, 5 months ago (2016-07-13 23:59:28 UTC) #8
RobertoCN
PTAL. https://codereview.chromium.org/2089833002/diff/60001/tracing/tracing/metrics/compare_samples.html File tracing/tracing/metrics/compare_samples.html (right): https://codereview.chromium.org/2089833002/diff/60001/tracing/tracing/metrics/compare_samples.html#newcode3 tracing/tracing/metrics/compare_samples.html:3: Copyright (c) 2016 The Chromium Authors. All rights ...
4 years, 5 months ago (2016-07-18 22:52:45 UTC) #9
RobertoCN
Ping.
4 years, 5 months ago (2016-07-19 20:05:42 UTC) #10
RobertoCN
On 2016/07/19 20:05:42, RobertoCN wrote: > Ping. https://www.youtube.com/watch?v=63326NF8EeA
4 years, 5 months ago (2016-07-20 22:33:06 UTC) #11
eakuefner
Sorry for the latency; some more comments. global nit: please use === instead of == ...
4 years, 5 months ago (2016-07-21 19:35:34 UTC) #12
RobertoCN
PTAL
4 years, 5 months ago (2016-07-22 05:20:14 UTC) #13
benjhayden
Is this CL still relevant? Can you rename results-valueset.json to test-valueset.json?
4 years, 4 months ago (2016-08-10 17:22:50 UTC) #14
benjhayden
https://codereview.chromium.org/2089833002/diff/140001/tracing/tracing/metrics/compare_samples.html File tracing/tracing/metrics/compare_samples.html (right): https://codereview.chromium.org/2089833002/diff/140001/tracing/tracing/metrics/compare_samples.html#newcode18 tracing/tracing/metrics/compare_samples.html:18: SIGNIFICANCE_LEVEL: 0.05, Can these constants go in tr.v.Numeric so ...
4 years, 4 months ago (2016-08-10 17:40:22 UTC) #15
benjhayden
Should this support buildbot output format for cc perf tests?
4 years, 4 months ago (2016-08-18 21:41:55 UTC) #16
benjhayden
https://codereview.chromium.org/2089833002/diff/140001/tracing/tracing/metrics/compare_samples.html File tracing/tracing/metrics/compare_samples.html (right): https://codereview.chromium.org/2089833002/diff/140001/tracing/tracing/metrics/compare_samples.html#newcode21 tracing/tracing/metrics/compare_samples.html:21: function geoMeanFromHistogram(h) { On 2016/08/10 at 17:40:21, benjhayden wrote: ...
4 years, 4 months ago (2016-08-18 22:06:34 UTC) #17
benjhayden
https://codereview.chromium.org/2089833002/diff/140001/tracing/tracing/metrics/compare_samples.html File tracing/tracing/metrics/compare_samples.html (right): https://codereview.chromium.org/2089833002/diff/140001/tracing/tracing/metrics/compare_samples.html#newcode21 tracing/tracing/metrics/compare_samples.html:21: function geoMeanFromHistogram(h) { On 2016/08/10 at 17:40:21, benjhayden wrote: ...
4 years, 4 months ago (2016-08-18 22:06:34 UTC) #18
benjhayden
https://codereview.chromium.org/2089833002/diff/140001/tracing/tracing/metrics/compare_samples.html File tracing/tracing/metrics/compare_samples.html (right): https://codereview.chromium.org/2089833002/diff/140001/tracing/tracing/metrics/compare_samples.html#newcode150 tracing/tracing/metrics/compare_samples.html:150: if (value.name != valueName) { var filtered = []; ...
4 years, 4 months ago (2016-08-18 22:14:11 UTC) #19
benjhayden
4 years, 4 months ago (2016-08-18 22:14:12 UTC) #20
benjhayden
https://codereview.chromium.org/2089833002/diff/140001/tracing/tracing/metrics/compare_samples.html File tracing/tracing/metrics/compare_samples.html (right): https://codereview.chromium.org/2089833002/diff/140001/tracing/tracing/metrics/compare_samples.html#newcode218 tracing/tracing/metrics/compare_samples.html:218: // Diagnostics Please open a bug to remove this ...
4 years, 4 months ago (2016-08-19 00:38:53 UTC) #21
nednguyen
On 2016/08/19 00:38:53, benjhayden wrote: > https://codereview.chromium.org/2089833002/diff/140001/tracing/tracing/metrics/compare_samples.html > File tracing/tracing/metrics/compare_samples.html (right): > > https://codereview.chromium.org/2089833002/diff/140001/tracing/tracing/metrics/compare_samples.html#newcode218 > ...
4 years, 4 months ago (2016-08-19 00:48:43 UTC) #22
RobertoCN
publishing my drafts https://codereview.chromium.org/2089833002/diff/140001/tracing/bin/compare_samples File tracing/bin/compare_samples (right): https://codereview.chromium.org/2089833002/diff/140001/tracing/bin/compare_samples#newcode28 tracing/bin/compare_samples:28: parser.add_argument('--valueset', dest='format', action='store_const', Add --buildbot https://codereview.chromium.org/2089833002/diff/140001/tracing/tracing/metrics/compare_samples.py ...
4 years, 3 months ago (2016-09-01 19:28:23 UTC) #23
RobertoCN
PTAL.. I addressed all the feedback that I consider should be done in this CL. ...
4 years, 3 months ago (2016-09-02 21:19:31 UTC) #27
benjhayden
https://codereview.chromium.org/2089833002/diff/140001/tracing/tracing/metrics/compare_samples.html File tracing/tracing/metrics/compare_samples.html (right): https://codereview.chromium.org/2089833002/diff/140001/tracing/tracing/metrics/compare_samples.html#newcode18 tracing/tracing/metrics/compare_samples.html:18: SIGNIFICANCE_LEVEL: 0.05, On 2016/09/02 at 21:19:31, RobertoCN wrote: > ...
4 years, 3 months ago (2016-09-02 23:04:40 UTC) #28
RobertoCN
Thanks for the feedback! It's been addressed, PTAL. https://codereview.chromium.org/2089833002/diff/140001/tracing/tracing/metrics/compare_samples.html File tracing/tracing/metrics/compare_samples.html (right): https://codereview.chromium.org/2089833002/diff/140001/tracing/tracing/metrics/compare_samples.html#newcode18 tracing/tracing/metrics/compare_samples.html:18: SIGNIFICANCE_LEVEL: ...
4 years, 3 months ago (2016-09-03 00:22:00 UTC) #29
RobertoCN
Gentle ping. This is blocking stuff.
4 years, 3 months ago (2016-09-07 00:57:46 UTC) #30
benjhayden
one nit then lgtm https://codereview.chromium.org/2089833002/diff/180001/tracing/tracing/metrics/compare_samples.html File tracing/tracing/metrics/compare_samples.html (right): https://codereview.chromium.org/2089833002/diff/180001/tracing/tracing/metrics/compare_samples.html#newcode225 tracing/tracing/metrics/compare_samples.html:225: var current; delete
4 years, 3 months ago (2016-09-07 16:18:26 UTC) #31
benjhayden
still lgtm, and please wait for other reviewers https://codereview.chromium.org/2089833002/diff/140001/tracing/tracing/metrics/compare_samples.html File tracing/tracing/metrics/compare_samples.html (right): https://codereview.chromium.org/2089833002/diff/140001/tracing/tracing/metrics/compare_samples.html#newcode18 tracing/tracing/metrics/compare_samples.html:18: SIGNIFICANCE_LEVEL: ...
4 years, 3 months ago (2016-09-07 16:20:58 UTC) #32
RobertoCN
https://codereview.chromium.org/2089833002/diff/180001/tracing/tracing/metrics/compare_samples.html File tracing/tracing/metrics/compare_samples.html (right): https://codereview.chromium.org/2089833002/diff/180001/tracing/tracing/metrics/compare_samples.html#newcode225 tracing/tracing/metrics/compare_samples.html:225: var current; On 2016/09/07 16:18:26, benjhayden wrote: > delete ...
4 years, 3 months ago (2016-09-07 18:14:32 UTC) #33
nednguyen
On 2016/09/07 18:14:32, RobertoCN wrote: > https://codereview.chromium.org/2089833002/diff/180001/tracing/tracing/metrics/compare_samples.html > File tracing/tracing/metrics/compare_samples.html (right): > > https://codereview.chromium.org/2089833002/diff/180001/tracing/tracing/metrics/compare_samples.html#newcode225 > ...
4 years, 3 months ago (2016-09-12 17:13:07 UTC) #34
nednguyen
https://codereview.chromium.org/2089833002/diff/200001/tracing/tracing/metrics/compare_samples.html File tracing/tracing/metrics/compare_samples.html (right): https://codereview.chromium.org/2089833002/diff/200001/tracing/tracing/metrics/compare_samples.html#newcode16 tracing/tracing/metrics/compare_samples.html:16: tr.exportTo('tr.metrics', function() { You also don't need this export.
4 years, 3 months ago (2016-09-12 17:13:32 UTC) #35
dtu
https://codereview.chromium.org/2089833002/diff/200001/tracing/tracing/metrics/compare_samples.html File tracing/tracing/metrics/compare_samples.html (right): https://codereview.chromium.org/2089833002/diff/200001/tracing/tracing/metrics/compare_samples.html#newcode207 tracing/tracing/metrics/compare_samples.html:207: var BisectComparison = { The word "bisect" isn't used ...
4 years, 3 months ago (2016-09-15 21:31:56 UTC) #36
dtu
https://codereview.chromium.org/2089833002/diff/200001/tracing/tracing/metrics/compare_samples.html File tracing/tracing/metrics/compare_samples.html (right): https://codereview.chromium.org/2089833002/diff/200001/tracing/tracing/metrics/compare_samples.html#newcode252 tracing/tracing/metrics/compare_samples.html:252: mean: tr.b.Statistics.mean(sample), Question for Ben and Ethan: are you ...
4 years, 3 months ago (2016-09-15 21:35:41 UTC) #37
RobertoCN
All feedback addressed. PTAL https://codereview.chromium.org/2089833002/diff/200001/tracing/tracing/metrics/compare_samples.html File tracing/tracing/metrics/compare_samples.html (right): https://codereview.chromium.org/2089833002/diff/200001/tracing/tracing/metrics/compare_samples.html#newcode16 tracing/tracing/metrics/compare_samples.html:16: tr.exportTo('tr.metrics', function() { On 2016/09/12 ...
4 years, 3 months ago (2016-09-19 20:07:43 UTC) #38
nednguyen
https://codereview.chromium.org/2089833002/diff/200001/tracing/tracing/metrics/compare_samples.html File tracing/tracing/metrics/compare_samples.html (right): https://codereview.chromium.org/2089833002/diff/200001/tracing/tracing/metrics/compare_samples.html#newcode16 tracing/tracing/metrics/compare_samples.html:16: tr.exportTo('tr.metrics', function() { On 2016/09/19 20:07:43, RobertoCN wrote: > ...
4 years, 3 months ago (2016-09-19 20:14:41 UTC) #39
benjhayden
https://codereview.chromium.org/2089833002/diff/200001/tracing/tracing/metrics/compare_samples.html File tracing/tracing/metrics/compare_samples.html (right): https://codereview.chromium.org/2089833002/diff/200001/tracing/tracing/metrics/compare_samples.html#newcode252 tracing/tracing/metrics/compare_samples.html:252: mean: tr.b.Statistics.mean(sample), On 2016/09/15 at 21:35:41, dtu wrote: > ...
4 years, 3 months ago (2016-09-21 06:33:25 UTC) #40
RobertoCN
On 2016/09/19 20:14:41, nednguyen wrote: > https://codereview.chromium.org/2089833002/diff/200001/tracing/tracing/metrics/compare_samples.html > File tracing/tracing/metrics/compare_samples.html (right): > > https://codereview.chromium.org/2089833002/diff/200001/tracing/tracing/metrics/compare_samples.html#newcode16 > ...
4 years, 2 months ago (2016-09-22 18:58:58 UTC) #41
nednguyen
https://codereview.chromium.org/2089833002/diff/220001/tracing/tracing/metrics/compare_samples_cmdline.html File tracing/tracing/metrics/compare_samples_cmdline.html (right): https://codereview.chromium.org/2089833002/diff/220001/tracing/tracing/metrics/compare_samples_cmdline.html#newcode249 tracing/tracing/metrics/compare_samples_cmdline.html:249: compareSamples: function(sampleA, sampleB) { This method should probably be ...
4 years, 2 months ago (2016-09-22 20:19:26 UTC) #42
benjhayden
https://codereview.chromium.org/2089833002/diff/220001/tracing/tracing/metrics/compare_samples_cmdline.html File tracing/tracing/metrics/compare_samples_cmdline.html (right): https://codereview.chromium.org/2089833002/diff/220001/tracing/tracing/metrics/compare_samples_cmdline.html#newcode249 tracing/tracing/metrics/compare_samples_cmdline.html:249: compareSamples: function(sampleA, sampleB) { On 2016/09/22 at 20:19:26, nednguyen ...
4 years, 2 months ago (2016-09-22 21:32:18 UTC) #43
RobertoCN
PTAL
4 years, 2 months ago (2016-09-26 22:58:19 UTC) #44
RobertoCN
https://codereview.chromium.org/2089833002/diff/220001/tracing/tracing/metrics/compare_samples_cmdline.html File tracing/tracing/metrics/compare_samples_cmdline.html (right): https://codereview.chromium.org/2089833002/diff/220001/tracing/tracing/metrics/compare_samples_cmdline.html#newcode249 tracing/tracing/metrics/compare_samples_cmdline.html:249: compareSamples: function(sampleA, sampleB) { On 2016/09/22 21:32:18, benjhayden wrote: ...
4 years, 2 months ago (2016-09-27 05:50:16 UTC) #45
benjhayden
a couple of questions and a nit, still lgtm but wait for other folks. I ...
4 years, 2 months ago (2016-09-27 06:25:54 UTC) #46
RobertoCN
Prasad, Ethan and Dave PTAL. I need one more lgtm to land this. https://codereview.chromium.org/2089833002/diff/240001/tracing/bin/compare_samples File ...
4 years, 2 months ago (2016-09-27 22:58:49 UTC) #47
nednguyen
https://codereview.chromium.org/2089833002/diff/260001/tracing/tracing/metrics/compare_samples_cmdline.html File tracing/tracing/metrics/compare_samples_cmdline.html (right): https://codereview.chromium.org/2089833002/diff/260001/tracing/tracing/metrics/compare_samples_cmdline.html#newcode207 tracing/tracing/metrics/compare_samples_cmdline.html:207: ENOUGH_SAMPLES: 18, I think it's important that non-bisect module ...
4 years, 2 months ago (2016-09-27 23:04:03 UTC) #48
eakuefner
lgtm https://codereview.chromium.org/2089833002/diff/260001/tracing/bin/compare_samples File tracing/bin/compare_samples (right): https://codereview.chromium.org/2089833002/diff/260001/tracing/bin/compare_samples#newcode2 tracing/bin/compare_samples:2: # Copyright (c) 2016 The Chromium Authors. All ...
4 years, 2 months ago (2016-09-29 17:01:12 UTC) #49
benjhayden
Sorry I broke test-valueset.json. :-) https://codereview.chromium.org/2364243002 Please rebase again.
4 years, 2 months ago (2016-09-29 17:04:49 UTC) #50
RobertoCN
Addressed Ned's comments. Ben, Ethan, PTAL at the compareSamples function as moved into tr.b.Statistics, I ...
4 years, 2 months ago (2016-09-30 20:01:27 UTC) #51
benjhayden
https://codereview.chromium.org/2089833002/diff/280001/tracing/tracing/base/statistics.html File tracing/tracing/base/statistics.html (right): https://codereview.chromium.org/2089833002/diff/280001/tracing/tracing/base/statistics.html#newcode805 tracing/tracing/base/statistics.html:805: Statistics.ENOUGH_SAMPLES = 18; Can you add a comment about ...
4 years, 2 months ago (2016-09-30 20:41:07 UTC) #52
RobertoCN
https://codereview.chromium.org/2089833002/diff/260001/tracing/bin/compare_samples File tracing/bin/compare_samples (right): https://codereview.chromium.org/2089833002/diff/260001/tracing/bin/compare_samples#newcode2 tracing/bin/compare_samples:2: # Copyright (c) 2016 The Chromium Authors. All rights ...
4 years, 2 months ago (2016-10-01 01:04:23 UTC) #53
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2089833002/300001
4 years, 2 months ago (2016-10-03 05:36:32 UTC) #56
commit-bot: I haz the power
Try jobs failed on following builders: Catapult Windows Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Windows%20Tryserver/builds/4989)
4 years, 2 months ago (2016-10-03 06:22:45 UTC) #58
benjhayden
https://codereview.chromium.org/2089833002/diff/300001/tracing/tracing/base/statistics.html File tracing/tracing/base/statistics.html (right): https://codereview.chromium.org/2089833002/diff/300001/tracing/tracing/base/statistics.html#newcode805 tracing/tracing/base/statistics.html:805: Statistics.MAX_SUGGESTED_SAMPLE_SIZE = 20; Is this a maximum or a ...
4 years, 2 months ago (2016-10-03 06:35:24 UTC) #59
nednguyen
https://codereview.chromium.org/2089833002/diff/300001/tracing/tracing/metrics/compare_samples_unittest.py File tracing/tracing/metrics/compare_samples_unittest.py (right): https://codereview.chromium.org/2089833002/diff/300001/tracing/tracing/metrics/compare_samples_unittest.py#newcode43 tracing/tracing/metrics/compare_samples_unittest.py:43: _, new_json_file = tempfile.mkstemp( I think the exception on ...
4 years, 2 months ago (2016-10-03 12:58:19 UTC) #60
RobertoCN
On 2016/10/03 06:35:24, benjhayden wrote: > https://codereview.chromium.org/2089833002/diff/300001/tracing/tracing/base/statistics.html > File tracing/tracing/base/statistics.html (right): > > https://codereview.chromium.org/2089833002/diff/300001/tracing/tracing/base/statistics.html#newcode805 > ...
4 years, 2 months ago (2016-10-03 15:39:47 UTC) #61
benjhayden
On 2016/10/03 at 15:39:47, robertocn wrote: > On 2016/10/03 06:35:24, benjhayden wrote: > > https://codereview.chromium.org/2089833002/diff/300001/tracing/tracing/base/statistics.html ...
4 years, 2 months ago (2016-10-03 16:28:56 UTC) #62
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2089833002/320001
4 years, 2 months ago (2016-10-03 17:15:05 UTC) #65
commit-bot: I haz the power
4 years, 2 months ago (2016-10-03 17:41:37 UTC) #67
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...

Powered by Google App Engine
This is Rietveld 408576698