Add instance-level in-memory cache for PushIfNeeded().

Add instance-level in-memory cache for PushIfNeeded().

This speeds up PushIfNeeded() execution times on a same instance of
AndroidCommands for repetitive files. This can be achieved by mapping host file
paths to their mtime at push time as tonyg@ pointed out although this assumes
that the device doesn't modify the files.

This is needed to make PurgeUnpinnedAshmem() reasonably fast to execute in
Telemetry. This CL decreases by 40 secs the execution time of netsim.top_10
whem memory is measured for each page (and PurgeUnpinnedAshmem() is called
before each measurement).

BUG=323494, 326929
R=bulach@chromium.org, craigdh@chromium.org, tonyg@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239757

[bulach, 2013-11-29 16:28:41]

lgtm once the bots (and frank/craig) are happy, thanks!!

the long term fix is to keep this in some long-term service so it'd be
persistent per "build cycle".

[tonyg, 2013-11-29 16:54:26]

lgtm

[Philippe, 2013-12-02 09:04:45]

On 2013/11/29 16:54:26, tonyg wrote:
> lgtm

Thanks guys! I'm now waiting for Frank/Craig.

[craigdh, 2013-12-02 17:09:35]

https://chromiumcodereview.appspot.com/97133002/diff/1/build/android/pylib/an...
File build/android/pylib/android_commands.py (right):

https://chromiumcodereview.appspot.com/97133002/diff/1/build/android/pylib/an...
build/android/pylib/android_commands.py:924: if host_path in
self._push_if_needed_cache:
What about when |host_path| is a directory? Updating the mtime of a file doesn't
update the mtime of all parent directories.

Seems like you'd have to cache the mtimes of all files in the directory (and
subdirectories recursively) on line 953 and subsequently check them all here.

[Philippe, 2013-12-02 17:16:43]

https://chromiumcodereview.appspot.com/97133002/diff/1/build/android/pylib/an...
File build/android/pylib/android_commands.py (right):

https://chromiumcodereview.appspot.com/97133002/diff/1/build/android/pylib/an...
build/android/pylib/android_commands.py:924: if host_path in
self._push_if_needed_cache:
On 2013/12/02 17:09:36, craigdh wrote:
> What about when |host_path| is a directory? Updating the mtime of a file
doesn't
> update the mtime of all parent directories.
> 
> Seems like you'd have to cache the mtimes of all files in the directory (and
> subdirectories recursively) on line 953 and subsequently check them all here.

Good catch :) I had forgotten that |host_path| could be a directory.

How about not caching directories instead (at least as a preliminary step)?

[craigdh, 2013-12-02 18:20:19]

https://chromiumcodereview.appspot.com/97133002/diff/1/build/android/pylib/an...
File build/android/pylib/android_commands.py (right):

https://chromiumcodereview.appspot.com/97133002/diff/1/build/android/pylib/an...
build/android/pylib/android_commands.py:924: if host_path in
self._push_if_needed_cache:
On 2013/12/02 17:16:43, Philippe wrote:
> On 2013/12/02 17:09:36, craigdh wrote:
> > What about when |host_path| is a directory? Updating the mtime of a file
> doesn't
> > update the mtime of all parent directories.
> > 
> > Seems like you'd have to cache the mtimes of all files in the directory (and
> > subdirectories recursively) on line 953 and subsequently check them all
here.
> 
> Good catch :) I had forgotten that |host_path| could be a directory.
> 
> How about not caching directories instead (at least as a preliminary step)?

It should be pretty easy to do with os.walk(). Do you mind trying and seeing if
the performance is reasonable?

[Philippe, 2013-12-03 10:31:31]

Thanks Craig! Please see my comment below.

https://chromiumcodereview.appspot.com/97133002/diff/1/build/android/pylib/an...
File build/android/pylib/android_commands.py (right):

https://chromiumcodereview.appspot.com/97133002/diff/1/build/android/pylib/an...
build/android/pylib/android_commands.py:924: if host_path in
self._push_if_needed_cache:
On 2013/12/02 18:20:19, craigdh wrote:
> On 2013/12/02 17:16:43, Philippe wrote:
> > On 2013/12/02 17:09:36, craigdh wrote:
> > > What about when |host_path| is a directory? Updating the mtime of a file
> > doesn't
> > > update the mtime of all parent directories.
> > > 
> > > Seems like you'd have to cache the mtimes of all files in the directory
(and
> > > subdirectories recursively) on line 953 and subsequently check them all
> here.
> > 
> > Good catch :) I had forgotten that |host_path| could be a directory.
> > 
> > How about not caching directories instead (at least as a preliminary step)?
> 
> It should be pretty easy to do with os.walk(). Do you mind trying and seeing
if
> the performance is reasonable?

My main concern was precisely that I have no immediate way to measure this with
directories. I wrote this change to speedup the execution of the memory
measurements in Telemetry (since I'm working on a CL that causes a significant
slowdown partly due to this specific PushIfNeeded() issue). PushIfNeeded() is
only used repetitively in that context to push the |purge_ashmem| binary. So
this sounded slightly like premature optimization to me and wasn't worth the
complexity IMO given the few edge cases that would have to be handled correctly
(e.g. |host_path|, when it's a directory, has now less children than it had when
the initial push happened or |host_path| was initially a directory but it's now
a file). I would prefer simplicity and correctness until we have good data :)
Now if you feel strongly about this I'm also happy to implement it in a next CL
since I would like first to move forward on memory measurements.

What do you think? I would be happy to file a bug tracking those future
improvements (and possibly assign it to myself although you guys are more
familiar/involved than me with this layer).

[Philippe, 2013-12-05 10:00:46]

Ping? :)


On Tue, Dec 3, 2013 at 11:31 AM, <pliard@chromium.org> wrote:

> Thanks Craig! Please see my comment below.
>
>
>
> https://chromiumcodereview.appspot.com/97133002/diff/1/
> build/android/pylib/android_commands.py
> File build/android/pylib/android_commands.py (right):
>
> https://chromiumcodereview.appspot.com/97133002/diff/1/
> build/android/pylib/android_commands.py#newcode924
> build/android/pylib/android_commands.py:924: if host_path in
> self._push_if_needed_cache:
> On 2013/12/02 18:20:19, craigdh wrote:
>
>> On 2013/12/02 17:16:43, Philippe wrote:
>> > On 2013/12/02 17:09:36, craigdh wrote:
>> > > What about when |host_path| is a directory? Updating the mtime of
>>
> a file
>
>> > doesn't
>> > > update the mtime of all parent directories.
>> > >
>> > > Seems like you'd have to cache the mtimes of all files in the
>>
> directory (and
>
>> > > subdirectories recursively) on line 953 and subsequently check
>>
> them all
>
>> here.
>> >
>> > Good catch :) I had forgotten that |host_path| could be a directory.
>> >
>> > How about not caching directories instead (at least as a preliminary
>>
> step)?
>
>  It should be pretty easy to do with os.walk(). Do you mind trying and
>>
> seeing if
>
>> the performance is reasonable?
>>
>
> My main concern was precisely that I have no immediate way to measure
> this with directories. I wrote this change to speedup the execution of
> the memory measurements in Telemetry (since I'm working on a CL that
> causes a significant slowdown partly due to this specific PushIfNeeded()
> issue). PushIfNeeded() is only used repetitively in that context to push
> the |purge_ashmem| binary. So this sounded slightly like premature
> optimization to me and wasn't worth the complexity IMO given the few
> edge cases that would have to be handled correctly (e.g. |host_path|,
> when it's a directory, has now less children than it had when the
> initial push happened or |host_path| was initially a directory but it's
> now a file). I would prefer simplicity and correctness until we have
> good data :) Now if you feel strongly about this I'm also happy to
> implement it in a next CL since I would like first to move forward on
> memory measurements.
>
> What do you think? I would be happy to file a bug tracking those future
> improvements (and possibly assign it to myself although you guys are
> more familiar/involved than me with this layer).
>
> https://chromiumcodereview.appspot.com/97133002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[craigdh, 2013-12-05 20:07:51]

> > What do you think? I would be happy to file a bug tracking those future
> > improvements (and possibly assign it to myself although you guys are
> > more familiar/involved than me with this layer).

Seems reasonable to add a TODO with a bug, correctness is definitely preferable
with PushIfNeeded, bugs here are hard to track down. lgtm with one suggestion.

[Philippe, 2013-12-09 09:30:52]

On 2013/12/05 20:07:51, craigdh wrote:
> > > What do you think? I would be happy to file a bug tracking those future
> > > improvements (and possibly assign it to myself although you guys are
> > > more familiar/involved than me with this layer).
> 
> Seems reasonable to add a TODO with a bug, correctness is definitely
preferable
> with PushIfNeeded, bugs here are hard to track down. lgtm with one suggestion.

Thanks Craig! I filed a bug and assigned it to you. I don't see your suggestion?
:)

[Philippe, 2013-12-10 10:31:01]

On 2013/12/09 09:30:52, Philippe wrote:
> On 2013/12/05 20:07:51, craigdh wrote:
> > > > What do you think? I would be happy to file a bug tracking those future
> > > > improvements (and possibly assign it to myself although you guys are
> > > > more familiar/involved than me with this layer).
> > 
> > Seems reasonable to add a TODO with a bug, correctness is definitely
> preferable
> > with PushIfNeeded, bugs here are hard to track down. lgtm with one
suggestion.
> 
> Thanks Craig! I filed a bug and assigned it to you. I don't see your
suggestion?
> :)

JFYI, I'm landing :) I will address your suggestion in a follow up CL.

[commit-bot: I haz the power, 2013-12-10 10:31:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/97133002/20001

[commit-bot: I haz the power, 2013-12-10 10:31:31]

Failed to apply patch for build/android/pylib/android_commands.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file build/android/pylib/android_commands.py
  Hunk #1 FAILED at 253.
  Hunk #2 succeeded at 965 (offset 48 lines).
  Hunk #3 succeeded at 998 (offset 48 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
build/android/pylib/android_commands.py.rej

Patch:       build/android/pylib/android_commands.py
Index: build/android/pylib/android_commands.py
diff --git a/build/android/pylib/android_commands.py
b/build/android/pylib/android_commands.py
index
8d8949cac74c8db343ff68b5c9a33da11d462bf8..d17a3f856fee5d57f0f08dc58b1a7ac83c144625
100644
--- a/build/android/pylib/android_commands.py
+++ b/build/android/pylib/android_commands.py
@@ -253,6 +253,7 @@ class AndroidCommands(object):
     self._actual_push_size = 0
     self._external_storage = ''
     self._util_wrapper = ''
+    self._push_if_needed_cache = {}
 
   def _LogShell(self, cmd):
     """Logs the adb shell command."""
@@ -917,6 +918,15 @@ class AndroidCommands(object):
     MAX_INDIVIDUAL_PUSHES = 50
     assert os.path.exists(host_path), 'Local path not found %s' % host_path
 
+    # See if the file on the host changed since the last push (if any) and
+    # return early if it didn't. Note that this shortcut assumes that the tests
+    # on the device don't modify the files.
+    if not os.path.isdir(host_path):
+      if host_path in self._push_if_needed_cache:
+        host_path_mtime = self._push_if_needed_cache[host_path]
+        if host_path_mtime == os.stat(host_path).st_mtime:
+          return
+
     def GetHostSize(path):
       return int(cmd_helper.GetCmdOutput(['du', '-sb', path]).split()[0])
 
@@ -941,6 +951,8 @@ class AndroidCommands(object):
       while True:
         output = self._adb.SendCommand(push_command, timeout_time=30 * 60)
         if _HasAdbPushSucceeded(output):
+          if not os.path.isdir(host_path):
+            self._push_if_needed_cache[host] = os.stat(host).st_mtime
           return
         if retry < 3:
           retry += 1

[Philippe, 2013-12-10 11:27:54]

Committed patchset #3 manually as r239757 (presubmit successful).

[craigdh, 2013-12-10 19:23:31]

Whoops, forgot to publish my draft comments.

https://chromiumcodereview.appspot.com/97133002/diff/20001/build/android/pyli...
File build/android/pylib/android_commands.py (right):

https://chromiumcodereview.appspot.com/97133002/diff/20001/build/android/pyli...
build/android/pylib/android_commands.py:954: if not os.path.isdir(host_path):
If we're only doing this for single files, then instead of doing this in the
loop (calling os.path.isdir for every file when we're pushing large directories)
just stick it at the end of PushIfNeeded (line 980). An exception is raised if
the push fails so this will only be cached if the file is successfully pushed.