|
|
DescriptionUpdate run_gtest_perf_test.py to streaming command output to stdout & log file at the same time
BUG=723821
TBR=dpranke@chromium.org
Review-Url: https://codereview.chromium.org/2889153007
Cr-Commit-Position: refs/heads/master@{#473376}
Committed: https://chromium.googlesource.com/chromium/src/+/c3648c4f352731076b62b779597e40fed8db78e6
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address Daniel's comments #
Total comments: 8
Patch Set 3 : Address Daniel's comments #
Messages
Total messages: 21 (12 generated)
Patchset #1 (id:1) has been deleted
nednguyen@google.com changed reviewers: + dpranke@chromium.org
Description was changed from ========== Update run_gtest_perf_test.py to streaming command output to stdout & log file at the same time BUG=723821 ========== to ========== Update run_gtest_perf_test.py to streaming command output to stdout & log file at the same time BUG=723821 TBR=dpranke@chromium.org ==========
The CQ bit was checked by nednguyen@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by nednguyen@google.com
nednguyen@google.com changed reviewers: + dnj@chromium.org
I defer to dnj@ here. lgtm if/when he approves.
https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common.py File testing/scripts/common.py (right): https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common.... testing/scripts/common.py:78: rc = 1 No point setting this here. https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common.... testing/scripts/common.py:81: stdoutfile_header = open(stdoutfile, 'w') nit: do this in try/finally clause, closing on finally. Alternatively, you can use "with" and a special contextmanager: @contextlib.contextmanager def maybe_open(path, mode): if not path: yield None return with open(path, mode) as fd: yield fd https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common.... testing/scripts/common.py:82: process = subprocess.Popen(argv, env=env, cwd=cwd, stdout=subprocess.PIPE, Note that before, if the argv[0] did not exist or wasn't executable, you would catch that in the "except Exception" block; now, it will raise an Exception. Is this OK? https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common.... testing/scripts/common.py:84: for c in iter(lambda: process.stdout.read(1), ''): How much data are we expecting here? Reading byte-by-byte and blocking the process on this may be very expensive if it's a medium or large volume. Consider reading line-by-line instead, at least? If the output here is anything other than trivial, you should also add an optimized path where you only attach pipes and manually manage them if "stdoutfile_header" is not None. https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common.... testing/scripts/common.py:87: stdoutfile_header.write(c) You need to close this file descriptor when you're done. I recommend the "with" / contextmanager approach that I outlined above to avoid this sort of error. https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common.... testing/scripts/common.py:89: rc = process.returncode Just use "process.returncode" in the 'print' and 'return' clauses, no need for this indirect variable.
https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common.py File testing/scripts/common.py (right): https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common.... testing/scripts/common.py:78: rc = 1 On 2017/05/19 20:56:18, dnj wrote: > No point setting this here. Done. https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common.... testing/scripts/common.py:81: stdoutfile_header = open(stdoutfile, 'w') On 2017/05/19 20:56:18, dnj wrote: > nit: do this in try/finally clause, closing on finally. > > Alternatively, you can use "with" and a special contextmanager: > > @contextlib.contextmanager > def maybe_open(path, mode): > if not path: > yield None > return > > with open(path, mode) as fd: > yield fd We no longer need this as I just assert stdoutfile is not None https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common.... testing/scripts/common.py:82: process = subprocess.Popen(argv, env=env, cwd=cwd, stdout=subprocess.PIPE, On 2017/05/19 20:56:18, dnj wrote: > Note that before, if the argv[0] did not exist or wasn't executable, you would > catch that in the "except Exception" block; now, it will raise an Exception. Is > this OK? That's ok as long as we have error message can point out what happened. https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common.... testing/scripts/common.py:84: for c in iter(lambda: process.stdout.read(1), ''): On 2017/05/19 20:56:18, dnj wrote: > How much data are we expecting here? Reading byte-by-byte and blocking the > process on this may be very expensive if it's a medium or large volume. Consider > reading line-by-line instead, at least? > > If the output here is anything other than trivial, you should also add an > optimized path where you only attach pipes and manually manage them if > "stdoutfile_header" is not None. This can depend. I think it's safer to not assuming the volume is small. I changed the code to use a polling loop for write to stdout instead. https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common.... testing/scripts/common.py:87: stdoutfile_header.write(c) On 2017/05/19 20:56:18, dnj wrote: > You need to close this file descriptor when you're done. I recommend the "with" > / contextmanager approach that I outlined above to avoid this sort of error. Done. https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common.... testing/scripts/common.py:89: rc = process.returncode On 2017/05/19 20:56:18, dnj wrote: > Just use "process.returncode" in the 'print' and 'return' clauses, no need for > this indirect variable. Done.
The CQ bit was checked by nednguyen@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm w/ comments/nits https://codereview.chromium.org/2889153007/diff/40001/testing/scripts/common.py File testing/scripts/common.py (right): https://codereview.chromium.org/2889153007/diff/40001/testing/scripts/common.... testing/scripts/common.py:76: def run_command_with_output(argv, stdoutfile, env=None, cwd=None, ): Nit, trailing comma. https://codereview.chromium.org/2889153007/diff/40001/testing/scripts/common.... testing/scripts/common.py:81: with io.open(stdoutfile, 'wb') as writer: Nit, you can combine this into a single block: with io.open(...) as writer, io.open(...) as reader: https://codereview.chromium.org/2889153007/diff/40001/testing/scripts/common.... testing/scripts/common.py:82: with io.open(stdoutfile, 'rb', 1) as reader: Note: the original used "w" (text-mode); "wb" and "rb" are *probably* fine, but double-check that nothing relied on Python's text conversion. https://codereview.chromium.org/2889153007/diff/40001/testing/scripts/common.... testing/scripts/common.py:87: time.sleep(1) Note: if the subprocess is fast and repeated (e.g., 100x 10ms surprocesses), this will add real delay. Consider something more aggressive, like sleep(0.1).
https://codereview.chromium.org/2889153007/diff/40001/testing/scripts/common.py File testing/scripts/common.py (right): https://codereview.chromium.org/2889153007/diff/40001/testing/scripts/common.... testing/scripts/common.py:76: def run_command_with_output(argv, stdoutfile, env=None, cwd=None, ): On 2017/05/19 21:27:16, dnj wrote: > Nit, trailing comma. Done. https://codereview.chromium.org/2889153007/diff/40001/testing/scripts/common.... testing/scripts/common.py:81: with io.open(stdoutfile, 'wb') as writer: On 2017/05/19 21:27:16, dnj wrote: > Nit, you can combine this into a single block: > > with io.open(...) as writer, io.open(...) as reader: Done. https://codereview.chromium.org/2889153007/diff/40001/testing/scripts/common.... testing/scripts/common.py:82: with io.open(stdoutfile, 'rb', 1) as reader: On 2017/05/19 21:27:16, dnj wrote: > Note: the original used "w" (text-mode); "wb" and "rb" are *probably* fine, but > double-check that nothing relied on Python's text conversion. Oh, I just use "w" & "r" here to be sure. https://codereview.chromium.org/2889153007/diff/40001/testing/scripts/common.... testing/scripts/common.py:87: time.sleep(1) On 2017/05/19 21:27:16, dnj wrote: > Note: if the subprocess is fast and repeated (e.g., 100x 10ms surprocesses), > this will add real delay. Consider something more aggressive, like sleep(0.1). Done.
The CQ bit was checked by nednguyen@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, dnj@chromium.org Link to the patchset: https://codereview.chromium.org/2889153007/#ps60001 (title: "Address Daniel's comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495229505627240, "parent_rev": "21501560f5074b5e07c397fedc09d47a0a6c5b3f", "commit_rev": "c3648c4f352731076b62b779597e40fed8db78e6"}
Message was sent while issue was closed.
Description was changed from ========== Update run_gtest_perf_test.py to streaming command output to stdout & log file at the same time BUG=723821 TBR=dpranke@chromium.org ========== to ========== Update run_gtest_perf_test.py to streaming command output to stdout & log file at the same time BUG=723821 TBR=dpranke@chromium.org Review-Url: https://codereview.chromium.org/2889153007 Cr-Commit-Position: refs/heads/master@{#473376} Committed: https://chromium.googlesource.com/chromium/src/+/c3648c4f352731076b62b779597e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/c3648c4f352731076b62b779597e... |