Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
732473003:
A more robust way to set/query CPU properties on devices (Closed)
DescriptionA more robust way to set/query CPU properties on devices
Provides a method _ForEachCpu which runs a specified operation on
each CPU file on the device. It is efficient, as all of the commands
are executed using a single shell script on the device (as it was
already done for some of the methods); and now also more robust, as we
get the exit status of each individual command run.
BUG=433056
Committed: https://crrev.com/18689623bac72514040e422003a89f230549252a
Cr-Commit-Position: refs/heads/master@{#304927}
Patch Set 1 #
Total comments: 16
Patch Set 2 : second revision #
Total comments: 6
Patch Set 3 : fixed coding style issues #
Total comments: 7
Patch Set 4 : fixed nits #Patch Set 5 : fixed "mock" interface used by telemetry for testing #
Messages
Total messages: 19 (4 generated)
perezju@chromium.org changed reviewers: + jbudorick@chromium.org, skyostil@chromium.org, vmiura@chromium.org
ptal https://codereview.chromium.org/732473003/diff/1/build/android/pylib/perf/per... File build/android/pylib/perf/perf_control.py (right): https://codereview.chromium.org/732473003/diff/1/build/android/pylib/perf/per... build/android/pylib/perf/perf_control.py:25: check_return=True, as_root=True) This will already throw an exception if either the path does not exist, or no cpu files are found on the path. https://codereview.chromium.org/732473003/diff/1/build/android/pylib/perf/per... build/android/pylib/perf/perf_control.py:103: return zip(self._cpu_files, output[0::2], (int(c) for c in output[1::2])) This is where the magic happens now, for each cpu we get a triple: (cpu, output, exit_code), with the result of running each command on the device. https://codereview.chromium.org/732473003/diff/1/build/android/pylib/perf/per... build/android/pylib/perf/perf_control.py:129: return all(out.rstrip() == '1' and code == 0 code will be non-zero if the file does not exist, and the output is not '1' (usually '0'), if the cpu is offline.
https://codereview.chromium.org/732473003/diff/1/build/android/pylib/device/d... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/732473003/diff/1/build/android/pylib/device/d... build/android/pylib/device/device_utils.py:374: as_root=False, single_line=False, raw_output=False, I'm wary of adding options simply for convenience rather than functionality. (|single_line| fell into this category, too.) Each option we add has a complexity cost for everyone who uses the function. Why are we adding this? https://codereview.chromium.org/732473003/diff/1/build/android/pylib/perf/per... File build/android/pylib/perf/perf_control.py (right): https://codereview.chromium.org/732473003/diff/1/build/android/pylib/perf/per... build/android/pylib/perf/perf_control.py:98: ' %s; echo -n "%%~%%$?%%~%%"\n' What's the reasoning behind "%~%$?%~%"? Do we just think it's sufficiently unlikely to be output by anything else? https://codereview.chromium.org/732473003/diff/1/build/android/pylib/perf/per... build/android/pylib/perf/perf_control.py:99: 'done' % (self._CPU_PATH, self._cpu_file_list, cmd)) I think this would be more readable if you handled each line individually and then joined them together once you've built them, e.g. script_commands = [ 'cd %s' % self._CPU_PATH, 'for CPU in %s' % self._cpu_file_list, 'do %s' % cmd, 'echo -n "%%~%%?%%~%%"', 'done' ] script = ';'.join(script_commands) nit: I'd also rather you use semicolons than newlines while handrolling shell scripts. https://codereview.chromium.org/732473003/diff/1/build/android/pylib/perf/per... build/android/pylib/perf/perf_control.py:101: as_root=True, raw_output=True) I don't think this justifies adding raw_output to RunShellCommand. https://codereview.chromium.org/732473003/diff/1/build/android/pylib/perf/per... build/android/pylib/perf/perf_control.py:107: 'path': cmd_helper.DoubleQuote(posixpath.join('$CPU', path)), posixpath.join?
https://codereview.chromium.org/732473003/diff/1/build/android/pylib/perf/per... File build/android/pylib/perf/perf_control.py (right): https://codereview.chromium.org/732473003/diff/1/build/android/pylib/perf/per... build/android/pylib/perf/perf_control.py:24: 'cd %s && ls -d cpu[0-9]*' % self._CPU_PATH, nit: You could replace "ls -d" with "echo" here. https://codereview.chromium.org/732473003/diff/1/build/android/pylib/perf/per... build/android/pylib/perf/perf_control.py:88: online = (out.rstrip() == '1' and code == 0 nit: I think something like |exit_status| may be a better name for |code|. https://codereview.chromium.org/732473003/diff/1/build/android/pylib/perf/per... build/android/pylib/perf/perf_control.py:98: ' %s; echo -n "%%~%%$?%%~%%"\n' On 2014/11/18 02:29:38, jbudorick wrote: > What's the reasoning behind "%~%$?%~%"? > > Do we just think it's sufficiently unlikely to be output by anything else? Alternatively we could append the exit codes to a variable and print that at the end for parsing.
Rewrote taking into account all suggestions. Also replaced some "cd"s with the |cwd| option of RunShellCommand (that's why it's there!). https://codereview.chromium.org/732473003/diff/1/build/android/pylib/perf/per... File build/android/pylib/perf/perf_control.py (right): https://codereview.chromium.org/732473003/diff/1/build/android/pylib/perf/per... build/android/pylib/perf/perf_control.py:24: 'cd %s && ls -d cpu[0-9]*' % self._CPU_PATH, On 2014/11/18 13:24:04, Sami wrote: > nit: You could replace "ls -d" with "echo" here. "ls -d" is more robust. If for whatever reason the CPU files are not found, ls will return a non-zero exit code and cause an exception to be raised. Echo, on the other hand, will make us think that a single cpu file named "cpu[0-9]*" was found. https://codereview.chromium.org/732473003/diff/1/build/android/pylib/perf/per... build/android/pylib/perf/perf_control.py:88: online = (out.rstrip() == '1' and code == 0 On 2014/11/18 13:24:04, Sami wrote: > nit: I think something like |exit_status| may be a better name for |code|. Acknowledged. I went for |status| just to keep it short in these list comprehensions. https://codereview.chromium.org/732473003/diff/1/build/android/pylib/perf/per... build/android/pylib/perf/perf_control.py:98: ' %s; echo -n "%%~%%$?%%~%%"\n' On 2014/11/18 13:24:04, Sami wrote: > On 2014/11/18 02:29:38, jbudorick wrote: > > What's the reasoning behind "%~%$?%~%"? > > > > Do we just think it's sufficiently unlikely to be output by anything else? > > Alternatively we could append the exit codes to a variable and print that at the > end for parsing. The reasoning is basically just that. Also I think it's best to keep the markers here, since they also serve as a separator between the outputs of different iterations of the loop. https://codereview.chromium.org/732473003/diff/1/build/android/pylib/perf/per... build/android/pylib/perf/perf_control.py:99: 'done' % (self._CPU_PATH, self._cpu_file_list, cmd)) On 2014/11/18 02:29:38, jbudorick wrote: > I think this would be more readable if you handled each line individually and > then joined them together once you've built them, e.g. > > script_commands = [ > 'cd %s' % self._CPU_PATH, > 'for CPU in %s' % self._cpu_file_list, > 'do %s' % cmd, > 'echo -n "%%~%%?%%~%%"', > 'done' > ] > script = ';'.join(script_commands) > > nit: I'd also rather you use semicolons than newlines while handrolling shell > scripts. Acknowledged. https://codereview.chromium.org/732473003/diff/1/build/android/pylib/perf/per... build/android/pylib/perf/perf_control.py:107: 'path': cmd_helper.DoubleQuote(posixpath.join('$CPU', path)), On 2014/11/18 02:29:37, jbudorick wrote: > posixpath.join? ok, ok, _maybe_ I don't have to be this much defensive on a private method that is only called with sensible values from *other* private methods within the same object. https://codereview.chromium.org/732473003/diff/20001/build/android/pylib/perf... File build/android/pylib/perf/perf_control.py (right): https://codereview.chromium.org/732473003/diff/20001/build/android/pylib/perf... build/android/pylib/perf/perf_control.py:101: output = '\n'.join(output).split('%~%') I do get the argument of not littering RunShellCommand with more options, but I do feel it's a bit "wrong" that there is no way at all to get back the raw output from the command (this join is not guaranteed to produce the exact same original output, since splitlines is effectively not reversible). Anyway, here it doesn't matter much, and perhaps in most other cases it also wont matter at all. I guess I'll just have to live with that small dissonance in my head. :P
https://codereview.chromium.org/732473003/diff/20001/build/android/pylib/perf... File build/android/pylib/perf/perf_control.py (right): https://codereview.chromium.org/732473003/diff/20001/build/android/pylib/perf... build/android/pylib/perf/perf_control.py:21: self._cpu_files = self._device.RunShellCommand('ls -d cpu[0-9]*', Same as below re param formatting. https://codereview.chromium.org/732473003/diff/20001/build/android/pylib/perf... build/android/pylib/perf/perf_control.py:88: for (_, out, status) in nit: move "in" down a line. https://codereview.chromium.org/732473003/diff/20001/build/android/pylib/perf... build/android/pylib/perf/perf_control.py:99: output = self._device.RunShellCommand(script, cwd=self._CPU_PATH, Either: 1) put parameters on the same line as the function call and indent subsequent lines to the depth of the first parameter, or 2) put all parameters on subsequent lines indented four spaces. Don't mix the two. https://codereview.chromium.org/732473003/diff/20001/build/android/pylib/perf... build/android/pylib/perf/perf_control.py:101: output = '\n'.join(output).split('%~%') On 2014/11/18 17:42:42, perezju wrote: > I do get the argument of not littering RunShellCommand with more options, but I > do feel it's a bit "wrong" that there is no way at all to get back the raw > output from the command (this join is not guaranteed to produce the exact same > original output, since splitlines is effectively not reversible). Anyway, here > it doesn't matter much, and perhaps in most other cases it also wont matter at > all. I guess I'll just have to live with that small dissonance in my head. :P I'm having a hard time coming up with a case where a caller would care about which lines end with CRLF vs which lines end with LF. If that happens, we can reconsider this option. https://codereview.chromium.org/732473003/diff/20001/build/android/pylib/perf... build/android/pylib/perf/perf_control.py:105: output = self._ForEachCpu('test -e %(file)s && echo %(value)s > %(file)s' Same as above re param formatting.
fixed coding style issues
lgtm w/ nits Wait for Sami before committing this. https://codereview.chromium.org/732473003/diff/40001/build/android/pylib/perf... File build/android/pylib/perf/perf_control.py (right): https://codereview.chromium.org/732473003/diff/40001/build/android/pylib/perf... build/android/pylib/perf/perf_control.py:94: 'for CPU in %s' % self._cpu_file_list, nit: 4 space indent. https://codereview.chromium.org/732473003/diff/40001/build/android/pylib/perf... build/android/pylib/perf/perf_control.py:105: output = self._ForEachCpu('test -e %(file)s && echo %(value)s > %(file)s' nit: Use a new-style format string here and str.format() here.
Thanks for the cleanup. lgtm with some minor comments. https://codereview.chromium.org/732473003/diff/40001/build/android/pylib/perf... File build/android/pylib/perf/perf_control.py (right): https://codereview.chromium.org/732473003/diff/40001/build/android/pylib/perf... build/android/pylib/perf/perf_control.py:24: logging.info('CPUs found: %s', self._cpu_file_list) Please keep the assert here. https://codereview.chromium.org/732473003/diff/40001/build/android/pylib/perf... build/android/pylib/perf/perf_control.py:109: logging.info('Succesfully set %s to %r on: %s', path, value, cpus) s/Succesfully/Successfully/ https://codereview.chromium.org/732473003/diff/40001/build/android/pylib/perf... build/android/pylib/perf/perf_control.py:130: if cpu != 'cpu0') Note to self: this will work on single core devices because all([]) == True.
Thanks both, I'm preparing a new revision. Just answering to some of your comments now. https://codereview.chromium.org/732473003/diff/40001/build/android/pylib/perf... File build/android/pylib/perf/perf_control.py (right): https://codereview.chromium.org/732473003/diff/40001/build/android/pylib/perf... build/android/pylib/perf/perf_control.py:24: logging.info('CPUs found: %s', self._cpu_file_list) On 2014/11/19 19:18:01, Sami wrote: > Please keep the assert here. As I mentioned in a previous comment, 'ls -d' will fail and raise an exception if no matching files are found. Maybe it's ok just to add a comment to that effect? should I add the assert anyway? (or maybe do both?) https://codereview.chromium.org/732473003/diff/40001/build/android/pylib/perf... build/android/pylib/perf/perf_control.py:105: output = self._ForEachCpu('test -e %(file)s && echo %(value)s > %(file)s' On 2014/11/19 19:04:41, jbudorick wrote: > nit: Use a new-style format string here and str.format() here. Ok. Will do that. What is the stance between new/old formatting styles? On some of my first CL's I was using the new sytle, but then reverted to the old one when I saw that most of the code was using that.
On 2014/11/19 19:26:04, perezju wrote: > Thanks both, I'm preparing a new revision. Just answering to some of your > comments now. > > https://codereview.chromium.org/732473003/diff/40001/build/android/pylib/perf... > File build/android/pylib/perf/perf_control.py (right): > > https://codereview.chromium.org/732473003/diff/40001/build/android/pylib/perf... > build/android/pylib/perf/perf_control.py:24: logging.info('CPUs found: %s', > self._cpu_file_list) > On 2014/11/19 19:18:01, Sami wrote: > > Please keep the assert here. > > As I mentioned in a previous comment, 'ls -d' will fail and raise an exception > if no matching files are found. Maybe it's ok just to add a comment to that > effect? should I add the assert anyway? (or maybe do both?) > > https://codereview.chromium.org/732473003/diff/40001/build/android/pylib/perf... > build/android/pylib/perf/perf_control.py:105: output = self._ForEachCpu('test -e > %(file)s && echo %(value)s > %(file)s' > On 2014/11/19 19:04:41, jbudorick wrote: > > nit: Use a new-style format string here and str.format() here. > > Ok. Will do that. What is the stance between new/old formatting styles? On some > of my first CL's I was using the new sytle, but then reverted to the old one > when I saw that most of the code was using that. The preference is currently old-style solely for consistency. However, in cases where we're naming the conversion specifier, I definitely prefer the new style. I would be open to converting to new style more broadly.
The CQ bit was checked by perezju@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/732473003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by perezju@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/732473003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/18689623bac72514040e422003a89f230549252a Cr-Commit-Position: refs/heads/master@{#304927} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
