Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
278763005:
[Telemetry] Speed up page_cycler on android. (Closed)
|
Created:
6 years, 7 months ago by bulach Modified:
6 years, 7 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Telemetry] Speed up page_cycler on android.
4m7s -> 3m51s: remove expensive regexp.
Short-circuits "VmHWM".
Remove old stats for xoom tablets.
BUG=372489
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270329
Patch Set 1 #
Total comments: 6
Messages
Total messages: 13 (0 generated)
ptal
lgtm https://codereview.chromium.org/278763005/diff/1/build/android/pylib/android_... File build/android/pylib/android_commands.py (left): https://codereview.chromium.org/278763005/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1627: for line in self.GetProtectedFileContents('/d/nvmap/generic-0/clients'): Are you sure we don't care about this any more for any nexus on the bots? If it is at all useful, we could also perhaps check for the file once, cache whether it exists and then shortcut reading the file if it didn't exist.
thanks tony! question below: https://codereview.chromium.org/278763005/diff/1/build/android/pylib/android_... File build/android/pylib/android_commands.py (left): https://codereview.chromium.org/278763005/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1627: for line in self.GetProtectedFileContents('/d/nvmap/generic-0/clients'): On 2014/05/13 14:37:50, tonyg wrote: > Are you sure we don't care about this any more for any nexus on the bots? If it > is at all useful, we could also perhaps check for the file once, cache whether > it exists and then shortcut reading the file if it didn't exist. well... :) way up in telemetry, this is not being used at all: https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... I'm not aware of any bots looking into this. ok to nuke it? git log would show anyways, should we ever want to bring this back...
The CQ bit was checked by tonyg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/278763005/1
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was checked by tonyg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/278763005/1
Message was sent while issue was closed.
Change committed as 270329
Message was sent while issue was closed.
This is my first code review here, so forgive me if this is inappropriate nit-picking, I need to calibrate my expectations! Simon https://codereview.chromium.org/278763005/diff/1/build/android/pylib/android_... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/278763005/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1627: peak_value_kb = int(line.split(':')[1].strip().split(' ')[0]) It looks like the code to read the peak_value_kb is almost the same as the new (much nicer!) code you added above to read the key & usage_kb. Is there a way to avoid this (almost) duplication? [... long discussion about functions returning keys & ints that aren't always used] https://codereview.chromium.org/278763005/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1631: logging.warning('Could not find memory peak value for pid ' + str(pid)) This relies on a zero peak value meaning it couldn't find a peak value. I assume that a zero-peak is not possible, but this feels like using one variable for two separate jobs. Especially as 'if not peak_value_kb' is treating peak_value_kb as a boolean, when we all know its really an int! Now the 'break' exists we could write the peak value directly into the usage_dict (unless writing 0 in is necessary?), and then use it's absence from the dictionary to trigger the logging warning... or set a 'key found' flag to true if efficiency is an issue... I've gone too far down this rabbit hole for a single line of code now ...
Message was sent while issue was closed.
thanks simon! comments inline, and new patch up for review. https://codereview.chromium.org/278763005/diff/1/build/android/pylib/android_... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/278763005/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1627: peak_value_kb = int(line.split(':')[1].strip().split(' ')[0]) On 2014/05/14 16:32:18, picksi wrote: > It looks like the code to read the peak_value_kb is almost the same as the new > (much nicer!) code you added above to read the key & usage_kb. Is there a way to > avoid this (almost) duplication? [... long discussion about functions returning > keys & ints that aren't always used] hehe :) there was another follow up on this: https://codereview.chromium.org/280703010/diff/20001/build/android/pylib/andr... so this duplication is no longer happening: the bit above is now using "showmap" command, which faster but has a slightly different format.. having said that, thanks for highlighting this! there's no need to use a "defaultdict" anymore, patch on its way: https://codereview.chromium.org/285693008 https://codereview.chromium.org/278763005/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1631: logging.warning('Could not find memory peak value for pid ' + str(pid)) On 2014/05/14 16:32:18, picksi wrote: > This relies on a zero peak value meaning it couldn't find a peak value. I assume > that a zero-peak is not possible, but this feels like using one variable for two > separate jobs. Especially as 'if not peak_value_kb' is treating peak_value_kb as > a boolean, when we all know its really an int! > > Now the 'break' exists we could write the peak value directly into the > usage_dict (unless writing 0 in is necessary?), and then use it's absence from > the dictionary to trigger the logging warning... or set a 'key found' flag to > true if efficiency is an issue... > > I've gone too far down this rabbit hole for a single line of code now ... yeah, this is a good use for python's "for..else" construction! (having said that, afaict the styleguide allows using boolean coercion for ints and None on "if" clauses, so it wasn't truly wrong given that it can't have a 0 peak... but yes, it'll be clearer now anyways :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
