Revert "Revert 239784 "Make page_cycler.py fully measure memory for each...""

Revert "Revert 239784 "Make page_cycler.py fully measure memory for each...""

media.media_cns_cases was failing with the following error:
TypeError: AddResults() takes at least 3 arguments (3 given)

BUG=323494
R=shadi@chromium.org, shadi@google.com, tonyg@chromium.org

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

[Philippe, 2013-12-10 17:55:53]

FYI, you can diff between patch set 1 and patch set 2. Patch set 1 is the
original CL.

Shadi, I would be super grateful if you could check locally the patch. I'm using
./tools/perf/run_measurement --browser=release media
tools/perf/page_sets/media_cns_cases.json which seems to produce a reasonable
output with the change too but we never know :)

[shadi, 2013-12-10 22:47:57]

LGTM % option issue.

I will also try to patch and run locally.

https://chromiumcodereview.appspot.com/105753006/diff/60001/tools/perf/measur...
File tools/perf/measurements/media.py (right):

https://chromiumcodereview.appspot.com/105753006/diff/60001/tools/perf/measur...
tools/perf/measurements/media.py:31:
memory.MemoryMetric.CustomizeBrowserOptions(options)
I was under the impression that --enable-memory-benchmarking might affect the
browser's performance. If that's the case can you only add the
'--enable-stats-collection-bindings' option needed to query histograms?

tonyg@ can you please confirm if '--enable-stats-collection-bindings' is better
to be avoided if not needed?

https://chromiumcodereview.appspot.com/105753006/diff/60001/tools/perf/measur...
tools/perf/measurements/media.py:57: self._memory_metric.AddResults(tab,
results, trace_name=trace_name)
Good catch :)

[shadi, 2013-12-10 22:57:03]

FWIW, media_cns_cases are disabled on Android and Linux (not windows), but I
have a CL up to enable it back on Linux. 

Running media_cns_cases is passing (even though you get all media errors
problems) and the benchmark is fine as far as this CL is concerned.

[Philippe, 2013-12-11 12:57:06]

Thanks Shadi!

+ Marcus and Dai for the DMP question.

https://chromiumcodereview.appspot.com/105753006/diff/60001/tools/perf/measur...
File tools/perf/measurements/media.py (right):

https://chromiumcodereview.appspot.com/105753006/diff/60001/tools/perf/measur...
tools/perf/measurements/media.py:31:
memory.MemoryMetric.CustomizeBrowserOptions(options)
On 2013/12/10 22:47:58, shadi wrote:
> I was under the impression that --enable-memory-benchmarking might affect the
> browser's performance. If that's the case can you only add the
> '--enable-stats-collection-bindings' option needed to query histograms?
> 
> tonyg@ can you please confirm if '--enable-stats-collection-bindings' is
better
> to be avoided if not needed?

Great catch! If I understand correctly --enable-memory-benchmarking would more
or less enable DMP. On Android this should not affect us since we are not
building with TCMalloc by default but yeah I guess it could quite significantly
affect performance on other platforms. In fact I don't see any reason to have it
enabled by default. If this is indeed needed for DMP then the flag should be
added only when DMP is used (e.g. in tcpmalloc_heap_profiler.py). I'm adding Dai
and Marcus who probably know more about this :)

https://chromiumcodereview.appspot.com/105753006/diff/60001/tools/perf/measur...
tools/perf/measurements/media.py:57: self._memory_metric.AddResults(tab,
results, trace_name=trace_name)
On 2013/12/10 22:47:58, shadi wrote:
> Good catch :)

Yeah, Python is the one who caught it and a bit too late unfortunately :)

[bulach, 2013-12-11 18:15:31]

https://chromiumcodereview.appspot.com/105753006/diff/60001/tools/perf/measur...
File tools/perf/measurements/media.py (right):

https://chromiumcodereview.appspot.com/105753006/diff/60001/tools/perf/measur...
tools/perf/measurements/media.py:31:
memory.MemoryMetric.CustomizeBrowserOptions(options)
On 2013/12/11 12:57:06, Philippe wrote:
> On 2013/12/10 22:47:58, shadi wrote:
> > I was under the impression that --enable-memory-benchmarking might affect
the
> > browser's performance. If that's the case can you only add the
> > '--enable-stats-collection-bindings' option needed to query histograms?
> > 
> > tonyg@ can you please confirm if '--enable-stats-collection-bindings' is
> better
> > to be avoided if not needed?
> 
> Great catch! If I understand correctly --enable-memory-benchmarking would more
> or less enable DMP. On Android this should not affect us since we are not
> building with TCMalloc by default but yeah I guess it could quite
significantly
> affect performance on other platforms. In fact I don't see any reason to have
it
> enabled by default. If this is indeed needed for DMP then the flag should be
> added only when DMP is used (e.g. in tcpmalloc_heap_profiler.py). I'm adding
Dai
> and Marcus who probably know more about this :)

yeah, no idea what it's doing here :) this switch, aka
kEnableMemoryBenchmarking, is only used to enable 
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

which then adds a "javascript" extension to allow
chrome.memoryBenchmarking.heapProfilerDump:

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...


I agree it should be moved to tcmalloc_heap_profiler.py, thanks for spotting it!

[Philippe, 2013-12-11 18:22:07]

Great, thanks Marcus! Tony and Shadi: PTAL :)


On Wed, Dec 11, 2013 at 7:15 PM, <bulach@chromium.org> wrote:

>
> https://chromiumcodereview.appspot.com/105753006/diff/
> 60001/tools/perf/measurements/media.py
> File tools/perf/measurements/media.py (right):
>
> https://chromiumcodereview.appspot.com/105753006/diff/
> 60001/tools/perf/measurements/media.py#newcode31
> tools/perf/measurements/media.py:31:
> memory.MemoryMetric.CustomizeBrowserOptions(options)
> On 2013/12/11 12:57:06, Philippe wrote:
>
>> On 2013/12/10 22:47:58, shadi wrote:
>> > I was under the impression that --enable-memory-benchmarking might
>>
> affect the
>
>> > browser's performance. If that's the case can you only add the
>> > '--enable-stats-collection-bindings' option needed to query
>>
> histograms?
>
>> >
>> > tonyg@ can you please confirm if
>>
> '--enable-stats-collection-bindings' is
>
>> better
>> > to be avoided if not needed?
>>
>
>  Great catch! If I understand correctly --enable-memory-benchmarking
>>
> would more
>
>> or less enable DMP. On Android this should not affect us since we are
>>
> not
>
>> building with TCMalloc by default but yeah I guess it could quite
>>
> significantly
>
>> affect performance on other platforms. In fact I don't see any reason
>>
> to have it
>
>> enabled by default. If this is indeed needed for DMP then the flag
>>
> should be
>
>> added only when DMP is used (e.g. in tcpmalloc_heap_profiler.py). I'm
>>
> adding Dai
>
>> and Marcus who probably know more about this :)
>>
>
> yeah, no idea what it's doing here :) this switch, aka
> kEnableMemoryBenchmarking, is only used to enable
> https://code.google.com/p/chromium/codesearch#chromium/
> src/content/browser/renderer_host/memory_benchmark_message_filter.cc&q=
> MemoryBenchmarkMessageFilter&sq=package:chromium&type=cs
>
> which then adds a "javascript" extension to allow
> chrome.memoryBenchmarking.heapProfilerDump:
>
> https://code.google.com/p/chromium/codesearch#chromium/
> src/content/renderer/memory_benchmarking_extension.cc&q=
> MemoryBenchmarkHostMsg_HeapProfilerDump&sq=package:chromium&type=cs
>
>
> I agree it should be moved to tcmalloc_heap_profiler.py, thanks for
> spotting it!
>
> https://chromiumcodereview.appspot.com/105753006/
>

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

[shadi1, 2013-12-11 18:25:33]

LGTM Thanks!

[Philippe, 2013-12-11 18:27:43]

Great, thanks Shadi! Waiting for Tony now :)


On Wed, Dec 11, 2013 at 7:25 PM, <shadi@google.com> wrote:

> LGTM Thanks!
>
> https://chromiumcodereview.appspot.com/105753006/
>

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

[Philippe, 2013-12-12 13:41:32]

On 2013/12/11 18:27:43, Philippe wrote:
> Great, thanks Shadi! Waiting for Tony now :)
> 
> 
> On Wed, Dec 11, 2013 at 7:25 PM, <mailto:shadi@google.com> wrote:
> 
> > LGTM Thanks!
> >
> > https://chromiumcodereview.appspot.com/105753006/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Tony: ping :) I look forward to landing this.

[Philippe, 2013-12-17 09:33:55]

Tony: ping? :)


On Thu, Dec 12, 2013 at 2:41 PM, <pliard@chromium.org> wrote:

> On 2013/12/11 18:27:43, Philippe wrote:
>
>> Great, thanks Shadi! Waiting for Tony now :)
>>
>
>
>  On Wed, Dec 11, 2013 at 7:25 PM, <mailto:shadi@google.com> wrote:
>>
>
>  > LGTM Thanks!
>> >
>> > https://chromiumcodereview.appspot.com/105753006/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> Tony: ping :) I look forward to landing this.
>
> https://codereview.chromium.org/105753006/
>

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

[tonyg, 2013-12-17 16:19:46]

lgtm

[Philippe, 2013-12-18 09:25:04]

On 2013/12/17 16:19:46, tonyg wrote:
> lgtm

Thanks Tony!

[commit-bot: I haz the power, 2013-12-18 09:25:42]

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

[commit-bot: I haz the power, 2013-12-18 10:06:19]

Retried try job too often on linux_rel for step(s) sync_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Philippe, 2013-12-18 11:23:13]

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