Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
839893004:
Refactor test scripts for android to improve performance (Closed)
|
Created:
5 years, 11 months ago by Jaekyun Seok (inactive) Modified:
5 years, 11 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org, klundberg Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor test scripts for android to improve performance
-. Skip checking package existence
-. Skip stopping an Activity
-. Reduce waiting time for fifo
BUG=449354
Committed: https://crrev.com/e3a24e9786ac3483c60e91c39742243aa15e7a14
Cr-Commit-Position: refs/heads/master@{#312552}
Patch Set 1 #Patch Set 2 : Rename a parameter #
Total comments: 20
Patch Set 3 : Apply jbudorick's comments #Patch Set 4 : Cache application paths #
Total comments: 3
Patch Set 5 : Skip checking package on JBMR2 or above #
Messages
Total messages: 31 (6 generated)
jaekyun@chromium.org changed reviewers: + klundberg@chromium.org
Karin, please review this CL.
jbudorick@chromium.org changed reviewers: + jbudorick@chromium.org
Started a try job to see how this affects content_browsertest execution time. I'm curious as to what changes will be responsible for any savings, since there are three separate optimizations in here: - the package_check_needed flag - the fifo check sleep tuning - the force_stop change https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:626: package_check_needed=True): timeout and retries should be the last 2 parameters. https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:642: if (not package_check_needed) or self.GetApplicationPath(package): I'm extremely wary about adding more somewhat superfluous options to DeviceUtils, _especially_ ones that let the client screw up (as this one does). https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/gtes... File build/android/pylib/gtest/test_package_apk.py (right): https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/gtes... build/android/pylib/gtest/test_package_apk.py:58: for i in range(110): 110...? https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/gtes... File build/android/pylib/gtest/test_runner.py (right): https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/gtes... build/android/pylib/gtest/test_runner.py:18: g_re_run = re.compile('\\[ RUN \\] ?(.*)\r\n') Constants should be ALL_CAPS, not g_with_underscores
On 2015/01/16 14:21:42, jbudorick wrote: > Started a try job to see how this affects content_browsertest execution time. > 7m2s with one test that needed to be retried is substantial. I'm even more curious about how that breaks down across your optimizations now. > I'm curious as to what changes will be responsible for any savings, since there > are three separate optimizations in here: > - the package_check_needed flag > - the fifo check sleep tuning > - the force_stop change > > https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... > File build/android/pylib/device/device_utils.py (right): > > https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... > build/android/pylib/device/device_utils.py:626: package_check_needed=True): > timeout and retries should be the last 2 parameters. > > https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... > build/android/pylib/device/device_utils.py:642: if (not package_check_needed) or > self.GetApplicationPath(package): > I'm extremely wary about adding more somewhat superfluous options to > DeviceUtils, _especially_ ones that let the client screw up (as this one does). > > https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/gtes... > File build/android/pylib/gtest/test_package_apk.py (right): > > https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/gtes... > build/android/pylib/gtest/test_package_apk.py:58: for i in range(110): > 110...? > > https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/gtes... > File build/android/pylib/gtest/test_runner.py (right): > > https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/gtes... > build/android/pylib/gtest/test_runner.py:18: g_re_run = re.compile('\\[ RUN > \\] ?(.*)\r\n') > Constants should be ALL_CAPS, not g_with_underscores
klundberg@chromium.org changed reviewers: - klundberg@chromium.org
klundberg@chromium.org changed reviewers: + klundberg@chromium.org
Moved myself to CC as John is the best reviewer for this :-)
On 2015/01/16 15:18:18, jbudorick wrote: > On 2015/01/16 14:21:42, jbudorick wrote: > > Started a try job to see how this affects content_browsertest execution time. > > > > 7m2s with one test that needed to be retried is substantial. I'm even more > curious about how that breaks down across your optimizations now. I will run tests more times, and let you know the result. > > > I'm curious as to what changes will be responsible for any savings, since > there > > are three separate optimizations in here: > > - the package_check_needed flag When I tested, each call to self.GetApplicationPath(package) took almost 1 sec. So applying this can save time. > > - the fifo check sleep tuning The current logic sleeps too much when the fifo isn't ready soon. For example, if the fifo gets ready after 1.5 secs, the current logic sleeps for 3 secs (1 + 2). > > - the force_stop change I wanted to remove options which isn't needed obviously. https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:626: package_check_needed=True): On 2015/01/16 14:21:41, jbudorick wrote: > timeout and retries should be the last 2 parameters. Done. https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:642: if (not package_check_needed) or self.GetApplicationPath(package): On 2015/01/16 14:21:41, jbudorick wrote: > I'm extremely wary about adding more somewhat superfluous options to > DeviceUtils, _especially_ ones that let the client screw up (as this one does). I agree with your concern, but each call to self.GetApplicationPath(package) took almost 1 sec when I tested, so I want to skip this when it isn't needed obviously. https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/gtes... File build/android/pylib/gtest/test_package_apk.py (right): https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/gtes... build/android/pylib/gtest/test_package_apk.py:58: for i in range(110): On 2015/01/16 14:21:41, jbudorick wrote: > 110...? I just wanted to keep the total possible sleep time. The previous total time was 55s (1+2+...+10). https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/gtes... File build/android/pylib/gtest/test_runner.py (right): https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/gtes... build/android/pylib/gtest/test_runner.py:18: g_re_run = re.compile('\\[ RUN \\] ?(.*)\r\n') On 2015/01/16 14:21:41, jbudorick wrote: > Constants should be ALL_CAPS, not g_with_underscores Done.
Kicked off another try job. https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:642: if (not package_check_needed) or self.GetApplicationPath(package): On 2015/01/18 23:01:18, Jaekyun Seok wrote: > On 2015/01/16 14:21:41, jbudorick wrote: > > I'm extremely wary about adding more somewhat superfluous options to > > DeviceUtils, _especially_ ones that let the client screw up (as this one > does). > > I agree with your concern, but each call to self.GetApplicationPath(package) > took almost 1 sec when I tested, so I want to skip this when it isn't needed > obviously. That's reasonable; I'm just not sure that the client should be determining when this isn't needed. This exposes an implementation detail that I don't think we should be exposing. I wonder if there's something a bit more clever we can do with caching the output of pm list packages... the tricky part of that would be keeping the cache from going stale. https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/gtes... File build/android/pylib/gtest/test_package_apk.py (right): https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/gtes... build/android/pylib/gtest/test_package_apk.py:58: for i in range(110): On 2015/01/18 23:01:18, Jaekyun Seok wrote: > On 2015/01/16 14:21:41, jbudorick wrote: > > 110...? > > I just wanted to keep the total possible sleep time. The previous total time was > 55s (1+2+...+10). Just make it 100.
https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:642: if (not package_check_needed) or self.GetApplicationPath(package): On 2015/01/20 14:11:23, jbudorick wrote: > That's reasonable; I'm just not sure that the client should be determining when > this isn't needed. This exposes an implementation detail that I don't think we > should be exposing. > > I wonder if there's something a bit more clever we can do with caching the > output of pm list packages... the tricky part of that would be keeping the cache > from going stale. Caching device status seems dangerous because we can't track all the status changes on a device. How about calling "device.RunShellCommand(['pm', 'clear', package], check_return=True)" directly from TestPackageApk.ClearApplicationState? https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/gtes... File build/android/pylib/gtest/test_package_apk.py (right): https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/gtes... build/android/pylib/gtest/test_package_apk.py:58: for i in range(110): On 2015/01/20 14:11:23, jbudorick wrote: > Just make it 100. I will do that.
https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:642: if (not package_check_needed) or self.GetApplicationPath(package): On 2015/01/20 22:00:23, Jaekyun Seok wrote: > On 2015/01/20 14:11:23, jbudorick wrote: > > That's reasonable; I'm just not sure that the client should be determining > when > > this isn't needed. This exposes an implementation detail that I don't think we > > should be exposing. > > > > I wonder if there's something a bit more clever we can do with caching the > > output of pm list packages... the tricky part of that would be keeping the > cache > > from going stale. > > Caching device status seems dangerous because we can't track all the status What do you mean by "we can't track all the status changes?" > changes on a device. How about calling "device.RunShellCommand(['pm', 'clear', > package], check_return=True)" directly from > TestPackageApk.ClearApplicationState? No, we actively discourage calling RunShellCommand when a viable alternative exists (as it does here).
https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:642: if (not package_check_needed) or self.GetApplicationPath(package): On 2015/01/20 22:04:04, jbudorick wrote: > On 2015/01/20 22:00:23, Jaekyun Seok wrote: > > On 2015/01/20 14:11:23, jbudorick wrote: > > > That's reasonable; I'm just not sure that the client should be determining > > when > > > this isn't needed. This exposes an implementation detail that I don't think > we > > > should be exposing. > > > > > > I wonder if there's something a bit more clever we can do with caching the > > > output of pm list packages... the tricky part of that would be keeping the > > cache > > > from going stale. > > > > Caching device status seems dangerous because we can't track all the status > > What do you mean by "we can't track all the status changes?" For example, a package could be uninstalled by device.RunShellCommand(...), and in that case, we can't update cache. Do you think we can ignore such case? > > > changes on a device. How about calling "device.RunShellCommand(['pm', 'clear', > > package], check_return=True)" directly from > > TestPackageApk.ClearApplicationState? > > No, we actively discourage calling RunShellCommand when a viable alternative > exists (as it does here).
jbudorick@chromium.org changed reviewers: + perezju@chromium.org
+Juan, who has also done a substantial amount of work on DeviceUtils https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:642: if (not package_check_needed) or self.GetApplicationPath(package): On 2015/01/20 22:11:17, Jaekyun Seok wrote: > On 2015/01/20 22:04:04, jbudorick wrote: > > On 2015/01/20 22:00:23, Jaekyun Seok wrote: > > > On 2015/01/20 14:11:23, jbudorick wrote: > > > > That's reasonable; I'm just not sure that the client should be determining > > > when > > > > this isn't needed. This exposes an implementation detail that I don't > think > > we > > > > should be exposing. > > > > > > > > I wonder if there's something a bit more clever we can do with caching the > > > > output of pm list packages... the tricky part of that would be keeping the > > > cache > > > > from going stale. > > > > > > Caching device status seems dangerous because we can't track all the status > > > > What do you mean by "we can't track all the status changes?" > > For example, a package could be uninstalled by device.RunShellCommand(...), and > in that case, we can't update cache. Do you think we can ignore such case? > I think that case is less risky than the current implementation, but I'll admit that it isn't risk-free. (Cases like that are one reason why we discourage using RunShellCommand raw where alternatives exist, as one does here) That said, we do some amount of caching already that could conceivably be messed up by RunShellCommand. Perhaps calls to RunShellCommand should clear non-RO cache items. > > > > > changes on a device. How about calling "device.RunShellCommand(['pm', > 'clear', > > > package], check_return=True)" directly from > > > TestPackageApk.ClearApplicationState? > > > > No, we actively discourage calling RunShellCommand when a viable alternative > > exists (as it does here). >
On 2015/01/20 22:22:44, jbudorick wrote: > +Juan, who has also done a substantial amount of work on DeviceUtils > > https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... > File build/android/pylib/device/device_utils.py (right): > > https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... > build/android/pylib/device/device_utils.py:642: if (not package_check_needed) or > self.GetApplicationPath(package): > On 2015/01/20 22:11:17, Jaekyun Seok wrote: > > On 2015/01/20 22:04:04, jbudorick wrote: > > > On 2015/01/20 22:00:23, Jaekyun Seok wrote: > > > > On 2015/01/20 14:11:23, jbudorick wrote: > > > > > That's reasonable; I'm just not sure that the client should be > determining > > > > when > > > > > this isn't needed. This exposes an implementation detail that I don't > > think > > > we > > > > > should be exposing. > > > > > > > > > > I wonder if there's something a bit more clever we can do with caching > the > > > > > output of pm list packages... the tricky part of that would be keeping > the > > > > cache > > > > > from going stale. > > > > > > > > Caching device status seems dangerous because we can't track all the > status > > > > > > What do you mean by "we can't track all the status changes?" > > > > For example, a package could be uninstalled by device.RunShellCommand(...), > and > > in that case, we can't update cache. Do you think we can ignore such case? > > > > I think that case is less risky than the current implementation, but I'll admit > that it isn't risk-free. (Cases like that are one reason why we discourage using > RunShellCommand raw where alternatives exist, as one does here) > (to be clear: by "current implementation," I mean your current patchset, not what's in the repo) > That said, we do some amount of caching already that could conceivably be messed > up by RunShellCommand. Perhaps calls to RunShellCommand should clear non-RO > cache items. > > > > > > > > changes on a device. How about calling "device.RunShellCommand(['pm', > > 'clear', > > > > package], check_return=True)" directly from > > > > TestPackageApk.ClearApplicationState? > > > > > > No, we actively discourage calling RunShellCommand when a viable alternative > > > exists (as it does here). > >
https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:642: if (not package_check_needed) or self.GetApplicationPath(package): On 2015/01/20 22:22:43, jbudorick wrote: > I think that case is less risky than the current implementation, but I'll admit > that it isn't risk-free. (Cases like that are one reason why we discourage using > RunShellCommand raw where alternatives exist, as one does here) I see. I will add cache to store application paths. > > That said, we do some amount of caching already that could conceivably be messed > up by RunShellCommand. Perhaps calls to RunShellCommand should clear non-RO > cache items. > For now, RunShellCommand is being used in many sites including TestPackageApk.ClearApplicationState. So clearing cache of application paths for every call to RunShellCommand will make caching useless. I will leave this clearing as TODO.
https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:642: if (not package_check_needed) or self.GetApplicationPath(package): On 2015/01/20 23:20:35, Jaekyun Seok wrote: > On 2015/01/20 22:22:43, jbudorick wrote: > > I think that case is less risky than the current implementation, but I'll > admit > > that it isn't risk-free. (Cases like that are one reason why we discourage > using > > RunShellCommand raw where alternatives exist, as one does here) > > I see. I will add cache to store application paths. This could probably be integrated into the existing cache, although perhaps the caching mechanism should be entirely moved into AdbWrapper. I'd at least recommend waiting until Juan chimes in, though. He's in EMEA, so it'll be a little while. > > > > > That said, we do some amount of caching already that could conceivably be > messed > > up by RunShellCommand. Perhaps calls to RunShellCommand should clear non-RO > > cache items. > > > > For now, RunShellCommand is being used in many sites including > TestPackageApk.ClearApplicationState. So clearing cache of application paths for > every call to RunShellCommand will make caching useless. > I will leave this clearing as TODO. I agree that that's out of scope of the bug. In face, this entire CL may be getting out of the scope of the bug, as it's getting heavily into DeviceUtils performance tuning rather than things specific to content_browsertests.
https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:642: if (not package_check_needed) or self.GetApplicationPath(package): Yeah, I also dislike the idea of adding this option. Because it helps improve the efficiency but only for this particular one caller. I would rather prefer if we find an alternative that could also bring benefits to other clients as well. On 2015/01/20 23:34:24, jbudorick wrote: > This could probably be integrated into the existing cache, although perhaps the > caching mechanism should be entirely moved into AdbWrapper. I'd at least > recommend waiting until Juan chimes in, though. He's in EMEA, so it'll be a > little while. I don't think caching should be done by AdbWrapper, which should only know how to run adb commands, but not what those commands do (e.g. lifetimes of data or what is read-only). > I agree that that's out of scope of the bug. In face, this entire CL may be > getting out of the scope of the bug, as it's getting heavily into DeviceUtils > performance tuning rather than things specific to content_browsertests. I like the idea of caching, but I'm not sure how easy will it be to get it right. We need to check which other DeviceUtils methods deal with pm and should invalidate the cache. Also we should probably do a quick check to see if other clients are calling 'pm' via RunShellCommand since we may want to have those using calling the appropriate DeviceUtils' methods instead. Alternatively, can we check whether the 'pm path' test is actually still needed? I've traced back the CL where that change was introduced to [1], when not having the check would break many bots. I've tested now, however, on a device on my desk and 'pm clear' seems to work fine even with non-existent packages (returns 'Failed' but doesn't hang as the comment suggests). Maybe this was fixed on recent Android versions? If so, maybe we can just skip the test if the version is recent enough? Would this be enough to reap the efficiency benefits without having to introduce much more complexity? [1]: https://codereview.chromium.org/12210036/#ps20002
https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:642: if (not package_check_needed) or self.GetApplicationPath(package): On 2015/01/21 10:28:52, perezju wrote: > Yeah, I also dislike the idea of adding this option. Because it helps improve > the efficiency but only for this particular one caller. I would rather prefer if > we find an alternative that could also bring benefits to other clients as well. > > On 2015/01/20 23:34:24, jbudorick wrote: > > This could probably be integrated into the existing cache, although perhaps > the > > caching mechanism should be entirely moved into AdbWrapper. I'd at least > > recommend waiting until Juan chimes in, though. He's in EMEA, so it'll be a > > little while. > > I don't think caching should be done by AdbWrapper, which should only know how > to run adb commands, but not what those commands do (e.g. lifetimes of data or > what is read-only). > My concern in this case is that AdbWrapper handles Uninstall directly. (I imagine there are similar cases.) > > I agree that that's out of scope of the bug. In face, this entire CL may be > > getting out of the scope of the bug, as it's getting heavily into DeviceUtils > > performance tuning rather than things specific to content_browsertests. > > I like the idea of caching, but I'm not sure how easy will it be to get it > right. We need to check which other DeviceUtils methods deal with pm and should > invalidate the cache. Also we should probably do a quick check to see if other > clients are calling 'pm' via RunShellCommand since we may want to have those > using calling the appropriate DeviceUtils' methods instead. > Agreed, it's tricky. > Alternatively, can we check whether the 'pm path' test is actually still needed? > I've traced back the CL where that change was introduced to [1], when not having > the check would break many bots. > > I've tested now, however, on a device on my desk and 'pm clear' seems to work > fine even with non-existent packages (returns 'Failed' but doesn't hang as the > comment suggests). Maybe this was fixed on recent Android versions? If so, maybe > we can just skip the test if the version is recent enough? Would this be enough > to reap the efficiency benefits without having to introduce much more > complexity? > > [1]: https://codereview.chromium.org/12210036/#ps20002 We'll have to test this out with older versions of android.
https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:642: if (not package_check_needed) or self.GetApplicationPath(package): On 2015/01/21 14:57:00, jbudorick wrote: > On 2015/01/21 10:28:52, perezju wrote: > > Yeah, I also dislike the idea of adding this option. Because it helps improve > > the efficiency but only for this particular one caller. I would rather prefer > if > > we find an alternative that could also bring benefits to other clients as > well. > > > > On 2015/01/20 23:34:24, jbudorick wrote: > > > This could probably be integrated into the existing cache, although perhaps > > the > > > caching mechanism should be entirely moved into AdbWrapper. I'd at least > > > recommend waiting until Juan chimes in, though. He's in EMEA, so it'll be a > > > little while. > > > > I don't think caching should be done by AdbWrapper, which should only know how > > to run adb commands, but not what those commands do (e.g. lifetimes of data or > > what is read-only). > > > > My concern in this case is that AdbWrapper handles Uninstall directly. (I > imagine there are similar cases.) > > > > I agree that that's out of scope of the bug. In face, this entire CL may be > > > getting out of the scope of the bug, as it's getting heavily into > DeviceUtils > > > performance tuning rather than things specific to content_browsertests. > > > > I like the idea of caching, but I'm not sure how easy will it be to get it > > right. We need to check which other DeviceUtils methods deal with pm and > should > > invalidate the cache. Also we should probably do a quick check to see if other > > clients are calling 'pm' via RunShellCommand since we may want to have those > > using calling the appropriate DeviceUtils' methods instead. > > > > Agreed, it's tricky. > > > Alternatively, can we check whether the 'pm path' test is actually still > needed? > > I've traced back the CL where that change was introduced to [1], when not > having > > the check would break many bots. > > > > I've tested now, however, on a device on my desk and 'pm clear' seems to work > > fine even with non-existent packages (returns 'Failed' but doesn't hang as the > > comment suggests). Maybe this was fixed on recent Android versions? If so, > maybe > > we can just skip the test if the version is recent enough? Would this be > enough > > to reap the efficiency benefits without having to introduce much more > > complexity? > > > > [1]: https://codereview.chromium.org/12210036/#ps20002 > > We'll have to test this out with older versions of android. If this last part _does_ work on versions starting with ICS, then that's definitely the way to go. If not, we could also consider implementing a safe version of pm clear that we install to the device (similar to what we do with unzip).
On 2015/01/21 15:03:14, jbudorick wrote: > https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... > File build/android/pylib/device/device_utils.py (right): > > https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... > build/android/pylib/device/device_utils.py:642: if (not package_check_needed) or > self.GetApplicationPath(package): > On 2015/01/21 14:57:00, jbudorick wrote: > > On 2015/01/21 10:28:52, perezju wrote: > > > Yeah, I also dislike the idea of adding this option. Because it helps > improve > > > the efficiency but only for this particular one caller. I would rather > prefer > > if > > > we find an alternative that could also bring benefits to other clients as > > well. > > > > > > On 2015/01/20 23:34:24, jbudorick wrote: > > > > This could probably be integrated into the existing cache, although > perhaps > > > the > > > > caching mechanism should be entirely moved into AdbWrapper. I'd at least > > > > recommend waiting until Juan chimes in, though. He's in EMEA, so it'll be > a > > > > little while. > > > > > > I don't think caching should be done by AdbWrapper, which should only know > how > > > to run adb commands, but not what those commands do (e.g. lifetimes of data > or > > > what is read-only). > > > > > > > My concern in this case is that AdbWrapper handles Uninstall directly. (I > > imagine there are similar cases.) > > > > > > I agree that that's out of scope of the bug. In face, this entire CL may > be > > > > getting out of the scope of the bug, as it's getting heavily into > > DeviceUtils > > > > performance tuning rather than things specific to content_browsertests. > > > > > > I like the idea of caching, but I'm not sure how easy will it be to get it > > > right. We need to check which other DeviceUtils methods deal with pm and > > should > > > invalidate the cache. Also we should probably do a quick check to see if > other > > > clients are calling 'pm' via RunShellCommand since we may want to have those > > > using calling the appropriate DeviceUtils' methods instead. > > > > > > > Agreed, it's tricky. > > > > > Alternatively, can we check whether the 'pm path' test is actually still > > needed? > > > I've traced back the CL where that change was introduced to [1], when not > > having > > > the check would break many bots. > > > > > > I've tested now, however, on a device on my desk and 'pm clear' seems to > work > > > fine even with non-existent packages (returns 'Failed' but doesn't hang as > the > > > comment suggests). Maybe this was fixed on recent Android versions? If so, > > maybe > > > we can just skip the test if the version is recent enough? Would this be > > enough > > > to reap the efficiency benefits without having to introduce much more > > > complexity? > > > > > > [1]: https://codereview.chromium.org/12210036/#ps20002 > > > > We'll have to test this out with older versions of android. > > If this last part _does_ work on versions starting with ICS, then that's > definitely the way to go. > > If not, we could also consider implementing a safe version of pm clear that we > install to the device (similar to what we do with unzip). I confirmed that pm clear didn't end when trying "pm clear xxx.xxx" on my test nexus S device with ICS mr1 build (IMM76S). I will prepare a patch to cache application paths.
PTAL. I've uploaded a new patch to cache application paths.
On 2015/01/21 21:54:24, Jaekyun Seok wrote: > On 2015/01/21 15:03:14, jbudorick wrote: > > > https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... > > File build/android/pylib/device/device_utils.py (right): > > > > > https://codereview.chromium.org/839893004/diff/20001/build/android/pylib/devi... > > build/android/pylib/device/device_utils.py:642: if (not package_check_needed) > or > > self.GetApplicationPath(package): > > On 2015/01/21 14:57:00, jbudorick wrote: > > > On 2015/01/21 10:28:52, perezju wrote: > > > > Yeah, I also dislike the idea of adding this option. Because it helps > > improve > > > > the efficiency but only for this particular one caller. I would rather > > prefer > > > if > > > > we find an alternative that could also bring benefits to other clients as > > > well. > > > > > > > > On 2015/01/20 23:34:24, jbudorick wrote: > > > > > This could probably be integrated into the existing cache, although > > perhaps > > > > the > > > > > caching mechanism should be entirely moved into AdbWrapper. I'd at least > > > > > recommend waiting until Juan chimes in, though. He's in EMEA, so it'll > be > > a > > > > > little while. > > > > > > > > I don't think caching should be done by AdbWrapper, which should only know > > how > > > > to run adb commands, but not what those commands do (e.g. lifetimes of > data > > or > > > > what is read-only). > > > > > > > > > > My concern in this case is that AdbWrapper handles Uninstall directly. (I > > > imagine there are similar cases.) > > > > > > > > I agree that that's out of scope of the bug. In face, this entire CL may > > be > > > > > getting out of the scope of the bug, as it's getting heavily into > > > DeviceUtils > > > > > performance tuning rather than things specific to content_browsertests. > > > > > > > > I like the idea of caching, but I'm not sure how easy will it be to get it > > > > right. We need to check which other DeviceUtils methods deal with pm and > > > should > > > > invalidate the cache. Also we should probably do a quick check to see if > > other > > > > clients are calling 'pm' via RunShellCommand since we may want to have > those > > > > using calling the appropriate DeviceUtils' methods instead. > > > > > > > > > > Agreed, it's tricky. > > > > > > > Alternatively, can we check whether the 'pm path' test is actually still > > > needed? > > > > I've traced back the CL where that change was introduced to [1], when not > > > having > > > > the check would break many bots. > > > > > > > > I've tested now, however, on a device on my desk and 'pm clear' seems to > > work > > > > fine even with non-existent packages (returns 'Failed' but doesn't hang as > > the > > > > comment suggests). Maybe this was fixed on recent Android versions? If so, > > > maybe > > > > we can just skip the test if the version is recent enough? Would this be > > > enough > > > > to reap the efficiency benefits without having to introduce much more > > > > complexity? > > > > > > > > [1]: https://codereview.chromium.org/12210036/#ps20002 > > > > > > We'll have to test this out with older versions of android. > > > > If this last part _does_ work on versions starting with ICS, then that's > > definitely the way to go. > > > > If not, we could also consider implementing a safe version of pm clear that we > > install to the device (similar to what we do with unzip). > > I confirmed that pm clear didn't end when trying "pm clear xxx.xxx" on my test > nexus S device with ICS mr1 build (IMM76S). > I misunderstood this when I initially read it. :( I'm going to review your current patch, but since we know this does work on some versions of Android, perhaps we could either: - hold this until we aren't running bots on versions of Android affected by this - tune DeviceUtils by os version s.t. we only do the check on android versions that have the bad pm implementation > I will prepare a patch to cache application paths.
@perezju: I suggested the cache in AdbWrapper (or in some way that involved AdbWrapper) because, as it stands, a client that runs device.adb.Uninstall will invalidate the cache. I'd rather explore some of our other options than continue pursuing the cache at this time. Apologies for leading you astray. https://codereview.chromium.org/839893004/diff/60001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/839893004/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:283: application_paths = self._cache['application_paths'] If we stay with the cache, please move this above the comment. It's explaining the lines that follow. https://codereview.chromium.org/839893004/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:287: should_check_return = (self.build_version_sdk < w.r.t. my tuning suggestion: we already have some version-dependent code here. https://codereview.chromium.org/839893004/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:474: if cmd.startswith('pm install') or cmd.startswith('pm uninstall'): O_O This looks _extremely_ dangerous / flaky / terrifying / high-maintenance / etc.
PTAL. I confirmed that the hanging problem of "pm clear" was fixed since JB MR2. So I've uploaded a patch to skip checking package on android versions which are JB MR2 or above.
lgtm IIRC, the devices attached to the CQ bots are on some build of KitKat, so we should see improvements similar to what your testing initially showed. Thanks for doing this, Jaekyun.
The CQ bit was checked by jaekyun@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/839893004/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/e3a24e9786ac3483c60e91c39742243aa15e7a14 Cr-Commit-Position: refs/heads/master@{#312552} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
