Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1681283004:
[tools] add --pretty switch to run_perf.py
|
Created:
4 years, 10 months ago by Camillo Bruni Modified:
4 years, 6 months ago Reviewers:
Michael Achenbach CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Descriptionreland [tools] add --pretty switch to run_perf.py
This CL improves running our internal benchmarks locally by adding the
--pretty option to tools/run_perf.py. With the flag enabled we print
the run-time of each benchmark directly and avoid the json output at
the end.
NOTRY=true
Committed: https://crrev.com/b543c40d78c3aa8283a7d9fccb6c9e26a9a81f90
Cr-Commit-Position: refs/heads/master@{#34023}
Patch Set 1 #Patch Set 2 : fixing indent #Patch Set 3 : better error handling #
Total comments: 6
Patch Set 4 : cleanup and addressing comments #
Total comments: 6
Patch Set 5 : more clean, less nit #
Total comments: 2
Patch Set 6 : using average and stdev only when printing #
Total comments: 1
Patch Set 7 : adding mock method #
Total comments: 1
Patch Set 8 : fix output concatenation on android #
Total comments: 1
Messages
Total messages: 35 (12 generated)
cbruni@chromium.org changed reviewers: + machenbach@chromium.org
PTAL. I hacked the run_perf.py script to be more useful when running locally.
Please keep the unittests passing: cd tools/unittests python -m unittest run_perf_test https://codereview.chromium.org/1681283004/diff/40001/tools/run_perf.py File tools/run_perf.py (right): https://codereview.chromium.org/1681283004/diff/40001/tools/run_perf.py#newco... tools/run_perf.py:221: return sum(float(x) for x in self.results) / len(self.results) nit: sum(self.results) / float(len(self.results)) might get the same. https://codereview.chromium.org/1681283004/diff/40001/tools/run_perf.py#newco... tools/run_perf.py:229: "stddev": self.GetDeviation(), There are benchmarks that provide their own stddev value (self.stddev). Those tests have internal loops and only provide one value as result (which already is a mean) and one stddev. It shouldn't get overwritten if it exists. https://codereview.chromium.org/1681283004/diff/40001/tools/run_perf.py#newco... tools/run_perf.py:962: if options.pretty: Maybe factor out this piece of code as printing is a separate concern. I assume you do printing here and not below to see partial results earlier while the runner is active?
PTAL again. addressed nits and comemnts https://codereview.chromium.org/1681283004/diff/40001/tools/run_perf.py File tools/run_perf.py (right): https://codereview.chromium.org/1681283004/diff/40001/tools/run_perf.py#newco... tools/run_perf.py:221: return sum(float(x) for x in self.results) / len(self.results) On 2016/02/11 at 09:44:07, Michael Achenbach wrote: > nit: sum(self.results) / float(len(self.results)) might get the same. I think I got some strings in the results https://codereview.chromium.org/1681283004/diff/40001/tools/run_perf.py#newco... tools/run_perf.py:229: "stddev": self.GetDeviation(), On 2016/02/11 at 09:44:07, Michael Achenbach wrote: > There are benchmarks that provide their own stddev value (self.stddev). Those tests have internal loops and only provide one value as result (which already is a mean) and one stddev. It shouldn't get overwritten if it exists. ups... I wanted to return the existing value in GetDeviation() if it exists https://codereview.chromium.org/1681283004/diff/40001/tools/run_perf.py#newco... tools/run_perf.py:962: if options.pretty: On 2016/02/11 at 09:44:07, Michael Achenbach wrote: > Maybe factor out this piece of code as printing is a separate concern. I assume you do printing here and not below to see partial results earlier while the runner is active? Yes, I want to have direct feedback. I'll put it in a separate method that should make it a bit more readable.
https://codereview.chromium.org/1681283004/diff/60001/tools/run_perf.py File tools/run_perf.py (right): https://codereview.chromium.org/1681283004/diff/60001/tools/run_perf.py#newco... tools/run_perf.py:223: if self.stddev: This might deserve a small comment (sorry for not providing one myself somewhere). https://codereview.chromium.org/1681283004/diff/60001/tools/run_perf.py#newco... tools/run_perf.py:225: return sum(x for x in self.results) / len(self.results) nit: Now that you removed the float() conversion, sum(self.results) should really work :) https://codereview.chromium.org/1681283004/diff/60001/tools/run_perf.py#newco... tools/run_perf.py:652: def PrintResult(self): This needs a second argument, just like the function below. Otherwise it errors out when pretty isn't used. https://codereview.chromium.org/1681283004/diff/60001/tools/run_perf.py#newco... tools/run_perf.py:690: Could you add a todo to make this available to the android platform? I don't see why this wouldn't work already now, except that we should probably chose some other abstraction but inheritance.
cleaned up some more code and addressed those nits :) https://codereview.chromium.org/1681283004/diff/60001/tools/run_perf.py File tools/run_perf.py (right): https://codereview.chromium.org/1681283004/diff/60001/tools/run_perf.py#newco... tools/run_perf.py:225: return sum(x for x in self.results) / len(self.results) On 2016/02/11 at 13:01:11, Michael Achenbach wrote: > nit: Now that you removed the float() conversion, sum(self.results) should really work :) getting there... incrementally :D https://codereview.chromium.org/1681283004/diff/60001/tools/run_perf.py#newco... tools/run_perf.py:690: On 2016/02/11 at 13:01:10, Michael Achenbach wrote: > Could you add a todo to make this available to the android platform? I don't see why this wouldn't work already now, except that we should probably chose some other abstraction but inheritance. done https://codereview.chromium.org/1681283004/diff/80001/tools/run_perf.py File tools/run_perf.py (right): https://codereview.chromium.org/1681283004/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:614: Moved all basic "public" methods up here.
Looks good. But the tests still fail. Now there is the average field everywhere in the results. https://codereview.chromium.org/1681283004/diff/80001/tools/run_perf.py File tools/run_perf.py (right): https://codereview.chromium.org/1681283004/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:866: print title % "Stdout" You could add the self._PrintStdout(title, output) method here too and pull it up to Platform.
PTAL again ;) moved now all stddev / average calculation to the Mixin, that should keep the rest clean
https://codereview.chromium.org/1681283004/diff/100001/tools/run_perf.py File tools/run_perf.py (right): https://codereview.chromium.org/1681283004/diff/100001/tools/run_perf.py#newc... tools/run_perf.py:1013: platform.PrintResult(result) A tiny thing left to let the tests pass. Please add: platform.PrintResult = MagicMock(return_value=None) around line 412 in unittests/run_perf_test.py.
Description was changed from ========== [tools] add --pretty switch to run_perf.py This CL improves running our internal benchmarks locally by adding the --pretty option to tools/run_perf.py. With the flag enabled we print the run-time of each benchmark directly and avoid the json output at the end. BUG= ========== to ========== [tools] add --pretty switch to run_perf.py This CL improves running our internal benchmarks locally by adding the --pretty option to tools/run_perf.py. With the flag enabled we print the run-time of each benchmark directly and avoid the json output at the end. NOTRY=true ==========
PTAL, added MagicMock
lgtm
The CQ bit was checked by cbruni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1681283004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1681283004/120001
Message was sent while issue was closed.
Description was changed from ========== [tools] add --pretty switch to run_perf.py This CL improves running our internal benchmarks locally by adding the --pretty option to tools/run_perf.py. With the flag enabled we print the run-time of each benchmark directly and avoid the json output at the end. NOTRY=true ========== to ========== [tools] add --pretty switch to run_perf.py This CL improves running our internal benchmarks locally by adding the --pretty option to tools/run_perf.py. With the flag enabled we print the run-time of each benchmark directly and avoid the json output at the end. NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [tools] add --pretty switch to run_perf.py This CL improves running our internal benchmarks locally by adding the --pretty option to tools/run_perf.py. With the flag enabled we print the run-time of each benchmark directly and avoid the json output at the end. NOTRY=true ========== to ========== [tools] add --pretty switch to run_perf.py This CL improves running our internal benchmarks locally by adding the --pretty option to tools/run_perf.py. With the flag enabled we print the run-time of each benchmark directly and avoid the json output at the end. NOTRY=true Committed: https://crrev.com/83f69507ab1b9380b56758b747d4f3fabc849e49 Cr-Commit-Position: refs/heads/master@{#33981} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/83f69507ab1b9380b56758b747d4f3fabc849e49 Cr-Commit-Position: refs/heads/master@{#33981}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/1696293002/ by machenbach@chromium.org. The reason for reverting is: [Sheriff] Breaks android runs. Will look later why..
Message was sent while issue was closed.
Please reland with the fix below. https://codereview.chromium.org/1681283004/diff/120001/tools/run_perf.py File tools/run_perf.py (right): https://codereview.chromium.org/1681283004/diff/120001/tools/run_perf.py#newc... tools/run_perf.py:847: print "\n".join(output.stdout) hmm, python. Here, output seems to be a list of strings. So like on the left side: print "\n".join(output)
The CQ bit was checked by cbruni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/1681283004/#ps140001 (title: "fix output concatenation on android")
The CQ bit was unchecked by cbruni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1681283004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1681283004/140001
Description was changed from ========== [tools] add --pretty switch to run_perf.py This CL improves running our internal benchmarks locally by adding the --pretty option to tools/run_perf.py. With the flag enabled we print the run-time of each benchmark directly and avoid the json output at the end. NOTRY=true Committed: https://crrev.com/83f69507ab1b9380b56758b747d4f3fabc849e49 Cr-Commit-Position: refs/heads/master@{#33981} ========== to ========== reland [tools] add --pretty switch to run_perf.py This CL improves running our internal benchmarks locally by adding the --pretty option to tools/run_perf.py. With the flag enabled we print the run-time of each benchmark directly and avoid the json output at the end. NOTRY=true ==========
lgtm
The CQ bit was checked by machenbach@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1681283004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1681283004/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== reland [tools] add --pretty switch to run_perf.py This CL improves running our internal benchmarks locally by adding the --pretty option to tools/run_perf.py. With the flag enabled we print the run-time of each benchmark directly and avoid the json output at the end. NOTRY=true ========== to ========== reland [tools] add --pretty switch to run_perf.py This CL improves running our internal benchmarks locally by adding the --pretty option to tools/run_perf.py. With the flag enabled we print the run-time of each benchmark directly and avoid the json output at the end. NOTRY=true Committed: https://crrev.com/b543c40d78c3aa8283a7d9fccb6c9e26a9a81f90 Cr-Commit-Position: refs/heads/master@{#34023} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/b543c40d78c3aa8283a7d9fccb6c9e26a9a81f90 Cr-Commit-Position: refs/heads/master@{#34023}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/1700953002/ by machenbach@chromium.org. The reason for reverting is: [Sheriff] Still fails on android..
Message was sent while issue was closed.
https://codereview.chromium.org/1681283004/diff/140001/tools/run_perf.py File tools/run_perf.py (right): https://codereview.chromium.org/1681283004/diff/140001/tools/run_perf.py#newc... tools/run_perf.py:876: return stdout Meh. This needs to contain the stdout from above. Maybe the print method should also return the stdout?
Message was sent while issue was closed.
Ping. I think the last state of this was reverted? Or did it reland?
Message was sent while issue was closed.
Description was changed from ========== reland [tools] add --pretty switch to run_perf.py This CL improves running our internal benchmarks locally by adding the --pretty option to tools/run_perf.py. With the flag enabled we print the run-time of each benchmark directly and avoid the json output at the end. NOTRY=true Committed: https://crrev.com/b543c40d78c3aa8283a7d9fccb6c9e26a9a81f90 Cr-Commit-Position: refs/heads/master@{#34023} ========== to ========== reland [tools] add --pretty switch to run_perf.py This CL improves running our internal benchmarks locally by adding the --pretty option to tools/run_perf.py. With the flag enabled we print the run-time of each benchmark directly and avoid the json output at the end. NOTRY=true Committed: https://crrev.com/b543c40d78c3aa8283a7d9fccb6c9e26a9a81f90 Cr-Commit-Position: refs/heads/master@{#34023} ========== |
