sandwich: Make speed-index and memory measurement optional from run-all

sandwich: Make speed-index and memory measurement optional from run-all

Before, the run-all sub-command was not recording the speed-index
video, and the memory measurements were always done using the memory
dump trace events.

Memory dump trace event are large and may impact loading performance
on low-end devices. This CL makes memory measurement optional and
also add the option to measure speed-index in the new command line
flag --measure.

BUG=582080
Committed: https://crrev.com/413a55d3b89527353ae88aa8f9881b791379f652
Cr-Commit-Position: refs/heads/master@{#390407}

[gabadie, 2016-04-28 14:29:32]

gabadie@chromium.org changed reviewers:
+ pasko@chromium.org

[gabadie, 2016-04-28 14:29:33]

Hey Egor,

This CL makes the speed-index and memory measurements optional from the
`run-all` sub-command, to study easily how they impact loading performance on
low-end devices.

PTAL! =)

https://codereview.chromium.org/1925803003/diff/1/tools/android/loading/sandw...
File tools/android/loading/sandwich.py (right):

https://codereview.chromium.org/1925803003/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich.py:300: runner.job_repeat = args.url_repeat
Passing down these benchmark specific parameters using boiler plate code to make
them becoming members of SandwichTaskBuilder was starting to annoy me very much.
So I have decided in this CL to use the full power of the runner transformers.
Instead of making SandwichTaskBuilder more fat.

[pasko, 2016-04-28 15:04:59]

looking good

I think it would be easier to follow if PopulateLoadBenchmark just took a list
of transformers. If the list grows, we would need to modify only one place each
time. Also, if the list gets non-trivial, we can print the list of transformers
later.

WDYT?

https://codereview.chromium.org/1925803003/diff/1/tools/android/loading/sandw...
File tools/android/loading/sandwich.py (right):

https://codereview.chromium.org/1925803003/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich.py:187: nargs='+', help='Enable optional
measure.')
Enable optional measurements.

https://codereview.chromium.org/1925803003/diff/1/tools/android/loading/sandw...
File tools/android/loading/sandwich_metrics.py (right):

https://codereview.chromium.org/1925803003/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich_metrics.py:137: """Extract the metrics from a
given trace.
nit: I don't see a problem with the previous comment. We can call the functions
"ExtractSomething" and explain what it does more colloquially as "Pulls all of
Something".

Up to you

https://codereview.chromium.org/1925803003/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich_metrics.py:155: def
_ExtractMemoryMetricsFromLoadingTrace(loading_trace):
Having two functions with very similar long names makes it harder to read. So I
would suggest:

_ExtractAllMetrics(loading_trace) and _ExtractMemoryMetrics(loading_trace)

Just a friendly optional suggestion. Current naming is not problematic, and I
would not object to it too much.

https://codereview.chromium.org/1925803003/diff/1/tools/android/loading/sandw...
File tools/android/loading/sandwich_runner.py (right):

https://codereview.chromium.org/1925803003/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich_runner.py:195: [MEMORY_DUMP_CATEGORY] if
self.record_memory_dumps else []
More straightforward and does not require awkward backslashes:

additional_categories = []
if self.record_memory_dump:
  additional_categories = [MEMORY_DUMP_CATEGORY]

https://codereview.chromium.org/1925803003/diff/1/tools/android/loading/sandw...
File tools/android/loading/sandwich_task_builder.py (right):

https://codereview.chromium.org/1925803003/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich_task_builder.py:20: parent_transformer=lambda _:
None):
default arg value is not needed, please remove

[gabadie, 2016-04-28 15:56:22]

Thanks Egor! Indeed the list of transformers is a good idea and I have done it
in the newly uploaded revision. PTAL.

https://codereview.chromium.org/1925803003/diff/1/tools/android/loading/sandw...
File tools/android/loading/sandwich.py (right):

https://codereview.chromium.org/1925803003/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich.py:187: nargs='+', help='Enable optional
measure.')
On 2016/04/28 15:04:59, pasko wrote:
> Enable optional measurements.

Done.

https://codereview.chromium.org/1925803003/diff/1/tools/android/loading/sandw...
File tools/android/loading/sandwich_metrics.py (right):

https://codereview.chromium.org/1925803003/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich_metrics.py:137: """Extract the metrics from a
given trace.
On 2016/04/28 15:04:59, pasko wrote:
> nit: I don't see a problem with the previous comment. We can call the
functions
> "ExtractSomething" and explain what it does more colloquially as "Pulls all of
> Something".
> 
> Up to you

I see your point, but here there was an inconsistency between the verb of the
doc string and the name of the functions. I have updated the doc string with
what I believe is the most adapted. Done.

https://codereview.chromium.org/1925803003/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich_metrics.py:155: def
_ExtractMemoryMetricsFromLoadingTrace(loading_trace):
On 2016/04/28 15:04:59, pasko wrote:
> Having two functions with very similar long names makes it harder to read. So
I
> would suggest:
> 
> _ExtractAllMetrics(loading_trace) and _ExtractMemoryMetrics(loading_trace)
> 
> Just a friendly optional suggestion. Current naming is not problematic, and I
> would not object to it too much.

Used _ExtractDefaultMetrics and _ExtractMemoryMetrics

https://codereview.chromium.org/1925803003/diff/1/tools/android/loading/sandw...
File tools/android/loading/sandwich_runner.py (right):

https://codereview.chromium.org/1925803003/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich_runner.py:195: [MEMORY_DUMP_CATEGORY] if
self.record_memory_dumps else []
On 2016/04/28 15:04:59, pasko wrote:
> More straightforward and does not require awkward backslashes:
> 
> additional_categories = []
> if self.record_memory_dump:
>   additional_categories = [MEMORY_DUMP_CATEGORY]

Done.

https://codereview.chromium.org/1925803003/diff/1/tools/android/loading/sandw...
File tools/android/loading/sandwich_task_builder.py (right):

https://codereview.chromium.org/1925803003/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich_task_builder.py:20: parent_transformer=lambda _:
None):
On 2016/04/28 15:04:59, pasko wrote:
> default arg value is not needed, please remove

Done.

[pasko, 2016-04-28 16:07:50]

lgtm

https://codereview.chromium.org/1925803003/diff/20001/tools/android/loading/s...
File tools/android/loading/sandwich.py (right):

https://codereview.chromium.org/1925803003/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich.py:311: for network_condition in ['Regular4G',
'Regular3G', 'Regular2G']:
wondering if this change was essential for things to work or just looks better
this time?

https://codereview.chromium.org/1925803003/diff/20001/tools/android/loading/s...
File tools/android/loading/sandwich_task_builder.py (right):

https://codereview.chromium.org/1925803003/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich_task_builder.py:164: runner_transformer_name:
Name of the runner transformer used to generate
nit: transformer_names: A string describing the transformers, will be used in
Task names (prefer names without spaces and special characters).

[gabadie, 2016-04-28 16:24:01]

Thanks Egor! Uploaded a new revision addressing your nit. Landing.

https://codereview.chromium.org/1925803003/diff/20001/tools/android/loading/s...
File tools/android/loading/sandwich.py (right):

https://codereview.chromium.org/1925803003/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich.py:311: for network_condition in ['Regular4G',
'Regular3G', 'Regular2G']:
On 2016/04/28 16:07:50, pasko wrote:
> wondering if this change was essential for things to work or just looks better
> this time?

Ahah. I was getting very long lines looking crappy once reduce to 80 chars, so I
won some line count here by changing the indent level where they were. =D

https://codereview.chromium.org/1925803003/diff/20001/tools/android/loading/s...
File tools/android/loading/sandwich_task_builder.py (right):

https://codereview.chromium.org/1925803003/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich_task_builder.py:164: runner_transformer_name:
Name of the runner transformer used to generate
On 2016/04/28 16:07:50, pasko wrote:
> nit: transformer_names: A string describing the transformers, will be used in
> Task names (prefer names without spaces and special characters).

Done.

[gabadie, 2016-04-28 16:24:06]

The CQ bit was checked by gabadie@chromium.org

[gabadie, 2016-04-28 16:24:07]

The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1925803003/#ps40001
(title: "Addresses Egor's nit")

[commit-bot: I haz the power, 2016-04-28 16:24:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1925803003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1925803003/40001

[commit-bot: I haz the power, 2016-04-28 17:02:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-30 17:19:14]

Patchset 3 (id:??) landed as
https://crrev.com/413a55d3b89527353ae88aa8f9881b791379f652
Cr-Commit-Position: refs/heads/master@{#390407}

[commit-bot: I haz the power, 2016-04-30 17:56:16]

Description was changed from

==========
sandwich: Make speed-index and memory measurement optional from run-all

Before, the run-all sub-command was not recording the speed-index
video, and the memory measurements were always done using the memory
dump trace events.

Memory dump trace event are large and may impact loading performance
on low-end devices. This CL makes memory measurement optional and
also add the option to measure speed-index in the new command line
flag --measure.

BUG=582080
==========

to

==========
sandwich: Make speed-index and memory measurement optional from run-all

Before, the run-all sub-command was not recording the speed-index
video, and the memory measurements were always done using the memory
dump trace events.

Memory dump trace event are large and may impact loading performance
on low-end devices. This CL makes memory measurement optional and
also add the option to measure speed-index in the new command line
flag --measure.

BUG=582080

Committed: https://crrev.com/413a55d3b89527353ae88aa8f9881b791379f652
Cr-Commit-Position: refs/heads/master@{#390407}
==========