Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1375043002:
Speed up incremental_install by caching device checksums between runs. (Closed)
|
Created:
5 years, 2 months ago by agrieve Modified:
5 years, 2 months ago Reviewers:
jbudorick CC:
chromium-reviews, klundberg+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@device-utils-brace-fix Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSpeed up incremental_install by caching device checksums between runs.
DeviceUtils:
- Adds a constructor paramter to enable caching of checksums used by
PushChangedFiles.
- Adds methods to load / save a DeviceUtils cache.
installer.py:
- Enable caching of pushed files between runs by storing
- Add concurrency to setup / finalize steps
A no-op install now takes less than 2 seconds on my machine.
BUG=520082
Committed: https://crrev.com/edb55bd1cf583ec18040d8f1e7fbaef409e2a0ca
Cr-Commit-Position: refs/heads/master@{#352374}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 10
Patch Set 3 : review comments 1 #Patch Set 4 : rebase #Patch Set 5 : rebase for real #Patch Set 6 : update cache after pushing #Patch Set 7 : fix cache_commit_func #
Dependent Patchsets: Messages
Total messages: 37 (17 generated)
agrieve@chromium.org changed reviewers: + jbudorick@chromium.org
On 2015/09/29 17:22:07, agrieve wrote: > mailto:agrieve@chromium.org changed reviewers: > + mailto:jbudorick@chromium.org please review.
On 2015/09/29 17:23:08, agrieve wrote: > On 2015/09/29 17:22:07, agrieve wrote: > > mailto:agrieve@chromium.org changed reviewers: > > + mailto:jbudorick@chromium.org > > please review. nudge
This looks pretty good, and it's something I'd be interested in deploying to the bots once we no longer need to wipe them every run. https://codereview.chromium.org/1375043002/diff/20001/build/android/devil/and... File build/android/devil/android/device_utils.py (right): https://codereview.chromium.org/1375043002/diff/20001/build/android/devil/and... build/android/devil/android/device_utils.py:167: def __init__(self, device, enable_device_files_cache=False, Document this parameter. https://codereview.chromium.org/1375043002/diff/20001/build/android/devil/and... build/android/devil/android/device_utils.py:1940: # Map of device_path -> [ignore_other_files, map of path->checksum] Isn't this just a map of device_path -> checksum? https://codereview.chromium.org/1375043002/diff/20001/build/android/increment... File build/android/incremental_install/installer.py (right): https://codereview.chromium.org/1375043002/diff/20001/build/android/increment... build/android/incremental_install/installer.py:73: action='store_true', nit: using dest='threading', action='store_false' here would help clear up the double-negative below. https://codereview.chromium.org/1375043002/diff/20001/build/android/increment... build/android/incremental_install/installer.py:177: lines = device.RunShellCommand(cmd, large_output=True) This appears to be using the failure mode as a valid path. In that case, I'd recommend either explicitly specifying check_return=False or specifying check_return=True and catching an exception for the case where the cache doesn't exist. (I'm planning to eventually at least flip the default for check_return if not switch it to always-on.) https://codereview.chromium.org/1375043002/diff/20001/build/android/increment... build/android/incremental_install/installer.py:206: concurrently = not args.no_threading by using dest='threading' above, you could eliminate concurrently and just use args.threading directly.
https://codereview.chromium.org/1375043002/diff/20001/build/android/devil/and... File build/android/devil/android/device_utils.py (right): https://codereview.chromium.org/1375043002/diff/20001/build/android/devil/and... build/android/devil/android/device_utils.py:167: def __init__(self, device, enable_device_files_cache=False, On 2015/10/01 17:15:26, jbudorick wrote: > Document this parameter. Done. https://codereview.chromium.org/1375043002/diff/20001/build/android/devil/and... build/android/devil/android/device_utils.py:1940: # Map of device_path -> [ignore_other_files, map of path->checksum] On 2015/10/01 17:15:26, jbudorick wrote: > Isn't this just a map of device_path -> checksum? The comment is correct. The reason it's not a map of device_path -> checksum: - Say you push to /sdcard/foo: - Entries are now: /sdcard/foo/file1, /sdcard/foo/file2 - Say you now push to /sdcard: - You should not use the cached entries because they don't include all the files. Likewise, the ignore_other_files is necessary because it causes some files to be skipped over when set. https://codereview.chromium.org/1375043002/diff/20001/build/android/increment... File build/android/incremental_install/installer.py (right): https://codereview.chromium.org/1375043002/diff/20001/build/android/increment... build/android/incremental_install/installer.py:73: action='store_true', On 2015/10/01 17:15:26, jbudorick wrote: > nit: using dest='threading', action='store_false' here would help clear up the > double-negative below. Done. https://codereview.chromium.org/1375043002/diff/20001/build/android/increment... build/android/incremental_install/installer.py:177: lines = device.RunShellCommand(cmd, large_output=True) On 2015/10/01 17:15:27, jbudorick wrote: > This appears to be using the failure mode as a valid path. In that case, I'd > recommend either explicitly specifying check_return=False or specifying > check_return=True and catching an exception for the case where the cache doesn't > exist. > > (I'm planning to eventually at least flip the default for check_return if not > switch it to always-on.) Done. https://codereview.chromium.org/1375043002/diff/20001/build/android/increment... build/android/incremental_install/installer.py:206: concurrently = not args.no_threading On 2015/10/01 17:15:27, jbudorick wrote: > by using dest='threading' above, you could eliminate concurrently and just use > args.threading directly. Done. Although sadly the 80 character limit adds an extra level of complexity to trying to write succinct code :(.
lgtm
The CQ bit was checked by agrieve@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1375043002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1375043002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1375043002/#ps60001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1375043002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1375043002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1375043002/#ps80001 (title: "rebase for real")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1375043002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1375043002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1375043002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1375043002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1375043002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1375043002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1375043002/#ps120001 (title: "fix cache_commit_func")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1375043002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1375043002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by agrieve@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1375043002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1375043002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/edb55bd1cf583ec18040d8f1e7fbaef409e2a0ca Cr-Commit-Position: refs/heads/master@{#352374} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
