sandwich: Merge cache-validation.json and urls-for-resources.json tasks

sandwich: Merge cache-validation.json and urls-for-resources.json tasks

Before in the NoState-Prefetch benchmark tasks graph, Sandwich had
the common/cache-validation.json tasks as a leaf. But it could
lead to issue when selecting final tasks with -e REGEX, and
but forgetting to have the regex matching this important task as
well as the subset of task the user initially wanted to run.

This CL addresses this issue by merging the previous tasks
common/cache-validation.json and common/urls-for-resources.json
into a single common/patched-cache-validation.json tasks, and
have all the benchmark subgraphs depending on them so we are
sure to always run the cache validation at least once before the
benchmark runs.

This CL take this oportunity to output further statistics in the
CSV related to the cache genration such as the number of
sub-resources of an URL, the number of expected cached resources
and the actual number of successfully cached resources, to
identify at scale on many URLs from the CSVs potential issues
the WPR patcher could still have on some webpages.

Moreover, this CL take this change in the extract metrics task
change as an opportunity to parse the fat loading trace JSON
file only once per URL repeat row, in order to make theses tasks
faster.

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

[gabadie, 2016-06-02 15:23:31]

Description was changed from

==========
sandwich: Merge cache-validation.json and urls-for-resources.json tasks

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

to

==========
sandwich: Merge cache-validation.json and urls-for-resources.json tasks

Before in the NoState-Prefetch benchmark tasks graph, Sandwich had
the common/cache-validation.json tasks as a leaf. But it could
lead to issue when selecting final tasks with -e REGEX, and
but forgetting to have the regex matching this important task as
well as the subset of task the user initially wanted to run.

This CL addresses this issue by merging the previous tasks 
common/cache-validation.json and common/urls-for-resources.json
into a single common/patched-cache-validation.json tasks, and
have all the benchmark subgraphs depending on them so we are
sure to always run the cache validation at least once before the
benchmark runs.

This CL take this oportunity to output further statistics in the
CSV related to the cache genration such as the number of 
sub-resources of an URL, the number of expected cached resources
and the actual number of successfully cached resources, to
identify at scale on many URLs from the CSVs potential issues
the WPR patcher could still have on some webpages.

Moreover, this CL take this change in the extract metrics task
change as an opportunity to parse the fat loading trace JSON 
file only once per URL repeat row, in order to make theses tasks
faster.

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

[gabadie, 2016-06-02 15:30:37]

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

[pasko, 2016-06-03 17:13:46]

generally looking good

https://codereview.chromium.org/2033093002/diff/40001/tools/android/loading/s...
File tools/android/loading/sandwich_prefetch.py (right):

https://codereview.chromium.org/2033093002/diff/40001/tools/android/loading/s...
tools/android/loading/sandwich_prefetch.py:5: import csv
Let's add a top-level comment like:
# Implements a task builder for benchmarking effects of NoState Prefetch.
# Noticeable steps of the task pipeline:
#   * Save a WPR archive
#   * Process the archive to make all resources cacheable
#   * Find out which resources are discoverable by NoState Prefetch
(HTMLPreloadScanner)
#   * Load pages with empty/full/prefetched cache
#   * Extract most important metrics to a CSV

https://codereview.chromium.org/2033093002/diff/40001/tools/android/loading/s...
tools/android/loading/sandwich_prefetch.py:353: 'expected_cached_resources':
[URLs of resources expected to be cached]
nit: comma at the end

https://codereview.chromium.org/2033093002/diff/40001/tools/android/loading/s...
tools/android/loading/sandwich_prefetch.py:462: self._cache_validation_task =
None
I would prefer to just keep a path here than the whole task. Task is more
powerful, but with power comes more responsibility.

What if someone decides that it's a good idea to discover whether a task was
run? That would be a bad design, right? Stating explicitly what we need here
would make it for a more relaxed reading/maintaining.

https://codereview.chromium.org/2033093002/diff/40001/tools/android/loading/s...
tools/android/loading/sandwich_prefetch.py:472:
common/patched-cache-validation.json
probably should rename later to something like patched-cache-metadata.json. It
better describes the fact that there is more than validation stuff.

Better be done in a separate change. Maybe a TODO(gabadie) here then?

https://codereview.chromium.org/2033093002/diff/40001/tools/android/loading/s...
tools/android/loading/sandwich_prefetch.py:534: depends on: some tasks saved by
PopulateCommonPipelines()
only depends on -setup.json, right?

https://codereview.chromium.org/2033093002/diff/40001/tools/android/loading/s...
tools/android/loading/sandwich_prefetch.py:536: depends on: some tasks saved by
PopulateCommonPipelines()
some? should it just say common/patched-cache-validation.json?

https://codereview.chromium.org/2033093002/diff/40001/tools/android/loading/s...
tools/android/loading/sandwich_prefetch.py:545:
'benchmark_repeat.subresource_count',
'benchmark_repeat' sounds more like a command than a category name. 

Suggestions:
* 'benchmark_resources'
* 'benchmark'

[gabadie, 2016-06-06 09:43:15]

https://codereview.chromium.org/2033093002/diff/40001/tools/android/loading/s...
File tools/android/loading/sandwich_prefetch.py (right):

https://codereview.chromium.org/2033093002/diff/40001/tools/android/loading/s...
tools/android/loading/sandwich_prefetch.py:5: import csv
On 2016/06/03 17:13:46, pasko wrote:
> Let's add a top-level comment like:
> # Implements a task builder for benchmarking effects of NoState Prefetch.
> # Noticeable steps of the task pipeline:
> #   * Save a WPR archive
> #   * Process the archive to make all resources cacheable
> #   * Find out which resources are discoverable by NoState Prefetch
> (HTMLPreloadScanner)
> #   * Load pages with empty/full/prefetched cache
> #   * Extract most important metrics to a CSV

Done.

https://codereview.chromium.org/2033093002/diff/40001/tools/android/loading/s...
tools/android/loading/sandwich_prefetch.py:353: 'expected_cached_resources':
[URLs of resources expected to be cached]
On 2016/06/03 17:13:46, pasko wrote:
> nit: comma at the end

Done.

https://codereview.chromium.org/2033093002/diff/40001/tools/android/loading/s...
tools/android/loading/sandwich_prefetch.py:462: self._cache_validation_task =
None
On 2016/06/03 17:13:46, pasko wrote:
> I would prefer to just keep a path here than the whole task. Task is more
> powerful, but with power comes more responsibility.
> 
> What if someone decides that it's a good idea to discover whether a task was
> run? That would be a bad design, right? Stating explicitly what we need here
> would make it for a more relaxed reading/maintaining.

Done.

https://codereview.chromium.org/2033093002/diff/40001/tools/android/loading/s...
tools/android/loading/sandwich_prefetch.py:472:
common/patched-cache-validation.json
On 2016/06/03 17:13:46, pasko wrote:
> probably should rename later to something like patched-cache-metadata.json. It
> better describes the fact that there is more than validation stuff.
> 
> Better be done in a separate change. Maybe a TODO(gabadie) here then?

Ok will make it in a separate CL.

https://codereview.chromium.org/2033093002/diff/40001/tools/android/loading/s...
tools/android/loading/sandwich_prefetch.py:534: depends on: some tasks saved by
PopulateCommonPipelines()
On 2016/06/03 17:13:46, pasko wrote:
> only depends on -setup.json, right?

Done.

https://codereview.chromium.org/2033093002/diff/40001/tools/android/loading/s...
tools/android/loading/sandwich_prefetch.py:536: depends on: some tasks saved by
PopulateCommonPipelines()
On 2016/06/03 17:13:46, pasko wrote:
> some? should it just say common/patched-cache-validation.json?

Done.

https://codereview.chromium.org/2033093002/diff/40001/tools/android/loading/s...
tools/android/loading/sandwich_prefetch.py:545:
'benchmark_repeat.subresource_count',
On 2016/06/03 17:13:46, pasko wrote:
> 'benchmark_repeat' sounds more like a command than a category name. 
> 
> Suggestions:
> * 'benchmark_resources'
> * 'benchmark'

Done.

[pasko, 2016-06-06 13:41:39]

lgtm

[gabadie, 2016-06-06 13:51:04]

On 2016/06/06 13:41:39, pasko wrote:
> lgtm

Thanks! Landing.

[gabadie, 2016-06-06 13:51:09]

The CQ bit was checked by gabadie@chromium.org

[commit-bot: I haz the power, 2016-06-06 13:51:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2033093002/80001

[commit-bot: I haz the power, 2016-06-06 14:36:26]

Description was changed from

==========
sandwich: Merge cache-validation.json and urls-for-resources.json tasks

Before in the NoState-Prefetch benchmark tasks graph, Sandwich had
the common/cache-validation.json tasks as a leaf. But it could
lead to issue when selecting final tasks with -e REGEX, and
but forgetting to have the regex matching this important task as
well as the subset of task the user initially wanted to run.

This CL addresses this issue by merging the previous tasks 
common/cache-validation.json and common/urls-for-resources.json
into a single common/patched-cache-validation.json tasks, and
have all the benchmark subgraphs depending on them so we are
sure to always run the cache validation at least once before the
benchmark runs.

This CL take this oportunity to output further statistics in the
CSV related to the cache genration such as the number of 
sub-resources of an URL, the number of expected cached resources
and the actual number of successfully cached resources, to
identify at scale on many URLs from the CSVs potential issues
the WPR patcher could still have on some webpages.

Moreover, this CL take this change in the extract metrics task
change as an opportunity to parse the fat loading trace JSON 
file only once per URL repeat row, in order to make theses tasks
faster.

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

to

==========
sandwich: Merge cache-validation.json and urls-for-resources.json tasks

Before in the NoState-Prefetch benchmark tasks graph, Sandwich had
the common/cache-validation.json tasks as a leaf. But it could
lead to issue when selecting final tasks with -e REGEX, and
but forgetting to have the regex matching this important task as
well as the subset of task the user initially wanted to run.

This CL addresses this issue by merging the previous tasks 
common/cache-validation.json and common/urls-for-resources.json
into a single common/patched-cache-validation.json tasks, and
have all the benchmark subgraphs depending on them so we are
sure to always run the cache validation at least once before the
benchmark runs.

This CL take this oportunity to output further statistics in the
CSV related to the cache genration such as the number of 
sub-resources of an URL, the number of expected cached resources
and the actual number of successfully cached resources, to
identify at scale on many URLs from the CSVs potential issues
the WPR patcher could still have on some webpages.

Moreover, this CL take this change in the extract metrics task
change as an opportunity to parse the fat loading trace JSON 
file only once per URL repeat row, in order to make theses tasks
faster.

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

[commit-bot: I haz the power, 2016-06-06 14:36:27]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-06 14:38:12]

Description was changed from

==========
sandwich: Merge cache-validation.json and urls-for-resources.json tasks

Before in the NoState-Prefetch benchmark tasks graph, Sandwich had
the common/cache-validation.json tasks as a leaf. But it could
lead to issue when selecting final tasks with -e REGEX, and
but forgetting to have the regex matching this important task as
well as the subset of task the user initially wanted to run.

This CL addresses this issue by merging the previous tasks 
common/cache-validation.json and common/urls-for-resources.json
into a single common/patched-cache-validation.json tasks, and
have all the benchmark subgraphs depending on them so we are
sure to always run the cache validation at least once before the
benchmark runs.

This CL take this oportunity to output further statistics in the
CSV related to the cache genration such as the number of 
sub-resources of an URL, the number of expected cached resources
and the actual number of successfully cached resources, to
identify at scale on many URLs from the CSVs potential issues
the WPR patcher could still have on some webpages.

Moreover, this CL take this change in the extract metrics task
change as an opportunity to parse the fat loading trace JSON 
file only once per URL repeat row, in order to make theses tasks
faster.

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

to

==========
sandwich: Merge cache-validation.json and urls-for-resources.json tasks

Before in the NoState-Prefetch benchmark tasks graph, Sandwich had
the common/cache-validation.json tasks as a leaf. But it could
lead to issue when selecting final tasks with -e REGEX, and
but forgetting to have the regex matching this important task as
well as the subset of task the user initially wanted to run.

This CL addresses this issue by merging the previous tasks
common/cache-validation.json and common/urls-for-resources.json
into a single common/patched-cache-validation.json tasks, and
have all the benchmark subgraphs depending on them so we are
sure to always run the cache validation at least once before the
benchmark runs.

This CL take this oportunity to output further statistics in the
CSV related to the cache genration such as the number of
sub-resources of an URL, the number of expected cached resources
and the actual number of successfully cached resources, to
identify at scale on many URLs from the CSVs potential issues
the WPR patcher could still have on some webpages.

Moreover, this CL take this change in the extract metrics task
change as an opportunity to parse the fat loading trace JSON
file only once per URL repeat row, in order to make theses tasks
faster.

BUG=582080

Committed: https://crrev.com/b20272f4bed6bf1775113960a2f2fbe70f74562d
Cr-Commit-Position: refs/heads/master@{#398021}
==========

[commit-bot: I haz the power, 2016-06-06 14:38:13]

Patchset 5 (id:??) landed as
https://crrev.com/b20272f4bed6bf1775113960a2f2fbe70f74562d
Cr-Commit-Position: refs/heads/master@{#398021}