Make page_cycler.py fully measure memory for each page.

Make page_cycler.py fully measure memory for each page.

Telemetry makes the distinction between metrics collected for each page and for
each page set. The fine-grained memory metrics were collected only per page set
(although there was some histogram data collected per page).

This CL makes the memory metric module systematically report the whole data per
page (and not only per page set) so that we can measure pages independently.
The per page set memory metric collection is now a no-op.

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

[Philippe, 2013-11-27 17:08:39]

I'm not fluent in Telemetry so please review this carefully :)

[tonyg, 2013-11-27 17:59:58]

lg2m, but quinten should probably approve.

Thank you for making this better :)

https://chromiumcodereview.appspot.com/91573003/diff/1/tools/perf/measurement...
File tools/perf/measurements/page_cycler.py (left):

https://chromiumcodereview.appspot.com/91573003/diff/1/tools/perf/measurement...
tools/perf/measurements/page_cycler.py:179:
self._memory_metric.AddSummaryResults(results)
You'll need to make the same change in media.py, memory.py, memory_multi_tab.py
and page_cycler_unittest.py.

Also, please make sure this is happy:
$ tools/perf/run_tests

https://chromiumcodereview.appspot.com/91573003/diff/1/tools/perf/metrics/mem...
File tools/perf/metrics/memory.py (right):

https://chromiumcodereview.appspot.com/91573003/diff/1/tools/perf/metrics/mem...
tools/perf/metrics/memory.py:148: def AddSummaryResults(self, results,
trace_name=None):
I think this can just be omitted, right?

[qyearsley, 2013-11-27 19:01:04]

As Tony mentioned, you'll also want to make the same change to the memory,
memory_multi_tab and media measurements. If, however, we wanted to keep the same
behavior for these other three measurments and just change page cycler, we might
want to provide both a AddResults and AddSummaryResults in metrics/memory.py,
and try to factor out the common logic that is re-used in both.

In addition, you should watch out for the first line of what was
"AddSummaryResults", i.e.:
  self._memory_stats = self._browser.memory_stats
Browser.memory_stats is actually a method call (see
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...)
and it could a significant amount of time. It's probably worth timing the run
time of page cycler on some simple page set before and after this change.

[qyearsley, 2013-11-27 19:01:55]

https://chromiumcodereview.appspot.com/91573003/diff/1/tools/perf/metrics/mem...
File tools/perf/metrics/memory.py (right):

https://chromiumcodereview.appspot.com/91573003/diff/1/tools/perf/metrics/mem...
tools/perf/metrics/memory.py:148: def AddSummaryResults(self, results,
trace_name=None):
On 2013/11/27 17:59:58, tonyg wrote:
> I think this can just be omitted, right?

In general, Metrics don't need to have a method AddSummaryResults, since it
isn't in the Metric base class right now (although it could be put in). Once the
call to this method is removed from the memory, memory_multi_tab and media
measurements, this can be removed.

[Philippe, 2013-11-28 10:30:13]

Thanks guys!

Marcus, I will need you to take a look at android_commands.py :)

Quinten had a good intuition regarding execution times.

page_cycler.netsim.top_10 takes 7m2s on my machine/device without my change,
10m17s with patch set 1 and 9m32s with patch set 2.

I believe the bottleneck is the amount of round trips that we do with the
device/adb to execute various commands (for each PID). One change that could
significantly decrease this delta I believe would be to make
GetMemoryUsageForPid able to operate on a list of PIDs (browser process +
renderer process). This would require some changes in Telemetry too I believe.

What do you guys think? Can we track this optimization for later/soon and move
forward on memory measurements?

https://chromiumcodereview.appspot.com/91573003/diff/1/tools/perf/metrics/mem...
File tools/perf/metrics/memory.py (right):

https://chromiumcodereview.appspot.com/91573003/diff/1/tools/perf/metrics/mem...
tools/perf/metrics/memory.py:148: def AddSummaryResults(self, results,
trace_name=None):
On 2013/11/27 19:01:55, qyearsley wrote:
> On 2013/11/27 17:59:58, tonyg wrote:
> > I think this can just be omitted, right?
> 
> In general, Metrics don't need to have a method AddSummaryResults, since it
> isn't in the Metric base class right now (although it could be put in). Once
the
> call to this method is removed from the memory, memory_multi_tab and media
> measurements, this can be removed.

Yeah, I removed this method.

[bulach, 2013-11-28 13:31:23]

thanks pliard! one suggestion below, let me know your thoughts:

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

https://codereview.chromium.org/91573003/diff/20001/build/android/pylib/andro...
build/android/pylib/android_commands.py:1449: def PurgeUnpinnedAshmem(self):
I think both _PushPurgeAshmemBinaryIfNeeded and PurgeUnpinnedAshmem would be
better suited higher up in android_platform_backend.py, wdyt?

craig is undergoing a big effort to trim this down and have a slightly saner API
at this level, so we could help him by moving this up to the only place where
it's needed..

[Philippe, 2013-11-28 17:03:04]

Thanks Marcus!

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

https://codereview.chromium.org/91573003/diff/20001/build/android/pylib/andro...
build/android/pylib/android_commands.py:1449: def PurgeUnpinnedAshmem(self):
On 2013/11/28 13:31:24, bulach wrote:
> I think both _PushPurgeAshmemBinaryIfNeeded and PurgeUnpinnedAshmem would be
> better suited higher up in android_platform_backend.py, wdyt?
> 
> craig is undergoing a big effort to trim this down and have a slightly saner
API
> at this level, so we could help him by moving this up to the only place where
> it's needed..

Yeah, agreed :)

[bulach, 2013-11-28 17:18:01]

lgtm, thanks! please let qyearsley have the final word :)

[tonyg, 2013-11-28 17:31:04]

The most recent version raises some more issues I'd like to address before
landing this. It is possible that the best way to address some of them is in a
separate CL prior to this one.

https://codereview.chromium.org/91573003/diff/40001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/91573003/diff/40001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:19: from
pylib import constants  # pylint: disable=F0401
This is out of order

Also, just a reminder these imports happen for all platforms, so make sure the
cq is happy.

https://codereview.chromium.org/91573003/diff/40001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:115: if not
self._purge_ashmem_binary_was_pushed:
This isn't the ideal level for this caching. I've been thinking more lately
about how to make android_commands.py talk to the device as infrequently as
possible in order to improve cycle time. PushIfNeeded() is a good opportunity.

Let's modify PushIfNeeded to store a cache of the {host_path: mtime} of the host
binary each time it computes the md5sum. Then if the host binary's mtime is
unchanged, shortcut the remainder of the checks.

This will prevent the caching hack from proliferating throughout the codebase.

https://codereview.chromium.org/91573003/diff/40001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:120:
self._adb.PushIfNeeded(host_path, device_path)
host_path_finder and PushIfNeeded should not be used directly in Telemetry this
way.

Please use android_prebuilt_profiler_helper:InstallOnDevice()

In another CL, we should move that out of the profiler directory.

https://codereview.chromium.org/91573003/diff/40001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:135:
self._PurgeUnpinnedAshmem()
Since this is system-wide, I don't think it belongs in a pid-specific method.
Seems like we should have a platform.PurgeUnpinnedMemory() method (with an
ashmem backing on android and a TODO on mac). Then browser.memory_stats would
only call it a single time before looping through all pids instead of the
current behavior of calling it before every pid.

[bulach, 2013-11-28 17:49:24]

+frankf / craigdh

https://codereview.chromium.org/91573003/diff/40001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/91573003/diff/40001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:115: if not
self._purge_ashmem_binary_was_pushed:
On 2013/11/28 17:31:05, tonyg wrote:
> This isn't the ideal level for this caching. I've been thinking more lately
> about how to make android_commands.py talk to the device as infrequently as
> possible in order to improve cycle time. PushIfNeeded() is a good opportunity.
> 
> Let's modify PushIfNeeded to store a cache of the {host_path: mtime} of the
host
> binary each time it computes the md5sum. Then if the host binary's mtime is
> unchanged, shortcut the remainder of the checks.
> 
> This will prevent the caching hack from proliferating throughout the codebase.


this is a great idea!!
there's some separate discussions about how to speed up our test cycles, so I'm
adding frankf and craig here..

note that changing just android_commands isn't enough either, since there'll be
multiple instances, one per test...

I think what we need is to decouple into a "Pusher" / "Sync" long-running
service that would keep this dict for a longer term (i.e., duration of a build
cycle, or individual test if not being ran by buildbot...).

[Philippe, 2013-11-29 13:26:20]

Yeah, great idea! In terms of design, there is also another approach that
would have less impact on clients: have a global thread-safe map in
android_commands.py. I'm not a huge fan of this but at least the change
would be quite isolated. Also the long-running service would have to be
thread-safe anyway Marcus, right? What do you think?


On Thu, Nov 28, 2013 at 6:49 PM, <bulach@chromium.org> wrote:

> +frankf / craigdh
>
>
>
> https://codereview.chromium.org/91573003/diff/40001/tools/
> telemetry/telemetry/core/platform/android_platform_backend.py
> File tools/telemetry/telemetry/core/platform/android_platform_backend.py
> (right):
>
> https://codereview.chromium.org/91573003/diff/40001/tools/
> telemetry/telemetry/core/platform/android_platform_backend.py#newcode115
> tools/telemetry/telemetry/core/platform/android_platform_backend.py:115:
> if not self._purge_ashmem_binary_was_pushed:
> On 2013/11/28 17:31:05, tonyg wrote:
>
>> This isn't the ideal level for this caching. I've been thinking more
>>
> lately
>
>> about how to make android_commands.py talk to the device as
>>
> infrequently as
>
>> possible in order to improve cycle time. PushIfNeeded() is a good
>>
> opportunity.
>
>  Let's modify PushIfNeeded to store a cache of the {host_path: mtime}
>>
> of the host
>
>> binary each time it computes the md5sum. Then if the host binary's
>>
> mtime is
>
>> unchanged, shortcut the remainder of the checks.
>>
>
>  This will prevent the caching hack from proliferating throughout the
>>
> codebase.
>
> this is a great idea!!
> there's some separate discussions about how to speed up our test cycles,
> so I'm adding frankf and craig here..
>
> note that changing just android_commands isn't enough either, since
> there'll be multiple instances, one per test...
>
> I think what we need is to decouple into a "Pusher" / "Sync"
> long-running service that would keep this dict for a longer term (i.e.,
> duration of a build cycle, or individual test if not being ran by
> buildbot...).
>
> https://codereview.chromium.org/91573003/
>

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

[bulach, 2013-11-29 13:41:52]

yeah, I was thinking that the existing android_commands.py itself wouldn't
change, but then the impl would call into this service.. it'll have the be
thread safe, yes. :)

having a global map in android_commands.py will be ok as a stop-gap, but it
wouldn't solve the problem for buildbots, since each test would have its own
android_commands instance...

regardless of the approach, one potential problem: we'd be changing the
semantics slightly.. right now, if a test writes to the file, we'd re-push it
later on.. by keeping this cache, there's a potential breakage since the files
will never be re-pushed... I hope we're not doing this, but just in case,
something to keep an eye on...

To be clear: I'm happy with this patch, happier with the cache in
android_commands, and happiest with the service... I think all of them will be
incremental improvements anyways!
I'll leave to tony and others to decide how to best get there.

On 2013/11/29 13:26:20, Philippe wrote:
> Yeah, great idea! In terms of design, there is also another approach that
> would have less impact on clients: have a global thread-safe map in
> android_commands.py. I'm not a huge fan of this but at least the change
> would be quite isolated. Also the long-running service would have to be
> thread-safe anyway Marcus, right? What do you think?
> 
> 
> On Thu, Nov 28, 2013 at 6:49 PM, <mailto:bulach@chromium.org> wrote:
> 
> > +frankf / craigdh
> >
> >
> >
> > https://codereview.chromium.org/91573003/diff/40001/tools/
> > telemetry/telemetry/core/platform/android_platform_backend.py
> > File tools/telemetry/telemetry/core/platform/android_platform_backend.py
> > (right):
> >
> > https://codereview.chromium.org/91573003/diff/40001/tools/
> > telemetry/telemetry/core/platform/android_platform_backend.py#newcode115
> > tools/telemetry/telemetry/core/platform/android_platform_backend.py:115:
> > if not self._purge_ashmem_binary_was_pushed:
> > On 2013/11/28 17:31:05, tonyg wrote:
> >
> >> This isn't the ideal level for this caching. I've been thinking more
> >>
> > lately
> >
> >> about how to make android_commands.py talk to the device as
> >>
> > infrequently as
> >
> >> possible in order to improve cycle time. PushIfNeeded() is a good
> >>
> > opportunity.
> >
> >  Let's modify PushIfNeeded to store a cache of the {host_path: mtime}
> >>
> > of the host
> >
> >> binary each time it computes the md5sum. Then if the host binary's
> >>
> > mtime is
> >
> >> unchanged, shortcut the remainder of the checks.
> >>
> >
> >  This will prevent the caching hack from proliferating throughout the
> >>
> > codebase.
> >
> > this is a great idea!!
> > there's some separate discussions about how to speed up our test cycles,
> > so I'm adding frankf and craig here..
> >
> > note that changing just android_commands isn't enough either, since
> > there'll be multiple instances, one per test...
> >
> > I think what we need is to decouple into a "Pusher" / "Sync"
> > long-running service that would keep this dict for a longer term (i.e.,
> > duration of a build cycle, or individual test if not being ran by
> > buildbot...).
> >
> > https://codereview.chromium.org/91573003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Philippe, 2013-11-29 13:47:24]

Ok, thanks Marcus! Agreed, let's iterate :) I will look into adding an
android_commands instance-level cache and I will see if anything breaks.


On Fri, Nov 29, 2013 at 2:41 PM, <bulach@chromium.org> wrote:

> yeah, I was thinking that the existing android_commands.py itself wouldn't
> change, but then the impl would call into this service.. it'll have the be
> thread safe, yes. :)
>
> having a global map in android_commands.py will be ok as a stop-gap, but it
> wouldn't solve the problem for buildbots, since each test would have its
> own
> android_commands instance...
>
> regardless of the approach, one potential problem: we'd be changing the
> semantics slightly.. right now, if a test writes to the file, we'd re-push
> it
> later on.. by keeping this cache, there's a potential breakage since the
> files
> will never be re-pushed... I hope we're not doing this, but just in case,
> something to keep an eye on...
>
> To be clear: I'm happy with this patch, happier with the cache in
> android_commands, and happiest with the service... I think all of them
> will be
> incremental improvements anyways!
> I'll leave to tony and others to decide how to best get there.
>
>
> On 2013/11/29 13:26:20, Philippe wrote:
>
>> Yeah, great idea! In terms of design, there is also another approach that
>> would have less impact on clients: have a global thread-safe map in
>> android_commands.py. I'm not a huge fan of this but at least the change
>> would be quite isolated. Also the long-running service would have to be
>> thread-safe anyway Marcus, right? What do you think?
>>
>
>
>  On Thu, Nov 28, 2013 at 6:49 PM, <mailto:bulach@chromium.org> wrote:
>>
>
>  > +frankf / craigdh
>> >
>> >
>> >
>> > https://codereview.chromium.org/91573003/diff/40001/tools/
>> > telemetry/telemetry/core/platform/android_platform_backend.py
>> > File tools/telemetry/telemetry/core/platform/android_
>> platform_backend.py
>> > (right):
>> >
>> > https://codereview.chromium.org/91573003/diff/40001/tools/
>> > telemetry/telemetry/core/platform/android_platform_
>> backend.py#newcode115
>> > tools/telemetry/telemetry/core/platform/android_
>> platform_backend.py:115:
>> > if not self._purge_ashmem_binary_was_pushed:
>> > On 2013/11/28 17:31:05, tonyg wrote:
>> >
>> >> This isn't the ideal level for this caching. I've been thinking more
>> >>
>> > lately
>> >
>> >> about how to make android_commands.py talk to the device as
>> >>
>> > infrequently as
>> >
>> >> possible in order to improve cycle time. PushIfNeeded() is a good
>> >>
>> > opportunity.
>> >
>> >  Let's modify PushIfNeeded to store a cache of the {host_path: mtime}
>> >>
>> > of the host
>> >
>> >> binary each time it computes the md5sum. Then if the host binary's
>> >>
>> > mtime is
>> >
>> >> unchanged, shortcut the remainder of the checks.
>> >>
>> >
>> >  This will prevent the caching hack from proliferating throughout the
>> >>
>> > codebase.
>> >
>> > this is a great idea!!
>> > there's some separate discussions about how to speed up our test cycles,
>> > so I'm adding frankf and craig here..
>> >
>> > note that changing just android_commands isn't enough either, since
>> > there'll be multiple instances, one per test...
>> >
>> > I think what we need is to decouple into a "Pusher" / "Sync"
>> > long-running service that would keep this dict for a longer term (i.e.,
>> > duration of a build cycle, or individual test if not being ran by
>> > buildbot...).
>> >
>> > https://codereview.chromium.org/91573003/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/91573003/
>

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

[Philippe, 2013-11-29 17:02:12]

Thanks guys! PTAL :) BTW, what is the immediate impact of this CL going to be
for perf sheriffs?

https://chromiumcodereview.appspot.com/91573003/diff/40001/tools/telemetry/te...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://chromiumcodereview.appspot.com/91573003/diff/40001/tools/telemetry/te...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:19: from
pylib import constants  # pylint: disable=F0401
On 2013/11/28 17:31:05, tonyg wrote:
> This is out of order
> 
> Also, just a reminder these imports happen for all platforms, so make sure the
> cq is happy.

Done.

https://chromiumcodereview.appspot.com/91573003/diff/40001/tools/telemetry/te...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:120:
self._adb.PushIfNeeded(host_path, device_path)
On 2013/11/28 17:31:05, tonyg wrote:
> host_path_finder and PushIfNeeded should not be used directly in Telemetry
this
> way.
> 
> Please use android_prebuilt_profiler_helper:InstallOnDevice()
> 
> In another CL, we should move that out of the profiler directory.

Done.

https://chromiumcodereview.appspot.com/91573003/diff/40001/tools/telemetry/te...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:135:
self._PurgeUnpinnedAshmem()
On 2013/11/28 17:31:05, tonyg wrote:
> Since this is system-wide, I don't think it belongs in a pid-specific method.
> Seems like we should have a platform.PurgeUnpinnedMemory() method (with an
> ashmem backing on android and a TODO on mac). Then browser.memory_stats would
> only call it a single time before looping through all pids instead of the
> current behavior of calling it before every pid.

Yeah, good idea. BTW we now have emulated discardable memory too thanks to
reveman@ on platforms that don't support it natively. Not sure if there is a
unified way to force purging on those though.

[tonyg, 2013-11-29 17:35:31]

Last comment, everything else is good to go.

Regarding perf sheriff impact, I recommend just sending an email to
perf-sheriffs@ instructing them to ignore any memory regressions that include
your patch in the regression range.

https://chromiumcodereview.appspot.com/91573003/diff/60001/tools/telemetry/te...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://chromiumcodereview.appspot.com/91573003/diff/60001/tools/telemetry/te...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:112:
host_path = host_path_finder.GetMostRecentHostPath('purge_ashmem')
This ignores Telemetry's prebuilt version, which we need to take advantage of.

To do so, use android_prebuilt_profiler_helper.GetHostPath() instead. Note that
SetupPrebuiltTools() puts everything in the right location.

I think we have a little bit of refactoring to do to make finding and installing
the right binaries a no-brainer.

[Philippe, 2013-12-02 09:16:57]

Thanks Tony!

https://chromiumcodereview.appspot.com/91573003/diff/60001/tools/telemetry/te...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://chromiumcodereview.appspot.com/91573003/diff/60001/tools/telemetry/te...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:112:
host_path = host_path_finder.GetMostRecentHostPath('purge_ashmem')
On 2013/11/29 17:35:32, tonyg wrote:
> This ignores Telemetry's prebuilt version, which we need to take advantage of.
> 
> To do so, use android_prebuilt_profiler_helper.GetHostPath() instead. Note
that
> SetupPrebuiltTools() puts everything in the right location.
> 
> I think we have a little bit of refactoring to do to make finding and
installing
> the right binaries a no-brainer.

Yes, good point. It turns out that I didn't need the full path anyway since
InstallOnDevice() is already using GetHostPath().

[tonyg, 2013-12-02 15:21:29]

lgtm

Thanks!

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

JFYI, I'm waiting for the other CL (doing the PushFileIfNeeded() caching)
to land (which requires Frank/Craig's approval) before I land this one :)


On Mon, Dec 2, 2013 at 4:21 PM, <tonyg@chromium.org> wrote:

> lgtm
>
> Thanks!
>
> https://chromiumcodereview.appspot.com/91573003/
>

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

[shadi, 2013-12-02 23:09:01]

https://chromiumcodereview.appspot.com/91573003/diff/1/tools/perf/metrics/mem...
File tools/perf/metrics/memory.py (left):

https://chromiumcodereview.appspot.com/91573003/diff/1/tools/perf/metrics/mem...
tools/perf/metrics/memory.py:122: if trace_name:
Why removing all 'trace_name' support? This will break existing traces and
create bunch of new ones for media benchmark at least.

[Philippe, 2013-12-03 12:54:59]

https://chromiumcodereview.appspot.com/91573003/diff/1/tools/perf/metrics/mem...
File tools/perf/metrics/memory.py (left):

https://chromiumcodereview.appspot.com/91573003/diff/1/tools/perf/metrics/mem...
tools/perf/metrics/memory.py:122: if trace_name:
On 2013/12/02 23:09:02, shadi wrote:
> Why removing all 'trace_name' support? This will break existing traces and
> create bunch of new ones for media benchmark at least.

Should I keep the AddSummaryResults() method and factor out its body (that would
be used by AddResults() too) so that media benchmarks are not affected? For page
cyclers we would only use AddResults(). I'm not familiar with media benchmarks
so any guidance would be appreciated :)

[shadi, 2013-12-03 17:06:52]

On 2013/12/03 12:54:59, Philippe wrote:
>
https://chromiumcodereview.appspot.com/91573003/diff/1/tools/perf/metrics/mem...
> File tools/perf/metrics/memory.py (left):
> 
>
https://chromiumcodereview.appspot.com/91573003/diff/1/tools/perf/metrics/mem...
> tools/perf/metrics/memory.py:122: if trace_name:
> On 2013/12/02 23:09:02, shadi wrote:
> > Why removing all 'trace_name' support? This will break existing traces and
> > create bunch of new ones for media benchmark at least.
> 
> Should I keep the AddSummaryResults() method and factor out its body (that
would
> be used by AddResults() too) so that media benchmarks are not affected? For
page
> cyclers we would only use AddResults(). I'm not familiar with media benchmarks
> so any guidance would be appreciated :)

I am fine with removing AddSummaryResults() in favor with adding an optional
trace_name arg to AddResults() similar to what is done in 
https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/metrics...

On a side note, have you/reviewers considered reporting the difference in memory
usage from Start() and Stop() of memory metric? It seems reasonable to assume
that memory metric reported is the memory consumed from the start of the metric
measurement till it's end, right? Anyway, I might work on this feature as a
separate CL if needed.

[pliard, 2013-12-05 09:48:21]

On 2013/12/03 17:06:52, shadi wrote:
> On 2013/12/03 12:54:59, Philippe wrote:
> >
>
https://chromiumcodereview.appspot.com/91573003/diff/1/tools/perf/metrics/mem...
> > File tools/perf/metrics/memory.py (left):
> > 
> >
>
https://chromiumcodereview.appspot.com/91573003/diff/1/tools/perf/metrics/mem...
> > tools/perf/metrics/memory.py:122: if trace_name:
> > On 2013/12/02 23:09:02, shadi wrote:
> > > Why removing all 'trace_name' support? This will break existing traces and
> > > create bunch of new ones for media benchmark at least.
> > 
> > Should I keep the AddSummaryResults() method and factor out its body (that
> would
> > be used by AddResults() too) so that media benchmarks are not affected? For
> page
> > cyclers we would only use AddResults(). I'm not familiar with media
benchmarks
> > so any guidance would be appreciated :)
> 
> I am fine with removing AddSummaryResults() in favor with adding an optional
> trace_name arg to AddResults() similar to what is done in 
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/metrics...
> 
> On a side note, have you/reviewers considered reporting the difference in
memory
> usage from Start() and Stop() of memory metric? It seems reasonable to assume
> that memory metric reported is the memory consumed from the start of the
metric
> measurement till it's end, right? Anyway, I might work on this feature as a
> separate CL if needed.

Thanks Shadi! I addressed your comment.

Regarding measuring the difference, I would personally be ok with that as part
of some extension metrics but I think we should keep measuring the full
footprint of the process as the main metric. We can talk more about it in your
bug.

[Philippe, 2013-12-09 14:17:18]

Shadi: ping :)


On Thu, Dec 5, 2013 at 10:48 AM, <pliard@google.com> wrote:

> On 2013/12/03 17:06:52, shadi wrote:
>
>> On 2013/12/03 12:54:59, Philippe wrote:
>> >
>>
>
> https://chromiumcodereview.appspot.com/91573003/diff/1/
> tools/perf/metrics/memory.py
>
>> > File tools/perf/metrics/memory.py (left):
>> >
>> >
>>
>
> https://chromiumcodereview.appspot.com/91573003/diff/1/
> tools/perf/metrics/memory.py#oldcode122
>
>> > tools/perf/metrics/memory.py:122: if trace_name:
>> > On 2013/12/02 23:09:02, shadi wrote:
>> > > Why removing all 'trace_name' support? This will break existing
>> traces and
>> > > create bunch of new ones for media benchmark at least.
>> >
>> > Should I keep the AddSummaryResults() method and factor out its body
>> (that
>> would
>> > be used by AddResults() too) so that media benchmarks are not affected?
>> For
>> page
>> > cyclers we would only use AddResults(). I'm not familiar with media
>>
> benchmarks
>
>> > so any guidance would be appreciated :)
>>
>
>  I am fine with removing AddSummaryResults() in favor with adding an
>> optional
>> trace_name arg to AddResults() similar to what is done in
>>
>
> https://code.google.com/p/chromium/codesearch#chromium/
> src/tools/perf/metrics/cpu.py&l=29
>
>  On a side note, have you/reviewers considered reporting the difference in
>>
> memory
>
>> usage from Start() and Stop() of memory metric? It seems reasonable to
>> assume
>> that memory metric reported is the memory consumed from the start of the
>>
> metric
>
>> measurement till it's end, right? Anyway, I might work on this feature as
>> a
>> separate CL if needed.
>>
>
> Thanks Shadi! I addressed your comment.
>
> Regarding measuring the difference, I would personally be ok with that as
> part
> of some extension metrics but I think we should keep measuring the full
> footprint of the process as the main metric. We can talk more about it in
> your
> bug.
>
> https://chromiumcodereview.appspot.com/91573003/
>

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

[shadi, 2013-12-09 17:23:41]

LGTM! 

Thanks for addressing my comments.

[Philippe, 2013-12-09 17:28:10]

Thanks Shadi!


On Mon, Dec 9, 2013 at 6:23 PM, <shadi@chromium.org> wrote:

> LGTM!
>
> Thanks for addressing my comments.
>
> https://chromiumcodereview.appspot.com/91573003/
>

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

[commit-bot: I haz the power, 2013-12-10 11:29:15]

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

[commit-bot: I haz the power, 2013-12-10 15:13:58]

Change committed as 239784

[Philippe, 2013-12-10 17:07:30]

FYI, I had to revert this :/ It caused a failure on Mac:
http://build.chromium.org/p/chromium.perf/builders/Mac10.6%20Perf%284%29/buil...

The Python typing error looks slightly weird though: "AddResults() takes at
least 3 arguments (3 given)"

I'm preparing a fix. I'm not unable to test media.media_cns_cases on my
workstation though:
$ ./tools/perf/run_benchmark --browser=release media.media_cns_cases
TEST IS DISABLED. SKIPPING.

Or can I exerce this code somehow with some unit tests that we would have?


On Tue, Dec 10, 2013 at 4:13 PM, <commit-bot@chromium.org> wrote:

> Change committed as 239784
>
> https://codereview.chromium.org/91573003/
>

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

[shadi, 2013-12-10 18:59:22]

are you running it on Windows? it should be disabled just on Windows. to
debug you can enable it locally just edit benchmarks/media.py.

I don't think the problem is with this benchmark per se, it probably is the
trace_name argument that is only used by media benchmarks atm
On Dec 10, 2013 9:07 AM, "Philippe Liard" <pliard@chromium.org> wrote:

> FYI, I had to revert this :/ It caused a failure on Mac:
>
http://build.chromium.org/p/chromium.perf/builders/Mac10.6%20Perf%284%29/buil...
>
> The Python typing error looks slightly weird though: "AddResults() takes
> at least 3 arguments (3 given)"
>
> I'm preparing a fix. I'm not unable to test media.media_cns_cases on my
> workstation though:
> $ ./tools/perf/run_benchmark --browser=release media.media_cns_cases
> TEST IS DISABLED. SKIPPING.
>
> Or can I exerce this code somehow with some unit tests that we would have?
>
>
> On Tue, Dec 10, 2013 at 4:13 PM, <commit-bot@chromium.org> wrote:
>
>> Change committed as 239784
>>
>> https://codereview.chromium.org/91573003/
>>
>
>

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