Enable vulcanizing perf_insights mapper

Enable vulcanizing perf_insights mapper

BUG=#1636

This patch introduce a major API change which is forcing users to 
specify the mapper function name in map_traces script to avoid 
dynamically loading the mapper file.


time ./perf_insights/bin/map_traces -c local-directory --trace_directory=tracing/test_data/ weatherReportMapFunction --no-vulcanize
...
real	0m47.256s
user	0m47.576s
sys	0m3.639s


time ./perf_insights/bin/map_traces -c local-directory --trace_directory=tracing/test_data/ weatherReportMapFunction 

real	0m28.908s
user	0m27.719s
sys	0m2.524s


A 40% total runtime improvement for trace files in tracing/test_data/

[nednguyen, 2015-11-03 01:18:22]

Description was changed from

==========
Enable vulcanizing perf_insights mapper


BUG=#
==========

to

==========
Enable vulcanizing perf_insights mapper


BUG=#1636



time ./perf_insights/bin/map_traces -c local-directory
--trace_directory=tracing/test_data/ weatherReportMapFunction --no-vulcanize
...
real	0m47.256s
user	0m47.576s
sys	0m3.639s


time ./perf_insights/bin/map_traces -c local-directory
--trace_directory=tracing/test_data/ weatherReportMapFunction 

real	0m28.908s
user	0m27.719s
sys	0m2.524s


A 40% total runtime improvement for trace files in tracing/test_data/
==========

[nednguyen, 2015-11-03 01:19:25]

nednguyen@google.com changed reviewers:
+ nduca@chromium.org, simonhatch@chromium.org

[nednguyen, 2015-11-03 01:19:26]

Not all the tests pass yet, but I would love to have some feedback for the
overall approach before fixing tests.

[nednguyen, 2015-11-03 01:20:55]

Description was changed from

==========
Enable vulcanizing perf_insights mapper


BUG=#1636



time ./perf_insights/bin/map_traces -c local-directory
--trace_directory=tracing/test_data/ weatherReportMapFunction --no-vulcanize
...
real	0m47.256s
user	0m47.576s
sys	0m3.639s


time ./perf_insights/bin/map_traces -c local-directory
--trace_directory=tracing/test_data/ weatherReportMapFunction 

real	0m28.908s
user	0m27.719s
sys	0m2.524s


A 40% total runtime improvement for trace files in tracing/test_data/
==========

to

==========
Enable vulcanizing perf_insights mapper

BUG=#1636

This patch introduce a major behavior change which is forcing users to 
specify the mapper function name in map_traces script to avoid 
dynamically loading the mapper file.


time ./perf_insights/bin/map_traces -c local-directory
--trace_directory=tracing/test_data/ weatherReportMapFunction --no-vulcanize
...
real	0m47.256s
user	0m47.576s
sys	0m3.639s


time ./perf_insights/bin/map_traces -c local-directory
--trace_directory=tracing/test_data/ weatherReportMapFunction 

real	0m28.908s
user	0m27.719s
sys	0m2.524s


A 40% total runtime improvement for trace files in tracing/test_data/
==========

[nednguyen, 2015-11-03 01:21:03]

Description was changed from

==========
Enable vulcanizing perf_insights mapper

BUG=#1636

This patch introduce a major behavior change which is forcing users to 
specify the mapper function name in map_traces script to avoid 
dynamically loading the mapper file.


time ./perf_insights/bin/map_traces -c local-directory
--trace_directory=tracing/test_data/ weatherReportMapFunction --no-vulcanize
...
real	0m47.256s
user	0m47.576s
sys	0m3.639s


time ./perf_insights/bin/map_traces -c local-directory
--trace_directory=tracing/test_data/ weatherReportMapFunction 

real	0m28.908s
user	0m27.719s
sys	0m2.524s


A 40% total runtime improvement for trace files in tracing/test_data/
==========

to

==========
Enable vulcanizing perf_insights mapper

BUG=#1636

This patch introduce a major API change which is forcing users to 
specify the mapper function name in map_traces script to avoid 
dynamically loading the mapper file.


time ./perf_insights/bin/map_traces -c local-directory
--trace_directory=tracing/test_data/ weatherReportMapFunction --no-vulcanize
...
real	0m47.256s
user	0m47.576s
sys	0m3.639s


time ./perf_insights/bin/map_traces -c local-directory
--trace_directory=tracing/test_data/ weatherReportMapFunction 

real	0m28.908s
user	0m27.719s
sys	0m2.524s


A 40% total runtime improvement for trace files in tracing/test_data/
==========

[nednguyen, 2015-11-03 17:03:27]

https://codereview.chromium.org/1417883005/diff/1/third_party/vinn/vinn/_vinn.py
File third_party/vinn/vinn/_vinn.py (right):

https://codereview.chromium.org/1417883005/diff/1/third_party/vinn/vinn/_vinn...
third_party/vinn/vinn/_vinn.py:93: def Vulcanize(file_path, outfile_path,
source_paths=None):
Err, this is left over change, please ignore.

[sh, 2015-11-03 17:27:12]

On 2015/11/03 17:03:27, nednguyen wrote:
>
https://codereview.chromium.org/1417883005/diff/1/third_party/vinn/vinn/_vinn.py
> File third_party/vinn/vinn/_vinn.py (right):
> 
>
https://codereview.chromium.org/1417883005/diff/1/third_party/vinn/vinn/_vinn...
> third_party/vinn/vinn/_vinn.py:93: def Vulcanize(file_path, outfile_path,
> source_paths=None):
> Err, this is left over change, please ignore.

Very cool, 40% is awesome!

General comments:

So if I understand the approach correctly, you pass in a mapper function name
rather than a path to a mapper, which means that the mapper had to be an
existing mapper registered in all_mappers.html.

From a cloud mapping perspective, we were planning on passing in vulcanized js
(this would allow developers to send up jobs to the cloud while building their
mappers). We'd need to change the endpoints to take mapper/reducer names instead
of the planned vulcanized js (which isn't a big deal), and it would mean that we
wouldn't be able to run arbitrary js, only what's been checked in.

The 2nd point, the loss of flexibility to run any arbitrary js to me doesn't
seem like it's that bad. It would mean developers wouldn't be able to send up
WIP stuff, they'd have to develop it locally, maybe that's a good thing?

[nduca, 2015-11-03 18:37:28]

> The 2nd point, the loss of flexibility to run any arbitrary js to me doesn't
> seem like it's that bad. It would mean developers wouldn't be able to send up
> WIP stuff, they'd have to develop it locally, maybe that's a good thing?

this is definitely an active area of discussion. there's some discussion of this
same issue on #1703... my current mental model here is that we want
MapperHandles to specify both function name they want to run AND the filename
they need to import. its kinda messy but i think its the most robust. i haven't
yet reconciled that thinking with this patch though, so maybe the mental model
needs to change still!

[nednguyen, 2015-11-03 20:09:33]

On 2015/11/03 18:37:28, nduca wrote:
> > The 2nd point, the loss of flexibility to run any arbitrary js to me doesn't
> > seem like it's that bad. It would mean developers wouldn't be able to send
up
> > WIP stuff, they'd have to develop it locally, maybe that's a good thing?
> 
> this is definitely an active area of discussion. there's some discussion of
this
> same issue on #1703... my current mental model here is that we want
> MapperHandles to specify both function name they want to run AND the filename
> they need to import. its kinda messy but i think its the most robust. i
haven't
> yet reconciled that thinking with this patch though, so maybe the mental model
> needs to change still!

Yeah, there are few moving pieces and I think I should hold off landing this
patch on MapperHandles. Meanwhile, we can be convinced that vulcanization does
help :-)