Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1228393009:
Upstream meminfo.py (Closed)
|
Created:
5 years, 5 months ago by David Trainor- moved to gerrit Modified:
5 years, 5 months ago Reviewers:
nyquist 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. |
DescriptionUpstream meminfo.py
- Rewrite it a bit to clean it up and get it working properly again
- The script tracks memory on processes on an Android device
BUG=
Committed: https://crrev.com/ef40dbe22b8c4346f9e5752fea4ecbed09ca19cd
Cr-Commit-Position: refs/heads/master@{#339757}
Patch Set 1 #
Total comments: 36
Patch Set 2 : Addressed comments #Patch Set 3 : Fixed potential bug in QueryMemory #Patch Set 4 : Fix comment line. #Messages
Total messages: 9 (2 generated)
dtrainor@chromium.org changed reviewers: + nyquist@chromium.org
ptal thanks!
https://codereview.chromium.org/1228393009/diff/1/tools/android/meminfo.py File tools/android/meminfo.py (right): https://codereview.chromium.org/1228393009/diff/1/tools/android/meminfo.py#ne... tools/android/meminfo.py:9: import commands Nit: Is this import unused? https://codereview.chromium.org/1228393009/diff/1/tools/android/meminfo.py#ne... tools/android/meminfo.py:10: import copy Nit: Is this import unused? https://codereview.chromium.org/1228393009/diff/1/tools/android/meminfo.py#ne... tools/android/meminfo.py:50: def ValidatePositiveNumber(val): This method allows 0, which is not positive. How about ValidateNonNegativeNumber? If so, please update comment and ArgumentTypeError. https://codereview.chromium.org/1228393009/diff/1/tools/android/meminfo.py#ne... tools/android/meminfo.py:111: if (not default_pid or pid == str(default_pid)) and (not process_filter this if-block looks a bit confusing since only the first part after the main 'and' is on this line. Could this be split up differently t make it more readable? Possibly by doing something like: pid_matches = not default_pid or pid == str(default_pid) name_matches = not process_filter or name.find(process_filter) != -1 if pid_matches and name_matches: ... https://codereview.chromium.org/1228393009/diff/1/tools/android/meminfo.py#ne... tools/android/meminfo.py:125: returns a list of values: [ Native, Pss, Dalvik]. If the process is not Nit: Missing space after Dalvid, or extra space before Native. https://codereview.chromium.org/1228393009/diff/1/tools/android/meminfo.py#ne... tools/android/meminfo.py:133: data_idx = None query_idx? https://codereview.chromium.org/1228393009/diff/1/tools/android/meminfo.py#ne... tools/android/meminfo.py:134: if match[0] == 'Native': Would this match the app summary as well? ... App Summary Pss(KB) ------ Java Heap: 11540 Native Heap: 10912 ... If so, the match ends up being Heap: which can't be easily converted to float. https://codereview.chromium.org/1228393009/diff/1/tools/android/meminfo.py#ne... tools/android/meminfo.py:151: # TODO(dtrainor): Find a generic way to query/fall back for other devices. Nit: Is this indent using spaces instead of tabs? https://codereview.chromium.org/1228393009/diff/1/tools/android/meminfo.py#ne... tools/android/meminfo.py:153: __NV_MAP_MODELS = ['Xoom'] Do we really need to support Xoom? https://codereview.chromium.org/1228393009/diff/1/tools/android/meminfo.py#ne... tools/android/meminfo.py:222: return GraphisHelper.__QueryNvMap(adb, pid) GraphicsHelper https://codereview.chromium.org/1228393009/diff/1/tools/android/meminfo.py#ne... tools/android/meminfo.py:334: return '\033[%sm'%num Nit: Add spaces around the last % to separate formatting from what to insert? https://codereview.chromium.org/1228393009/diff/1/tools/android/meminfo.py#ne... tools/android/meminfo.py:449: delta = round(delta, 2) Does this really edit the values in |deltas|? You don't need to do deltas[idx] = round(delta, 2) ? https://codereview.chromium.org/1228393009/diff/1/tools/android/meminfo.py#ne... tools/android/meminfo.py:455: 8, False, self.__GetDiffColor(deltas[idx])) Nit: Should this be indented one more step? https://codereview.chromium.org/1228393009/diff/1/tools/android/meminfo.py#ne... tools/android/meminfo.py:513: pids = set() This is done both here and in PrettyGraph. Extract to a common method? https://codereview.chromium.org/1228393009/diff/1/tools/android/meminfo.py#ne... tools/android/meminfo.py:608: dest='limit', Nit: I personally find it easier to read later in the code 'timelimit' instead of just 'limit'. https://codereview.chromium.org/1228393009/diff/1/tools/android/meminfo.py#ne... tools/android/meminfo.py:638: help='Pdf file to save graph of memory stats to.') Nit: PDF https://codereview.chromium.org/1228393009/diff/1/tools/android/meminfo.py#ne... tools/android/meminfo.py:667: elif not adb or device != str(adb): I had to look up AdbWrapper.__str__. That's cute! https://codereview.chromium.org/1228393009/diff/1/tools/android/meminfo.py#ne... tools/android/meminfo.py:669: old_snapshot = None Would we want to clear snapshots?
addressed comments. thanks! https://chromiumcodereview.appspot.com/1228393009/diff/1/tools/android/meminf... File tools/android/meminfo.py (right): https://chromiumcodereview.appspot.com/1228393009/diff/1/tools/android/meminf... tools/android/meminfo.py:9: import commands On 2015/07/17 21:42:11, nyquist wrote: > Nit: Is this import unused? Done. https://chromiumcodereview.appspot.com/1228393009/diff/1/tools/android/meminf... tools/android/meminfo.py:10: import copy On 2015/07/17 21:42:10, nyquist wrote: > Nit: Is this import unused? Done. https://chromiumcodereview.appspot.com/1228393009/diff/1/tools/android/meminf... tools/android/meminfo.py:50: def ValidatePositiveNumber(val): On 2015/07/17 21:42:11, nyquist wrote: > This method allows 0, which is not positive. How about > ValidateNonNegativeNumber? If so, please update comment and ArgumentTypeError. Done. https://chromiumcodereview.appspot.com/1228393009/diff/1/tools/android/meminf... tools/android/meminfo.py:111: if (not default_pid or pid == str(default_pid)) and (not process_filter On 2015/07/17 21:42:10, nyquist wrote: > this if-block looks a bit confusing since only the first part after the main > 'and' is on this line. Could this be split up differently t make it more > readable? > > Possibly by doing something like: > pid_matches = not default_pid or pid == str(default_pid) > name_matches = not process_filter or name.find(process_filter) != -1 > if pid_matches and name_matches: > ... Done. https://chromiumcodereview.appspot.com/1228393009/diff/1/tools/android/meminf... tools/android/meminfo.py:125: returns a list of values: [ Native, Pss, Dalvik]. If the process is not On 2015/07/17 21:42:11, nyquist wrote: > Nit: Missing space after Dalvid, or extra space before Native. Done. https://chromiumcodereview.appspot.com/1228393009/diff/1/tools/android/meminf... tools/android/meminfo.py:133: data_idx = None On 2015/07/17 21:42:10, nyquist wrote: > query_idx? gah good catch! https://chromiumcodereview.appspot.com/1228393009/diff/1/tools/android/meminf... tools/android/meminfo.py:134: if match[0] == 'Native': On 2015/07/17 21:42:10, nyquist wrote: > Would this match the app summary as well? > ... > App Summary > Pss(KB) > ------ > Java Heap: 11540 > Native Heap: 10912 > ... > > If so, the match ends up being Heap: which can't be easily converted to float. Since the query_idx is -2, wouldn't it pull the "heap alloc" column and count backwards? https://chromiumcodereview.appspot.com/1228393009/diff/1/tools/android/meminf... tools/android/meminfo.py:151: # TODO(dtrainor): Find a generic way to query/fall back for other devices. On 2015/07/17 21:42:11, nyquist wrote: > Nit: Is this indent using spaces instead of tabs? I don't think so. At least not in my editor! Will replace anyway to make sure! https://chromiumcodereview.appspot.com/1228393009/diff/1/tools/android/meminf... tools/android/meminfo.py:153: __NV_MAP_MODELS = ['Xoom'] On 2015/07/17 21:42:11, nyquist wrote: > Do we really need to support Xoom? No, but I'm not sure if other drivers use NV_MAP for tracking their graphics memory usage. I'll talk to Grace and see if it's deprecated. https://chromiumcodereview.appspot.com/1228393009/diff/1/tools/android/meminf... tools/android/meminfo.py:222: return GraphisHelper.__QueryNvMap(adb, pid) On 2015/07/17 21:42:10, nyquist wrote: > GraphicsHelper Guess you can tell I didn't test zoom since the refactor. ;) https://chromiumcodereview.appspot.com/1228393009/diff/1/tools/android/meminf... tools/android/meminfo.py:334: return '\033[%sm'%num On 2015/07/17 21:42:11, nyquist wrote: > Nit: Add spaces around the last % to separate formatting from what to insert? Done. https://chromiumcodereview.appspot.com/1228393009/diff/1/tools/android/meminf... tools/android/meminfo.py:449: delta = round(delta, 2) On 2015/07/17 21:42:10, nyquist wrote: > Does this really edit the values in |deltas|? You don't need to do deltas[idx] = > round(delta, 2) ? Turns out this code can be removed because I run because I round it below when I print it... (line 454). https://chromiumcodereview.appspot.com/1228393009/diff/1/tools/android/meminf... tools/android/meminfo.py:455: 8, False, self.__GetDiffColor(deltas[idx])) On 2015/07/17 21:42:11, nyquist wrote: > Nit: Should this be indented one more step? Done. https://chromiumcodereview.appspot.com/1228393009/diff/1/tools/android/meminf... tools/android/meminfo.py:513: pids = set() On 2015/07/17 21:42:11, nyquist wrote: > This is done both here and in PrettyGraph. Extract to a common method? Done. https://chromiumcodereview.appspot.com/1228393009/diff/1/tools/android/meminf... tools/android/meminfo.py:608: dest='limit', On 2015/07/17 21:42:10, nyquist wrote: > Nit: I personally find it easier to read later in the code 'timelimit' instead > of just 'limit'. Done. https://chromiumcodereview.appspot.com/1228393009/diff/1/tools/android/meminf... tools/android/meminfo.py:638: help='Pdf file to save graph of memory stats to.') On 2015/07/17 21:42:11, nyquist wrote: > Nit: PDF Done. https://chromiumcodereview.appspot.com/1228393009/diff/1/tools/android/meminf... tools/android/meminfo.py:667: elif not adb or device != str(adb): On 2015/07/17 21:42:10, nyquist wrote: > I had to look up AdbWrapper.__str__. That's cute! Yeah I thought so too when I saw that! :) https://chromiumcodereview.appspot.com/1228393009/diff/1/tools/android/meminf... tools/android/meminfo.py:669: old_snapshot = None On 2015/07/17 21:42:11, nyquist wrote: > Would we want to clear snapshots? O_o good point!
lgtm
The CQ bit was checked by dtrainor@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1228393009/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ef40dbe22b8c4346f9e5752fea4ecbed09ca19cd Cr-Commit-Position: refs/heads/master@{#339757} |
