Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(251)

Issue 1417883005: Enable vulcanizing perf_insights mapper

Created:
5 years, 1 month ago by nednguyen
Modified:
5 years, 1 month ago
Reviewers:
nduca, shatch
CC:
catapult-reviews_chromium.org, tracing-review_chromium.org
Base URL:
https://github.com/catapult-project/catapult@master
Target Ref:
refs/heads/master
Project:
catapult
Visibility:
Public.

Description

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/

Patch Set 1 #

Total comments: 1

Patch Set 2 : Undo changes to vinn #

Unified diffs Side-by-side diffs Delta from patch set Stats (+142 lines, -117 lines) Patch
M perf_insights/perf_insights/map_runner.py View 3 chunks +15 lines, -6 lines 0 comments Download
M perf_insights/perf_insights/map_single_trace.py View 3 chunks +96 lines, -64 lines 0 comments Download
M perf_insights/perf_insights/map_single_trace_cmdline.html View 3 chunks +1 line, -29 lines 0 comments Download
M perf_insights/perf_insights/map_traces.py View 3 chunks +11 lines, -4 lines 0 comments Download
M perf_insights/perf_insights/mappers/startup_map_function.html View 1 chunk +2 lines, -0 lines 0 comments Download
M perf_insights/perf_insights/mappers/task_info_map_function.html View 1 chunk +2 lines, -0 lines 0 comments Download
M perf_insights/perf_insights/mappers/weather_report_map_function.html View 1 chunk +2 lines, -0 lines 0 comments Download
M perf_insights/perf_insights/perf_insights_full_config.html View 1 chunk +0 lines, -1 line 0 comments Download
M third_party/py_vulcanize/py_vulcanize/generate.py View 2 chunks +5 lines, -3 lines 0 comments Download
M tracing/tracing/base/base.html View 1 chunk +8 lines, -0 lines 0 comments Download
M tracing/tracing/extras/importer/jszip.html View 1 chunk +0 lines, -10 lines 0 comments Download

Messages

Total messages: 9 (4 generated)
nednguyen
Not all the tests pass yet, but I would love to have some feedback for ...
5 years, 1 month ago (2015-11-03 01:19:26 UTC) #3
nednguyen
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.py#newcode93 third_party/vinn/vinn/_vinn.py:93: def Vulcanize(file_path, outfile_path, source_paths=None): Err, this is left over ...
5 years, 1 month ago (2015-11-03 17:03:27 UTC) #6
sh
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.py#newcode93 > ...
5 years, 1 month ago (2015-11-03 17:27:12 UTC) #7
nduca
> The 2nd point, the loss of flexibility to run any arbitrary js to me ...
5 years, 1 month ago (2015-11-03 18:37:28 UTC) #8
nednguyen
5 years, 1 month ago (2015-11-03 20:09:33 UTC) #9
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 :-)

Powered by Google App Engine
This is Rietveld 408576698