Purge unpinned ashmem before parsing /proc/$pid/smaps.

Purge unpinned ashmem before parsing /proc/$pid/smaps.

This will help stabilize memory measurements so that memory regressions can't
be hidden when unpinned ashmem gets purged by the kernel.

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

[Philippe, 2013-10-30 17:18:20]

https://codereview.chromium.org/52833002/diff/1/build/android/pylib/android_c...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/52833002/diff/1/build/android/pylib/android_c...
build/android/pylib/android_commands.py:1424: if constants.BuildTypeIsSet():
FYI, this condition is not always met which is why I wrote this function rather
than directly using constants.GetOutDirectory(). I'm making this function
private for now but I expect that other clients could make it public and use it
later.

[craigdh, 2013-10-31 00:10:12]

+marcus as I believe he requested the arbitrary build directories.

https://codereview.chromium.org/52833002/diff/1/build/android/pylib/android_c...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/52833002/diff/1/build/android/pylib/android_c...
build/android/pylib/android_commands.py:1424: if constants.BuildTypeIsSet():
On 2013/10/30 17:18:21, Philippe wrote:
> FYI, this condition is not always met which is why I wrote this function
rather
> than directly using constants.GetOutDirectory(). I'm making this function
> private for now but I expect that other clients could make it public and use
it
> later.

My understanding is that the goal is to support arbitrary out/* directories,
which is why all assumptions of Debug/Release were removed. To support an
arbitrary build type it must be passed in and set somewhere as there is no
reliable way to pick the right out directory. I'm opposed to magic searching for
common cases, explicit is better.

[Philippe, 2013-10-31 09:02:13]

On 2013/10/31 00:10:12, craigdh wrote:
> +marcus as I believe he requested the arbitrary build directories.
> 
>
https://codereview.chromium.org/52833002/diff/1/build/android/pylib/android_c...
> File build/android/pylib/android_commands.py (right):
> 
>
https://codereview.chromium.org/52833002/diff/1/build/android/pylib/android_c...
> build/android/pylib/android_commands.py:1424: if constants.BuildTypeIsSet():
> On 2013/10/30 17:18:21, Philippe wrote:
> > FYI, this condition is not always met which is why I wrote this function
> rather
> > than directly using constants.GetOutDirectory(). I'm making this function
> > private for now but I expect that other clients could make it public and use
> it
> > later.
> 
> My understanding is that the goal is to support arbitrary out/* directories,
> which is why all assumptions of Debug/Release were removed. To support an
> arbitrary build type it must be passed in and set somewhere as there is no
> reliable way to pick the right out directory. I'm opposed to magic searching
for
> common cases, explicit is better.

Thanks Craig! FYI, Marcus is OOO this week. Note that Telemetry already tries to
set the build type according to where it can find the various binaries (e.g.
md5sum). This new function only does magic searching as a fallback (although I
agree that we should explicitly set early the build type).

[tonyg, 2013-11-03 05:05:18]

https://codereview.chromium.org/52833002/diff/1/build/android/pylib/android_c...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/52833002/diff/1/build/android/pylib/android_c...
build/android/pylib/android_commands.py:1481: self._PurgeUnpinnedAshmem()
Telemetry calls GetMemoryUsageForPid, and before we introduce this dependency on
a new binary, we need to check it into cloud storage like the other binaries
required to run on android. We don't want to require building anything in order
to run Telemetry on Android.

See: SetupPrebuiltTools in adb_commands.py

We also need to update instructions on the Telemetry wiki and error messages.

[digit1, 2013-11-04 14:15:46]

lgtm (for the third_party/ashmem changes) :)

[digit1, 2013-11-04 14:15:55]

https://codereview.chromium.org/52833002/diff/1/build/android/pylib/android_c...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/52833002/diff/1/build/android/pylib/android_c...
build/android/pylib/android_commands.py:1445: build_type = 'Debug'
I'm ok with self-detection, but if you don't plan to remove this, please ensure
that there is a logging.warning() call here to notify the user. This will help
catch surprises later on.

[Philippe, 2013-11-04 14:45:54]

Thanks David!

Marcus and Tony: PTAL :)

https://codereview.chromium.org/52833002/diff/1/build/android/pylib/android_c...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/52833002/diff/1/build/android/pylib/android_c...
build/android/pylib/android_commands.py:1424: if constants.BuildTypeIsSet():
On 2013/10/31 00:10:13, craigdh wrote:
> On 2013/10/30 17:18:21, Philippe wrote:
> > FYI, this condition is not always met which is why I wrote this function
> rather
> > than directly using constants.GetOutDirectory(). I'm making this function
> > private for now but I expect that other clients could make it public and use
> it
> > later.
> 
> My understanding is that the goal is to support arbitrary out/* directories,
> which is why all assumptions of Debug/Release were removed. To support an
> arbitrary build type it must be passed in and set somewhere as there is no
> reliable way to pick the right out directory. I'm opposed to magic searching
for
> common cases, explicit is better.

I will let Marcus comment on this :)

https://codereview.chromium.org/52833002/diff/1/build/android/pylib/android_c...
build/android/pylib/android_commands.py:1445: build_type = 'Debug'
On 2013/11/04 14:15:55, digit1 wrote:
> I'm ok with self-detection, but if you don't plan to remove this, please
ensure
> that there is a logging.warning() call here to notify the user. This will help
> catch surprises later on.

Yeah good idea. We got bitten more than once with this :)

https://codereview.chromium.org/52833002/diff/1/build/android/pylib/android_c...
build/android/pylib/android_commands.py:1481: self._PurgeUnpinnedAshmem()
On 2013/11/03 05:05:18, tonyg wrote:
> Telemetry calls GetMemoryUsageForPid, and before we introduce this dependency
on
> a new binary, we need to check it into cloud storage like the other binaries
> required to run on android. We don't want to require building anything in
order
> to run Telemetry on Android.
> 
> See: SetupPrebuiltTools in adb_commands.py
> 
> We also need to update instructions on the Telemetry wiki and error messages.

Thanks for the instructions Tony. I have just pushed the 'purge_ashmem' ARM
release binary to cloud storage (since it looked good to David). I added the
corresponding SHA1 file and updated SetupPrebuildTools(). I haven't seen any
specific documentation that I should update?

[frankf, 2013-11-04 18:17:05]

https://codereview.chromium.org/52833002/diff/1/build/android/pylib/android_c...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/52833002/diff/1/build/android/pylib/android_c...
build/android/pylib/android_commands.py:1413: def
_GetExistingHostOutputPath(file_name):
If you decide to keep this, it should be moved to a utility module in
pylib/utils since it's not related to adb commands.

[bulach, 2013-11-04 20:22:06]

thanks pliard! some general comments below..

https://codereview.chromium.org/52833002/diff/90001/build/android/pylib/andro...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/52833002/diff/90001/build/android/pylib/andro...
build/android/pylib/android_commands.py:1413: def
_GetExistingHostOutputPath(file_name):
perhaps we should make the following a public API?

https://codereview.chromium.org/55923003/diff/1/third_party/android_platform/...

see TranslateLibPath, it essentially gets all possible file names, then filters
with os.path.exists and sorts with os.path.getmtime.. I think cjhopman wrote
that, pretty clever and compact.. :)

regardless, I don't think this should set yet-another-cache of
_detected_build_type / SetBuildType though, this gets hairy too quickly...
perhaps just GetMostRecentHostBinary(file_name) would be enough? and agree with
frank, move it out into its own module..

https://codereview.chromium.org/52833002/diff/90001/tools/android/purge_ashme...
File tools/android/purge_ashmem/purge_ashmem.c (right):

https://codereview.chromium.org/52833002/diff/90001/tools/android/purge_ashme...
tools/android/purge_ashmem/purge_ashmem.c:9: #include
"../../../third_party/ashmem/ashmem.h"
I think the common way is to -I../../.. so this becomes #include
"third_party/ashmem/ashmem.h" ?

[Philippe, 2013-11-05 09:22:13]

Thanks guys!

https://codereview.chromium.org/52833002/diff/90001/build/android/pylib/andro...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/52833002/diff/90001/build/android/pylib/andro...
build/android/pylib/android_commands.py:1413: def
_GetExistingHostOutputPath(file_name):
On 2013/11/04 20:22:06, bulach wrote:
> perhaps we should make the following a public API?
> 
>
https://codereview.chromium.org/55923003/diff/1/third_party/android_platform/...
> 
> see TranslateLibPath, it essentially gets all possible file names, then
filters
> with os.path.exists and sorts with os.path.getmtime.. I think cjhopman wrote
> that, pretty clever and compact.. :)
> 
> regardless, I don't think this should set yet-another-cache of
> _detected_build_type / SetBuildType though, this gets hairy too quickly...
> perhaps just GetMostRecentHostBinary(file_name) would be enough? and agree
with
> frank, move it out into its own module..

Agreed. I moved this to utils/host_path_finder.py. Nice function indeed. I like
the fact that there is no branch in it (especially true in Python :)).

https://codereview.chromium.org/52833002/diff/90001/tools/android/purge_ashme...
File tools/android/purge_ashmem/purge_ashmem.c (right):

https://codereview.chromium.org/52833002/diff/90001/tools/android/purge_ashme...
tools/android/purge_ashmem/purge_ashmem.c:9: #include
"../../../third_party/ashmem/ashmem.h"
On 2013/11/04 20:22:06, bulach wrote:
> I think the common way is to -I../../.. so this becomes #include
> "third_party/ashmem/ashmem.h" ?

Done.

[bulach, 2013-11-05 16:25:14]

lgtm, thanks! I have one suggestion below:

https://codereview.chromium.org/52833002/diff/280001/build/android/pylib/andr...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/52833002/diff/280001/build/android/pylib/andr...
build/android/pylib/android_commands.py:1415: def _PurgeUnpinnedAshmem(self):
probably best to make this public and let callers decide whether or not they
want to purge, wdyt?
(and it can also be done just once rather than per pid)

[Philippe, 2013-11-05 16:37:41]

Thanks Marcus! I'm now waiting for Frank and Tony.

https://codereview.chromium.org/52833002/diff/280001/build/android/pylib/andr...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/52833002/diff/280001/build/android/pylib/andr...
build/android/pylib/android_commands.py:1415: def _PurgeUnpinnedAshmem(self):
On 2013/11/05 16:25:14, bulach wrote:
> probably best to make this public and let callers decide whether or not they
> want to purge, wdyt?
> (and it can also be done just once rather than per pid)

Yeah, good idea. I don't think many clients will need to have ashmem purged so
it's probably ok if we let the client deal with this detail (which he will
certainly miss but again clients other than Telemetry should not have to worry
too much about this).
I did not find an easy/not too hackish way for doing it only once though.

[frankf, 2013-11-05 19:22:56]

https://codereview.chromium.org/52833002/diff/330001/build/android/pylib/andr...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/52833002/diff/330001/build/android/pylib/andr...
build/android/pylib/android_commands.py:1426: logging.error('Could not purge
ashmem. Measurement might be unstable.')
Generally, we're moving to make these methods more pythonic which means throwing
an exception that the caller can handle however they wish instead of just
logging error which might go unnoticed and lost in the noise.

[Philippe, 2013-11-06 16:51:08]

Thanks Frank!

https://chromiumcodereview.appspot.com/52833002/diff/330001/build/android/pyl...
File build/android/pylib/android_commands.py (right):

https://chromiumcodereview.appspot.com/52833002/diff/330001/build/android/pyl...
build/android/pylib/android_commands.py:1426: logging.error('Could not purge
ashmem. Measurement might be unstable.')
On 2013/11/05 19:22:56, frankf wrote:
> Generally, we're moving to make these methods more pythonic which means
throwing
> an exception that the caller can handle however they wish instead of just
> logging error which might go unnoticed and lost in the noise.

Done.

[Philippe, 2013-11-06 18:34:39]

On 2013/11/06 16:51:08, Philippe wrote:
> Thanks Frank!
> 
>
https://chromiumcodereview.appspot.com/52833002/diff/330001/build/android/pyl...
> File build/android/pylib/android_commands.py (right):
> 
>
https://chromiumcodereview.appspot.com/52833002/diff/330001/build/android/pyl...
> build/android/pylib/android_commands.py:1426: logging.error('Could not purge
> ashmem. Measurement might be unstable.')
> On 2013/11/05 19:22:56, frankf wrote:
> > Generally, we're moving to make these methods more pythonic which means
> throwing
> > an exception that the caller can handle however they wish instead of just
> > logging error which might go unnoticed and lost in the noise.
> 
> Done.

CC'ing Grace.

[frankf, 2013-11-06 19:04:52]

lgtm, Thanks.

[tonyg, 2013-11-06 19:21:22]

lgtm

But a could we also update a couple of other places in this patch.

https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

s|ninja -C out/Release forwarder2 md5sum|ninja -C out/Release android_tools|

https://code.google.com/p/chromium/codesearch#chromium/src/tools/bisect-perf-...

s|targets = ['chromium_testshell', 'cc_perftests_apk', 'forwarder2',
'md5sum']|targets = ['chromium_testshell', 'cc_perftests_apk', 'android_tools']|

Finally, here:
http://www.chromium.org/developers/telemetry/run_locally

[Philippe, 2013-11-13 17:26:31]

On 2013/11/06 19:21:22, tonyg wrote:
> lgtm
> 
> But a could we also update a couple of other places in this patch.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
> 
> s|ninja -C out/Release forwarder2 md5sum|ninja -C out/Release android_tools|
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/bisect-perf-...
> 
> s|targets = ['chromium_testshell', 'cc_perftests_apk', 'forwarder2',
> 'md5sum']|targets = ['chromium_testshell', 'cc_perftests_apk',
'android_tools']|
> 
> Finally, here:
> http://www.chromium.org/developers/telemetry/run_locally

Thanks Tony! I have just updated the places you pointed out (+ rebased). I had
indeed missed the fact that we were not using the android_tools rule.

[commit-bot: I haz the power, 2013-11-13 17:26:56]

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

[commit-bot: I haz the power, 2013-11-13 17:42:06]

Retried try job too often on linux_rel for step(s) cacheinvalidation_unittests,
cc_unittests, check_deps, chromedriver2_unittests, components_unittests,
content_browsertests, content_unittests, crypto_unittests, gpu_unittests,
ipc_tests, jingle_unittests, media_unittests, nacl_integration, ppapi_unittests,
printing_unittests, remoting_unittests, sandbox_linux_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-14 10:12:41]

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

[commit-bot: I haz the power, 2013-11-14 11:00:32]

Change committed as 235115