Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1077173002:
[Android] Tune DeviceUtils commands that are prone to large outputs. (Closed)
|
Created:
5 years, 8 months ago by jbudorick Modified:
5 years, 8 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Tune DeviceUtils commands that are prone to large outputs.
BUG=472144
Committed: https://crrev.com/6e20669903fad7caa6d6b326ba9e6992f0761680
Cr-Commit-Position: refs/heads/master@{#325241}
Patch Set 1 #
Total comments: 7
Patch Set 2 : PIPESTATUS #
Total comments: 15
Patch Set 3 : perezju comments #
Total comments: 5
Patch Set 4 : #
Total comments: 2
Patch Set 5 : rebase and fix SingleQuote #
Messages
Total messages: 22 (4 generated)
jbudorick@chromium.org changed reviewers: + perezju@chromium.org, rnephew@chromium.org
On 2015/04/10 15:58:29, jbudorick wrote: Not that it matters, LGTM
not lgtm. These are the sorts of things we *could* fix at DeviceUtils without reading large outputs, so I think we should. https://codereview.chromium.org/1077173002/diff/1/build/android/pylib/device/... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1077173002/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:1024: large_output=(not bool(size)))) I don't think this makes much sense, and introduces a cyclic dependency that makes me worry. We are using "cat" as an optimization to avoid having to pull the file from the device. But with large_output=True we'll instead cat the file in the device to _another_ file which is then read using ... ReadFile! I would prefer: - If we can't determine the size of the file, play it safe and just do a pull. - If the file of the size is 0, don't event try to read anything from the device, just return '' (or '\n'?). - Only use cat if we know that the size is small enough so that large_output would not be needed (otherwise the "optimization" is pointless). https://codereview.chromium.org/1077173002/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:1359: large_output=True): This is precisely one of the things I really think we shouldn't do. I think that, instead of large_output=True, we should change the command to 'ps | grep -F {{process_name}}' instead. https://codereview.chromium.org/1077173002/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:1433: large_output=True) How about: 'showmap {{pid}} | grep TOTAL'
rnephew@google.com changed reviewers: + rnephew@google.com
https://codereview.chromium.org/1077173002/diff/1/build/android/pylib/device/... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1077173002/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:1024: large_output=(not bool(size)))) On 2015/04/13 09:58:32, perezju wrote: > I don't think this makes much sense, and introduces a cyclic dependency that > makes me worry. > > We are using "cat" as an optimization to avoid having to pull the file from the > device. But with large_output=True we'll instead cat the file in the device to > _another_ file which is then read using ... ReadFile! > > I would prefer: > - If we can't determine the size of the file, play it safe and just do a pull. > - If the file of the size is 0, don't event try to read anything from the > device, just return '' (or '\n'?). In at least the /proc/ fs on the device, and probably elsewhere, some files return 0 size even though they are not 0 size. One example is /proc/wakelocks: adb shell ls -l /proc/wakelocks ; adb shell cat /proc/wakelocks | wc -r--r--r-- root root 0 2015-04-13 13:19 wakelocks 64 576 3729 > - Only use cat if we know that the size is small enough so that large_output > would not be needed (otherwise the "optimization" is pointless).
Added _RunPipedShellCommand for ps | grep and showmap | grep https://codereview.chromium.org/1077173002/diff/1/build/android/pylib/device/... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1077173002/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:1024: large_output=(not bool(size)))) On 2015/04/13 09:58:32, perezju wrote: > I don't think this makes much sense, and introduces a cyclic dependency that > makes me worry. This doesn't make much sense because what it's reading doesn't really make much sense. e.g. telemetry reads /proc/timer_list here: https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... /proc/timer_list reports zero size, but actually will dump quite a bit of data when catting: jbudorick@jbudorick:~$ adb shell ls -l /proc/timer_list && adb pull /proc/timer_list timer_list.txt && ls -l timer_list.txt -r--r--r-- root root 0 2015-04-13 08:18 timer_list 70 KB/s (5670 bytes in 0.078s) -rw-r----- 1 jbudorick eng 5670 Apr 13 06:18 timer_list.txt > > We are using "cat" as an optimization to avoid having to pull the file from the > device. But with large_output=True we'll instead cat the file in the device to > _another_ file which is then read using ... ReadFile! > > I would prefer: > - If we can't determine the size of the file, play it safe and just do a pull. I had thought that pull didn't work with files that report zero size, but it certainly seems to do so. I'll check this again on K and then switch so long as it works. > - If the file of the size is 0, don't event try to read anything from the > device, just return '' (or '\n'?). This is wrong, see above. > - Only use cat if we know that the size is small enough so that large_output > would not be needed (otherwise the "optimization" is pointless). https://codereview.chromium.org/1077173002/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:1359: large_output=True): On 2015/04/13 09:58:32, perezju wrote: > This is precisely one of the things I really think we shouldn't do. > > I think that, instead of large_output=True, we should change the command to 'ps > | grep -F {{process_name}}' instead. Done. https://codereview.chromium.org/1077173002/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:1433: large_output=True) On 2015/04/13 09:58:32, perezju wrote: > How about: 'showmap {{pid}} | grep TOTAL' Done.
Thanks, this is looking really good! I just have a few minor comments/questions. https://codereview.chromium.org/1077173002/diff/20001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1077173002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:579: script = ' '.join(script) I'm not sure I link this. The caller should be responsible for building the script as needed. https://codereview.chromium.org/1077173002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:587: logging.error('exit statuses of shell script %r missing.', script) RunShellCommand should have already logged the script as it was run. So here maybe the message can just say something like: 'pipe exit statuses of shell script missing.' ? https://codereview.chromium.org/1077173002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:589: script, output, status=None, if the PIPESTATUS_LEADER is missing, maybe we want to add back the |pipstatus_line| back to the |output| for the exception message? https://codereview.chromium.org/1077173002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1045: if size > 0 and size <= self._MAX_ADB_OUTPUT_LENGTH: I think you can write this as: 0 < size <= self._MAX_ADB_OUTPUT_LENGTH https://codereview.chromium.org/1077173002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1383: 'ps | grep -F %s' % process_name, check_return=True) use cmd_helper.SingleQuote(process_name) https://codereview.chromium.org/1077173002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1384: except device_errors.AdbShellCommandFailedError as e: just to clarify, what is the relationship between the "pipe exit statuses" and the regular exit status checked by check_return=True? In particular, are we sure that e.stats below will always be a list? Is it possible that the pipe exit statues are all fine, but the "main" exit status fails? (If not, maybe _RunPopedShellCommand should always run with check_return=False and avoid the double check?) https://codereview.chromium.org/1077173002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1465: 'showmap %s | grep TOTAL' % str(pid), as_root=True, check_return=True) maybe rewrite as: 'showmap %d | grep TOTAL' % pid so we don't need any quoting, while making sure that the pid is really a number.
On 2015/04/14 10:17:29, perezju wrote: > https://codereview.chromium.org/1077173002/diff/20001/build/android/pylib/dev... > build/android/pylib/device/device_utils.py:579: script = ' '.join(script) > I'm not sure I link this. The caller should be responsible for building the > script as needed. s/link/like/
https://codereview.chromium.org/1077173002/diff/20001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1077173002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1465: 'showmap %s | grep TOTAL' % str(pid), as_root=True, check_return=True) On 2015/04/14 10:17:29, perezju wrote: > maybe rewrite as: 'showmap %d | grep TOTAL' % pid > > so we don't need any quoting, while making sure that the pid is really a number. actually, maybe: 'showmap %d | grep TOTAL' % int(pid) because (I don't like it) but pids are often passed around as strings.
https://codereview.chromium.org/1077173002/diff/20001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1077173002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:579: script = ' '.join(script) On 2015/04/14 10:17:29, perezju wrote: > I'm not sure I link this. The caller should be responsible for building the > script as needed. I wasn't sure about this. Removed. https://codereview.chromium.org/1077173002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:587: logging.error('exit statuses of shell script %r missing.', script) On 2015/04/14 10:17:29, perezju wrote: > RunShellCommand should have already logged the script as it was run. So here > maybe the message can just say something like: 'pipe exit statuses of shell > script missing.' ? Done. https://codereview.chromium.org/1077173002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:589: script, output, status=None, On 2015/04/14 10:17:29, perezju wrote: > if the PIPESTATUS_LEADER is missing, maybe we want to add back the > |pipstatus_line| back to the |output| for the exception message? moved output = output[:-1] below this check https://codereview.chromium.org/1077173002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1045: if size > 0 and size <= self._MAX_ADB_OUTPUT_LENGTH: On 2015/04/14 10:17:29, perezju wrote: > I think you can write this as: 0 < size <= self._MAX_ADB_OUTPUT_LENGTH this is sorcery done https://codereview.chromium.org/1077173002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1383: 'ps | grep -F %s' % process_name, check_return=True) On 2015/04/14 10:17:29, perezju wrote: > use cmd_helper.SingleQuote(process_name) Done. https://codereview.chromium.org/1077173002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1384: except device_errors.AdbShellCommandFailedError as e: On 2015/04/14 10:17:29, perezju wrote: > just to clarify, what is the relationship between the "pipe exit statuses" and > the regular exit status checked by check_return=True? > > In particular, are we sure that e.stats below will always be a list? Is it > possible that the pipe exit statues are all fine, but the "main" exit status > fails? (If not, maybe _RunPopedShellCommand should always run with > check_return=False and avoid the double check?) I'm not sure whether $? is the same as the last item in $PIPESTATUS or if it's any failure that happened in the pipe. I suppose that, in that scenario, e.status would not be a list. I'm ok with having check_return explicitly off in calls to _RunPipedShellCommand. https://codereview.chromium.org/1077173002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1465: 'showmap %s | grep TOTAL' % str(pid), as_root=True, check_return=True) On 2015/04/14 10:49:19, perezju wrote: > On 2015/04/14 10:17:29, perezju wrote: > > maybe rewrite as: 'showmap %d | grep TOTAL' % pid > > > > so we don't need any quoting, while making sure that the pid is really a > number. > > actually, maybe: 'showmap %d | grep TOTAL' % int(pid) > > because (I don't like it) but pids are often passed around as strings. Done.
lgtm with nits https://codereview.chromium.org/1077173002/diff/40001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1077173002/diff/40001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1384: check_return=False) nit: I'm ok with this, but maybe _RunPipedShellCommand should set kwargs['check_return'] = False? https://codereview.chromium.org/1077173002/diff/40001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1555: if d.GetDeviceSerial() not in blacklist] what's this? merged from another CL?
ptal https://codereview.chromium.org/1077173002/diff/40001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1077173002/diff/40001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1384: check_return=False) On 2015/04/14 14:13:35, perezju wrote: > nit: I'm ok with this, but maybe _RunPipedShellCommand should set > kwargs['check_return'] = False? After our offline discussion, I realized that check_return will only check the PIPESTATUS echo, not the actual command. We do care if that fails, so I'm actually going the other way and setting kwargs['check_return'] = True. https://codereview.chromium.org/1077173002/diff/40001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1555: if d.GetDeviceSerial() not in blacklist] On 2015/04/14 14:13:35, perezju wrote: > what's this? merged from another CL? You're looking at the 2-3 diff, I take it?
lgtm https://codereview.chromium.org/1077173002/diff/40001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1077173002/diff/40001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1555: if d.GetDeviceSerial() not in blacklist] On 2015/04/14 16:56:12, jbudorick wrote: > On 2015/04/14 14:13:35, perezju wrote: > > what's this? merged from another CL? > > You're looking at the 2-3 diff, I take it? I guess that was it.
https://codereview.chromium.org/1077173002/diff/60001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1077173002/diff/60001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1384: cmd_helper.SingleQuote('ps | grep -F %s' % process_name)) Oh, no, this is wrong, I think. It should be: ps_output = self._RunPipedShellCommand( 'ps | grep -F %s' % cmd_helper.SingleQuote(process_name)) just to make sure that the argument to grep is quoted appropriately if needed. I wonder how were the unittests passing?
Also you probably need to rebase after https://codereview.chromium.org/1079113002/
https://codereview.chromium.org/1077173002/diff/60001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1077173002/diff/60001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1384: cmd_helper.SingleQuote('ps | grep -F %s' % process_name)) On 2015/04/15 09:48:43, perezju wrote: > Oh, no, this is wrong, I think. It should be: > > ps_output = self._RunPipedShellCommand( > 'ps | grep -F %s' % cmd_helper.SingleQuote(process_name)) > > just to make sure that the argument to grep is quoted appropriately if needed. Done. > > I wonder how were the unittests passing? The corresponding changes to the unit tests were wrong, too.
The CQ bit was checked by jbudorick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rnephew@chromium.org, perezju@chromium.org Link to the patchset: https://codereview.chromium.org/1077173002/#ps80001 (title: "rebase and fix SingleQuote")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1077173002/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/6e20669903fad7caa6d6b326ba9e6992f0761680 Cr-Commit-Position: refs/heads/master@{#325241} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
