Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2224003003:
Vulcanize MD History to improve page-load performance (Closed)
|
Created:
4 years, 4 months ago by tsergeant Modified:
4 years, 4 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, asanka, dbeam+watch-polymer_chromium.org, michaelpg+watch-polymer_chromium.org, Patrick Dubroy, michaelpg+watch-md-ui_chromium.org, dbeam+watch-history_chromium.org, pam+watch_chromium.org, arv+watch_chromium.org, dbeam+watch-downloads_chromium.org, calamity Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionVulcanize MD History to improve page-load performance
Vulcanize is a module which concatenates HTML/JS files together and
greatly improves load-time performance for Polymer WebUI pages. This CL
generalises the vulcanize script used for MD Downloads so that it can
be used for history as well. Also adds uglifyjs to the vulcanize
toolchain, which is used to strip excess comments and whitespace from
the generated files.
The net result is a saving of 25% on the time taken to load the MD
History page (average times 1005ms vs 750ms, n=5), but an approximately
450Kb increase to the size of resources.pak. A follow-up CL to strip
comments from the vulcanize output will reduce this size increase to
150Kb.
BUG=629406
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
NOPRESUBMIT=true
TBR=phajdan.jr@chromium.org
Committed: https://crrev.com/72f1b524959fbf7ec4868f1a289c7ea3c467f14d
Cr-Commit-Position: refs/heads/master@{#412420}
Patch Set 1 : Rebase, revulcanize, avoid duplicating files #
Total comments: 18
Patch Set 2 : dbeam comments #Patch Set 3 : Change to keyword arguments #
Total comments: 12
Patch Set 4 : Revulcanize and address comments #Patch Set 5 : Fix Mac/CrOS #Patch Set 6 : Revulcanize #Patch Set 7 : Rebase #Dependent Patchsets: Messages
Total messages: 60 (37 generated)
Description was changed from ========== Vulcanize MD History to improve page-load performance Vulcanize is a module which concatenates HTML/JS files together and greatly improves load-time performance for Polymer WebUI pages. This CL generalises the vulcanize script used for MD Downloads so that it can be used for history as well. Also adds uglifyjs to the vulcanize toolchain, which is used to strip excess comments and whitespace from the generated files. The net result is an approximately 200Kb increase to the size of resources.pak, but a saving of 25% on the time taken to load the MD History page (average times 1005ms vs 750ms, n=5). BUG=629406 ========== to ========== Vulcanize MD History to improve page-load performance Vulcanize is a module which concatenates HTML/JS files together and greatly improves load-time performance for Polymer WebUI pages. This CL generalises the vulcanize script used for MD Downloads so that it can be used for history as well. Also adds uglifyjs to the vulcanize toolchain, which is used to strip excess comments and whitespace from the generated files. The net result is an approximately 200Kb increase to the size of resources.pak, but a saving of 25% on the time taken to load the MD History page (average times 1005ms vs 750ms, n=5). BUG=629406 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Vulcanize MD History to improve page-load performance Vulcanize is a module which concatenates HTML/JS files together and greatly improves load-time performance for Polymer WebUI pages. This CL generalises the vulcanize script used for MD Downloads so that it can be used for history as well. Also adds uglifyjs to the vulcanize toolchain, which is used to strip excess comments and whitespace from the generated files. The net result is an approximately 200Kb increase to the size of resources.pak, but a saving of 25% on the time taken to load the MD History page (average times 1005ms vs 750ms, n=5). BUG=629406 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Vulcanize MD History to improve page-load performance Vulcanize is a module which concatenates HTML/JS files together and greatly improves load-time performance for Polymer WebUI pages. This CL generalises the vulcanize script used for MD Downloads so that it can be used for history as well. Also adds uglifyjs to the vulcanize toolchain, which is used to strip excess comments and whitespace from the generated files. The net result is an approximately 200Kb increase to the size of resources.pak, but a saving of 25% on the time taken to load the MD History page (average times 1005ms vs 750ms, n=5). BUG=629406 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation NOPRESUBMIT=true ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
this was my version, which didn't separate app.*: https://codereview.chromium.org/2235593002/ i had initially cheated by removing the async import. fixing it involved not using $('history-app') since the dom-module has that ID. why are you keeping app.* separate in the vulcanized version?
On 2016/08/10 01:43:00, michaelpg wrote: > this was my version, which didn't separate app.*: > https://codereview.chromium.org/2235593002/ > > i had initially cheated by removing the async import. fixing it involved not > using $('history-app') since the dom-module has that ID. > > why are you keeping app.* separate in the vulcanized version? The intention was that it would keep our super fast first-paint, while not having a significant impact on overall performance (since 99% of files are still vulcanized). I haven't actually tried the other alternative, though. I'll give it a go this afternoon and see what it looks like.
> I haven't actually tried the other alternative, though. I'll give it a go this > afternoon and see what it looks like. Having now given this a go: It slows down painting of our app shim by a few hundred milliseconds, making the load feel jankier. I suspect there's a small improvement (1-2%) in load performance, but it's pretty much lost in the noise. So, I think I'd prefer keeping the approach with app.vulcanized.html.
Description was changed from ========== Vulcanize MD History to improve page-load performance Vulcanize is a module which concatenates HTML/JS files together and greatly improves load-time performance for Polymer WebUI pages. This CL generalises the vulcanize script used for MD Downloads so that it can be used for history as well. Also adds uglifyjs to the vulcanize toolchain, which is used to strip excess comments and whitespace from the generated files. The net result is an approximately 200Kb increase to the size of resources.pak, but a saving of 25% on the time taken to load the MD History page (average times 1005ms vs 750ms, n=5). BUG=629406 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation NOPRESUBMIT=true ========== to ========== Vulcanize MD History to improve page-load performance Vulcanize is a module which concatenates HTML/JS files together and greatly improves load-time performance for Polymer WebUI pages. This CL generalises the vulcanize script used for MD Downloads so that it can be used for history as well. Also adds uglifyjs to the vulcanize toolchain, which is used to strip excess comments and whitespace from the generated files. The net result is an approximately 150Kb increase to the size of resources.pak, but a saving of 25% on the time taken to load the MD History page (average times 1005ms vs 750ms, n=5). BUG=629406 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation NOPRESUBMIT=true ==========
Patchset #1 (id:10001) has been deleted
Patchset #1 (id:20001) has been deleted
tsergeant@chromium.org changed reviewers: + dbeam@chromium.org
+dbeam to review, cc calamity as FYI https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... File chrome/browser/resources/vulcanize.py (right): https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... chrome/browser/resources/vulcanize.py:89: _run_cmd(['uglifyjs', js_out_path, Uglify-with-beautify is...a little strange. Stripping comments saves us at least 200K, so we definitely want to do it, and uglify is the best option I've found for stripping all comments without destroying the structure of the code. It does still end up in effectively rewriting big chunks of md_downloads/crisper.js, though. We could use //third_party/closure_compiler/js_minify.py, but that strips out almost all whitespace unconditionally, leaving us with a big blob which is terrible for git.
can we do uglify separately? i'd like to minimize the variables between potential regressions. if we do this separately it'll be easier to reason about. i'm totally into it, though. this looks more awesomer but more legitimate. https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... File chrome/browser/resources/vulcanize.py (right): https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... chrome/browser/resources/vulcanize.py:52: def vulcanize(directory, host, html_in_file, html_out_file, js_out_file, can you name some or all of these params? https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... chrome/browser/resources/vulcanize.py:52: def vulcanize(directory, host, html_in_file, html_out_file, js_out_file, nit: this is not public-ish to those that import this .py file. do you want that? I don't. https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... chrome/browser/resources/vulcanize.py:53: extra_args = []): python style guide asks you to use something like None type for this, i.e. extra_args=None (as well as drop the spaces next to =) and if you want to homogenize the type later, use extra_args = extra_args or [] https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... chrome/browser/resources/vulcanize.py:79: '<include src="', '<include src="../../../../ui/webui/resources/js/')) sooooooo are we continuing this hack? ../../../../ is specific to chrome/browser/resources/md_downloads (it just happens to match the depth of chrome/browser/resources/md_history as well) it's also probably silly to just assume all <include> want to go to ui/webui/resources/js/ https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... chrome/browser/resources/vulcanize.py:89: _run_cmd(['uglifyjs', js_out_path, On 2016/08/10 06:40:34, tsergeant wrote: > Uglify-with-beautify is...a little strange. Stripping comments saves us at least > 200K, so we definitely want to do it, and uglify is the best option I've found > for stripping all comments without destroying the structure of the code. It does > still end up in effectively rewriting big chunks of md_downloads/crisper.js, > though. > > We could use //third_party/closure_compiler/js_minify.py, but that strips out > almost all whitespace unconditionally, leaving us with a big blob which is > terrible for git. I don't really care what we use, but either way we should sync up with aberent. https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... chrome/browser/resources/vulcanize.py:106: \n\n between top-levels https://codereview.chromium.org/2224003003/diff/30001/docs/vulcanize.md File docs/vulcanize.md (right): https://codereview.chromium.org/2224003003/diff/30001/docs/vulcanize.md#newcode1 docs/vulcanize.md:1: # Vulcanizing Material Design pages nit: Material Design * -> Chrome Polymer UIs?
Patchset #3 (id:70001) has been deleted
Patchset #2 (id:50001) has been deleted
I've removed uglify for now and will add it back in a follow-up. https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... File chrome/browser/resources/vulcanize.py (right): https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... chrome/browser/resources/vulcanize.py:52: def vulcanize(directory, host, html_in_file, html_out_file, js_out_file, On 2016/08/10 18:17:41, Dan Beam wrote: > can you name some or all of these params? I'm not sure what you mean? They are named? https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... chrome/browser/resources/vulcanize.py:53: extra_args = []): On 2016/08/10 18:17:41, Dan Beam wrote: > python style guide asks you to use something like None type for this, i.e. > > extra_args=None > > (as well as drop the spaces next to =) > > and if you want to homogenize the type later, use > > extra_args = extra_args or [] Done. https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... chrome/browser/resources/vulcanize.py:79: '<include src="', '<include src="../../../../ui/webui/resources/js/')) On 2016/08/10 18:17:41, Dan Beam wrote: > sooooooo are we continuing this hack? > > ../../../../ is specific to chrome/browser/resources/md_downloads (it just > happens to match the depth of chrome/browser/resources/md_history as well) > > it's also probably silly to just assume all <include> want to go to > ui/webui/resources/js/ Hmmmmmmmmm. At the moment, the only place this is used is util.js including assert.js. We can replace that with an import in util.html, which results in the same vulcanize output for downloads. However, we still need to deal with the <include> tag in util.js. I've changed this line to rewrite the tag so grit doesn't pick it up. I'm not sure if there's any particularly nice way to do it. https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... chrome/browser/resources/vulcanize.py:106: On 2016/08/10 18:17:41, Dan Beam wrote: > \n\n between top-levels Done. https://codereview.chromium.org/2224003003/diff/30001/docs/vulcanize.md File docs/vulcanize.md (right): https://codereview.chromium.org/2224003003/diff/30001/docs/vulcanize.md#newcode1 docs/vulcanize.md:1: # Vulcanizing Material Design pages On 2016/08/10 18:17:41, Dan Beam wrote: > nit: Material Design * -> Chrome Polymer UIs? Done.
Description was changed from ========== Vulcanize MD History to improve page-load performance Vulcanize is a module which concatenates HTML/JS files together and greatly improves load-time performance for Polymer WebUI pages. This CL generalises the vulcanize script used for MD Downloads so that it can be used for history as well. Also adds uglifyjs to the vulcanize toolchain, which is used to strip excess comments and whitespace from the generated files. The net result is an approximately 150Kb increase to the size of resources.pak, but a saving of 25% on the time taken to load the MD History page (average times 1005ms vs 750ms, n=5). BUG=629406 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation NOPRESUBMIT=true ========== to ========== Vulcanize MD History to improve page-load performance Vulcanize is a module which concatenates HTML/JS files together and greatly improves load-time performance for Polymer WebUI pages. This CL generalises the vulcanize script used for MD Downloads so that it can be used for history as well. Also adds uglifyjs to the vulcanize toolchain, which is used to strip excess comments and whitespace from the generated files. The net result is a saving of 25% on the time taken to load the MD History page (average times 1005ms vs 750ms, n=5), but an approximately 450Kb increase to the size of resources.pak. A follow-up CL to strip comments from the vulcanize output will reduce this size increase to 150Kb. BUG=629406 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation NOPRESUBMIT=true ==========
lgtm let's see what blows up! https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... File chrome/browser/resources/vulcanize.py (right): https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... chrome/browser/resources/vulcanize.py:52: def vulcanize(directory, host, html_in_file, html_out_file, js_out_file, On 2016/08/11 01:28:26, tsergeant wrote: > On 2016/08/10 18:17:41, Dan Beam wrote: > > can you name some or all of these params? > > I'm not sure what you mean? They are named? sorry, keyword=arguments https://docs.python.org/3/tutorial/controlflow.html#keyword-arguments
tsergeant@chromium.org changed reviewers: + phajdan.jr@chromium.org
Thanks for reviewing, Dan. I'll hold off on committing until I'm back in office on Monday in case things do blow up. In the meantime, +phajdan.jr@ for tools/copyright_scanner OWNERS https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... File chrome/browser/resources/vulcanize.py (right): https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... chrome/browser/resources/vulcanize.py:52: def vulcanize(directory, host, html_in_file, html_out_file, js_out_file, On 2016/08/11 02:29:49, Dan Beam wrote: > On 2016/08/11 01:28:26, tsergeant wrote: > > On 2016/08/10 18:17:41, Dan Beam wrote: > > > can you name some or all of these params? > > > > I'm not sure what you mean? They are named? > > sorry, keyword=arguments > https://docs.python.org/3/tutorial/controlflow.html#keyword-arguments Right, got it. I've changed the output files to keyword arguments. https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... chrome/browser/resources/vulcanize.py:79: '<include src="', '<include src="../../../../ui/webui/resources/js/')) On 2016/08/11 01:28:26, tsergeant wrote: > On 2016/08/10 18:17:41, Dan Beam wrote: > > sooooooo are we continuing this hack? > > > > ../../../../ is specific to chrome/browser/resources/md_downloads (it just > > happens to match the depth of chrome/browser/resources/md_history as well) > > > > it's also probably silly to just assume all <include> want to go to > > ui/webui/resources/js/ > > Hmmmmmmmmm. > > At the moment, the only place this is used is util.js including assert.js. We > can replace that with an import in util.html, which results in the same > vulcanize output for downloads. > > However, we still need to deal with the <include> tag in util.js. I've changed > this line to rewrite the tag so grit doesn't pick it up. I'm not sure if there's > any particularly nice way to do it. After discussing this: I'm leaving it this way for simplicity's sake, and the small amount of debugging information it gives.
still lgtm but https://codereview.chromium.org/2224003003/diff/110001/chrome/browser/resourc... File chrome/browser/resources/vulcanize.py (right): https://codereview.chromium.org/2224003003/diff/110001/chrome/browser/resourc... chrome/browser/resources/vulcanize.py:53: js_out_file='crisper.js', extra_args=None): can we me all of these keywords? looking at the callsites of _vulcanize(), nothing is particularly intuitive and they seem easy-ish to mess up directory=directory, host=host, html_in_file=html_in_file https://codereview.chromium.org/2224003003/diff/110001/chrome/browser/resourc... chrome/browser/resources/vulcanize.py:93: _vulcanize('md_downloads', 'downloads', 'downloads.html') and then from each callsite use the keywords because it's way easier to read, IMO _vulcanize(directory='md_downloads', host='downloads', html_in_file='downloads.html') etc.
michaelpg@chromium.org changed reviewers: + michaelpg@chromium.org
https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... File chrome/browser/resources/vulcanize.py (right): https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... chrome/browser/resources/vulcanize.py:79: '<include src="', '<include src="../../../../ui/webui/resources/js/')) On 2016/08/11 03:28:00, tsergeant wrote: > On 2016/08/11 01:28:26, tsergeant wrote: > > On 2016/08/10 18:17:41, Dan Beam wrote: > > > sooooooo are we continuing this hack? > > > > > > ../../../../ is specific to chrome/browser/resources/md_downloads (it just > > > happens to match the depth of chrome/browser/resources/md_history as well) > > > > > > it's also probably silly to just assume all <include> want to go to > > > ui/webui/resources/js/ > > > > Hmmmmmmmmm. > > > > At the moment, the only place this is used is util.js including assert.js. We > > can replace that with an import in util.html, which results in the same > > vulcanize output for downloads. > > > > However, we still need to deal with the <include> tag in util.js. I've changed > > this line to rewrite the tag so grit doesn't pick it up. I'm not sure if > there's > > any particularly nice way to do it. > > After discussing this: I'm leaving it this way for simplicity's sake, and the > small amount of debugging information it gives. I think the right way to do this is to run grit *before* vulcanizing. This is basically required for Settings because of all the <if> grit tags, but it should take care of <include> as well before it ever gets to vulcanize. Here's the hacky script I use locally: https://codereview.chromium.org/1870853003/diff/20001/chrome/browser/resource... https://codereview.chromium.org/2224003003/diff/110001/chrome/browser/resourc... File chrome/browser/resources/md_history/PRESUBMIT.py (right): https://codereview.chromium.org/2224003003/diff/110001/chrome/browser/resourc... chrome/browser/resources/md_history/PRESUBMIT.py:20: history_changes = filter(_is_md_history_file, paths) IDK how useful this is; we ought to include changes to third_party/polymer and ui/webui/resources as well, but it's hard to limit that to files inlined via vulcanize/crisper. And Downloads should have a similar presubmit. Ultimately I think "set noparent" in OWNERS is the only way to guard against mistakes without node in the build toolchain. https://codereview.chromium.org/2224003003/diff/110001/chrome/browser/resourc... chrome/browser/resources/md_history/PRESUBMIT.py:25: 'chrome/browser/resources/vulcanize.py.')] maybe docs/vulcanize.md instead? https://codereview.chromium.org/2224003003/diff/110001/chrome/browser/resourc... File chrome/browser/resources/md_history/history.html (left): https://codereview.chromium.org/2224003003/diff/110001/chrome/browser/resourc... chrome/browser/resources/md_history/history.html:81: <link rel="import" href="chrome://resources/html/cr.html"> ? https://codereview.chromium.org/2224003003/diff/110001/ui/webui/resources/htm... File ui/webui/resources/html/util.html (right): https://codereview.chromium.org/2224003003/diff/110001/ui/webui/resources/htm... ui/webui/resources/html/util.html:1: <link rel="import" href="chrome://resources/html/assert.html"> does this mean we load assert.js via <script> in assert.html, and also inlined in util.js?
I am slightly confused by this. Grit already has the ability to inline scripts, and to handle includes; how is what vulcanize does different? Grit also, now (since https://codereview.chromium.org/2179033002 landed), has the ability to do simple minification its javascript input files (whitespace and comment stripping, but nothing else), although, as yet, this is only enabled on Android. As such minifying them outside grit seems pointless. On 2016/08/11 07:42:15, michaelpg wrote: > https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... > File chrome/browser/resources/vulcanize.py (right): > > https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... > chrome/browser/resources/vulcanize.py:79: '<include src="', '<include > src="../../../../ui/webui/resources/js/')) > On 2016/08/11 03:28:00, tsergeant wrote: > > On 2016/08/11 01:28:26, tsergeant wrote: > > > On 2016/08/10 18:17:41, Dan Beam wrote: > > > > sooooooo are we continuing this hack? > > > > > > > > ../../../../ is specific to chrome/browser/resources/md_downloads (it just > > > > happens to match the depth of chrome/browser/resources/md_history as well) > > > > > > > > it's also probably silly to just assume all <include> want to go to > > > > ui/webui/resources/js/ > > > > > > Hmmmmmmmmm. > > > > > > At the moment, the only place this is used is util.js including assert.js. > We > > > can replace that with an import in util.html, which results in the same > > > vulcanize output for downloads. > > > > > > However, we still need to deal with the <include> tag in util.js. I've > changed > > > this line to rewrite the tag so grit doesn't pick it up. I'm not sure if > > there's > > > any particularly nice way to do it. > > > > After discussing this: I'm leaving it this way for simplicity's sake, and the > > small amount of debugging information it gives. > > I think the right way to do this is to run grit *before* vulcanizing. This is > basically required for Settings because of all the <if> grit tags, but it should > take care of <include> as well before it ever gets to vulcanize. Have a look at the flatten_resources.py code in https://codereview.chromium.org/2094193004/, which does this within grit. Note, however, that this did not, in the end, land, because it was felt that listing the source files in both GN (necessary to get the correct build dependencies) and in the .grd files would cause maintenance problems. Instead we decided to invoke the JS minimizer from within grit. See, in particular, brettw@'s comments on that CL and the discussion in https://groups.google.com/a/chromium.org/d/msg/chromium-dev/x0I23ImvGH0/Xa2u-.... > > Here's the hacky script I use locally: > https://codereview.chromium.org/1870853003/diff/20001/chrome/browser/resource... > > https://codereview.chromium.org/2224003003/diff/110001/chrome/browser/resourc... > File chrome/browser/resources/md_history/PRESUBMIT.py (right): > > https://codereview.chromium.org/2224003003/diff/110001/chrome/browser/resourc... > chrome/browser/resources/md_history/PRESUBMIT.py:20: history_changes = > filter(_is_md_history_file, paths) > IDK how useful this is; we ought to include changes to third_party/polymer and > ui/webui/resources as well, but it's hard to limit that to files inlined via > vulcanize/crisper. And Downloads should have a similar presubmit. > > Ultimately I think "set noparent" in OWNERS is the only way to guard against > mistakes without node in the build toolchain. > > https://codereview.chromium.org/2224003003/diff/110001/chrome/browser/resourc... > chrome/browser/resources/md_history/PRESUBMIT.py:25: > 'chrome/browser/resources/vulcanize.py.')] > maybe docs/vulcanize.md instead? > > https://codereview.chromium.org/2224003003/diff/110001/chrome/browser/resourc... > File chrome/browser/resources/md_history/history.html (left): > > https://codereview.chromium.org/2224003003/diff/110001/chrome/browser/resourc... > chrome/browser/resources/md_history/history.html:81: <link rel="import" > href="chrome://resources/html/cr.html"> > ? > > https://codereview.chromium.org/2224003003/diff/110001/ui/webui/resources/htm... > File ui/webui/resources/html/util.html (right): > > https://codereview.chromium.org/2224003003/diff/110001/ui/webui/resources/htm... > ui/webui/resources/html/util.html:1: <link rel="import" > href="chrome://resources/html/assert.html"> > does this mean we load assert.js via <script> in assert.html, and also inlined > in util.js?
On 2016/08/11 09:27:22, aberent wrote: > I am slightly confused by this. Grit already has the ability to inline scripts, > and to handle includes; how is what vulcanize does different? > > Grit also, now (since https://codereview.chromium.org/2179033002 landed), has > the ability to do simple minification its javascript input files (whitespace and > comment stripping, but nothing else), although, as yet, this is only enabled on > Android. As such minifying them outside grit seems pointless. There's many differences: * grit doesn't handle <link rel=import> (and I don't want it to) and uses regex rather than a real parser (like vulcanize does) and misses a lot of stuff * grit magically inlines things in unexpected ways sometimes (i.e. people copy paste flattenhtml=true without understanding what it does), vulcanize is fairly harder to misunderstand -- EVERYTHING is changed after, not just individual resources * grit inlines for every <include>, vulcanize is smart enough to ignore already included scripts. this is a big deal as it reduces file size a lot.
https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... File chrome/browser/resources/vulcanize.py (right): https://codereview.chromium.org/2224003003/diff/30001/chrome/browser/resource... chrome/browser/resources/vulcanize.py:79: '<include src="', '<include src="../../../../ui/webui/resources/js/')) On 2016/08/11 07:42:14, michaelpg wrote: > On 2016/08/11 03:28:00, tsergeant wrote: > > On 2016/08/11 01:28:26, tsergeant wrote: > > > On 2016/08/10 18:17:41, Dan Beam wrote: > > > > sooooooo are we continuing this hack? > > > > > > > > ../../../../ is specific to chrome/browser/resources/md_downloads (it just > > > > happens to match the depth of chrome/browser/resources/md_history as well) > > > > > > > > it's also probably silly to just assume all <include> want to go to > > > > ui/webui/resources/js/ > > > > > > Hmmmmmmmmm. > > > > > > At the moment, the only place this is used is util.js including assert.js. > We > > > can replace that with an import in util.html, which results in the same > > > vulcanize output for downloads. > > > > > > However, we still need to deal with the <include> tag in util.js. I've > changed > > > this line to rewrite the tag so grit doesn't pick it up. I'm not sure if > > there's > > > any particularly nice way to do it. > > > > After discussing this: I'm leaving it this way for simplicity's sake, and the > > small amount of debugging information it gives. > > I think the right way to do this is to run grit *before* vulcanizing. This is > basically required for Settings because of all the <if> grit tags, but it should > take care of <include> as well before it ever gets to vulcanize. > > Here's the hacky script I use locally: > https://codereview.chromium.org/1870853003/diff/20001/chrome/browser/resource... Grit output is platform specific (due to <if> tags), which is not well suited to what we're doing with History and Downloads (we don't want to be generating a HTML/JS file per platform and storing them in the repo). https://codereview.chromium.org/2224003003/diff/110001/chrome/browser/resourc... File chrome/browser/resources/md_history/PRESUBMIT.py (right): https://codereview.chromium.org/2224003003/diff/110001/chrome/browser/resourc... chrome/browser/resources/md_history/PRESUBMIT.py:20: history_changes = filter(_is_md_history_file, paths) On 2016/08/11 07:42:14, michaelpg wrote: > IDK how useful this is; we ought to include changes to third_party/polymer and > ui/webui/resources as well, but it's hard to limit that to files inlined via > vulcanize/crisper. And Downloads should have a similar presubmit. > > Ultimately I think "set noparent" in OWNERS is the only way to guard against > mistakes without node in the build toolchain. I'm aware that this will have false negatives, and I'm okay with it. This is mostly just to ensure that the vulcanized doesn't get too out of sync while history is being actively developed. I don't think Downloads really needs the same thing since changes are relatively infrequent. https://codereview.chromium.org/2224003003/diff/110001/chrome/browser/resourc... chrome/browser/resources/md_history/PRESUBMIT.py:25: 'chrome/browser/resources/vulcanize.py.')] On 2016/08/11 07:42:14, michaelpg wrote: > maybe docs/vulcanize.md instead? Done. https://codereview.chromium.org/2224003003/diff/110001/chrome/browser/resourc... File chrome/browser/resources/md_history/history.html (left): https://codereview.chromium.org/2224003003/diff/110001/chrome/browser/resourc... chrome/browser/resources/md_history/history.html:81: <link rel="import" href="chrome://resources/html/cr.html"> On 2016/08/11 07:42:14, michaelpg wrote: > ? This is not needed here. It's included directly in other element files where it is used. https://codereview.chromium.org/2224003003/diff/110001/chrome/browser/resourc... File chrome/browser/resources/vulcanize.py (right): https://codereview.chromium.org/2224003003/diff/110001/chrome/browser/resourc... chrome/browser/resources/vulcanize.py:53: js_out_file='crisper.js', extra_args=None): On 2016/08/11 05:44:34, Dan Beam wrote: > can we me all of these keywords? looking at the callsites of _vulcanize(), > nothing is particularly intuitive and they seem easy-ish to mess up > > directory=directory, host=host, html_in_file=html_in_file Done. I can just use them as keyword arguments from the callsites, there's no need to change anything in the function signature (and no way to force them to be used as keyword arguments). https://codereview.chromium.org/2224003003/diff/110001/chrome/browser/resourc... chrome/browser/resources/vulcanize.py:93: _vulcanize('md_downloads', 'downloads', 'downloads.html') On 2016/08/11 05:44:34, Dan Beam wrote: > and then from each callsite use the keywords because it's way easier to read, > IMO > > _vulcanize(directory='md_downloads', host='downloads', > html_in_file='downloads.html') > > etc. Done.
On 2016/08/11 09:27:22, aberent wrote: > Grit also, now (since https://codereview.chromium.org/2179033002 landed), has > the ability to do simple minification its javascript input files (whitespace and > comment stripping, but nothing else), although, as yet, this is only enabled on > Android. As such minifying them outside grit seems pointless. We can't use the grit functionality directly (although I'd really like to!) due to the same Java problems as everywhere else (we are desktop only). I considered using //third_party/closure_compiler/js_minify.py to do minification outside grit. but as it removes almost all whitespace, the resulting blob is pretty unfriendly to git. I'm not sure how much of a problem that would be in reality.
https://codereview.chromium.org/2224003003/diff/110001/ui/webui/resources/htm... File ui/webui/resources/html/util.html (right): https://codereview.chromium.org/2224003003/diff/110001/ui/webui/resources/htm... ui/webui/resources/html/util.html:1: <link rel="import" href="chrome://resources/html/assert.html"> On 2016/08/11 07:42:14, michaelpg wrote: > does this mean we load assert.js via <script> in assert.html, and also inlined > in util.js? Yes, looks like there are a few places where this will happen (md settings, md user manager, media router).
On 2016/08/15 06:22:07, tsergeant wrote: > https://codereview.chromium.org/2224003003/diff/110001/ui/webui/resources/htm... > File ui/webui/resources/html/util.html (right): > > https://codereview.chromium.org/2224003003/diff/110001/ui/webui/resources/htm... > ui/webui/resources/html/util.html:1: <link rel="import" > href="chrome://resources/html/assert.html"> > On 2016/08/11 07:42:14, michaelpg wrote: > > does this mean we load assert.js via <script> in assert.html, and also inlined > > in util.js? > > Yes, looks like there are a few places where this will happen (md settings, md > user manager, media router). yes, this is the major downside to grit's <include>: you can't just ensure something is included, you have to copy/paste it everywhere without an easy way to strip back out
The CQ bit was checked by dbeam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:170001) has been deleted
phajdan.jr@: Ping, can you please take a look? dbeam/michaelpg@: Are you happy for me to go ahead with this, or should grit directives be handled in a different way?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/16 08:30:37, tsergeant wrote: > phajdan.jr@: Ping, can you please take a look? > > dbeam/michaelpg@: Are you happy for me to go ahead with this, or should grit > directives be handled in a different way? I'm fine with it
lgtm but you may want to re-vulcanize after https://codereview.chromium.org/2249943002/ to get rid of some <if>s from the vulcanized output. i'm not sure why roboto.css still has the <if>s, maybe because of how it's imported, but presumably flattenhtml on the files will strip it out then, so as long as vulcanize still works it's fine
Description was changed from ========== Vulcanize MD History to improve page-load performance Vulcanize is a module which concatenates HTML/JS files together and greatly improves load-time performance for Polymer WebUI pages. This CL generalises the vulcanize script used for MD Downloads so that it can be used for history as well. Also adds uglifyjs to the vulcanize toolchain, which is used to strip excess comments and whitespace from the generated files. The net result is a saving of 25% on the time taken to load the MD History page (average times 1005ms vs 750ms, n=5), but an approximately 450Kb increase to the size of resources.pak. A follow-up CL to strip comments from the vulcanize output will reduce this size increase to 150Kb. BUG=629406 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation NOPRESUBMIT=true ========== to ========== Vulcanize MD History to improve page-load performance Vulcanize is a module which concatenates HTML/JS files together and greatly improves load-time performance for Polymer WebUI pages. This CL generalises the vulcanize script used for MD Downloads so that it can be used for history as well. Also adds uglifyjs to the vulcanize toolchain, which is used to strip excess comments and whitespace from the generated files. The net result is a saving of 25% on the time taken to load the MD History page (average times 1005ms vs 750ms, n=5), but an approximately 450Kb increase to the size of resources.pak. A follow-up CL to strip comments from the vulcanize output will reduce this size increase to 150Kb. BUG=629406 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation NOPRESUBMIT=true TBR=phajdan.jr@chromium.org ==========
Moving phajdan.jr@ to TBR.
The CQ bit was checked by tsergeant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/2224003003/#ps210001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:210001)
Message was sent while issue was closed.
Description was changed from ========== Vulcanize MD History to improve page-load performance Vulcanize is a module which concatenates HTML/JS files together and greatly improves load-time performance for Polymer WebUI pages. This CL generalises the vulcanize script used for MD Downloads so that it can be used for history as well. Also adds uglifyjs to the vulcanize toolchain, which is used to strip excess comments and whitespace from the generated files. The net result is a saving of 25% on the time taken to load the MD History page (average times 1005ms vs 750ms, n=5), but an approximately 450Kb increase to the size of resources.pak. A follow-up CL to strip comments from the vulcanize output will reduce this size increase to 150Kb. BUG=629406 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation NOPRESUBMIT=true TBR=phajdan.jr@chromium.org ========== to ========== Vulcanize MD History to improve page-load performance Vulcanize is a module which concatenates HTML/JS files together and greatly improves load-time performance for Polymer WebUI pages. This CL generalises the vulcanize script used for MD Downloads so that it can be used for history as well. Also adds uglifyjs to the vulcanize toolchain, which is used to strip excess comments and whitespace from the generated files. The net result is a saving of 25% on the time taken to load the MD History page (average times 1005ms vs 750ms, n=5), but an approximately 450Kb increase to the size of resources.pak. A follow-up CL to strip comments from the vulcanize output will reduce this size increase to 150Kb. BUG=629406 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation NOPRESUBMIT=true TBR=phajdan.jr@chromium.org Committed: https://crrev.com/72f1b524959fbf7ec4868f1a289c7ea3c467f14d Cr-Commit-Position: refs/heads/master@{#412420} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/72f1b524959fbf7ec4868f1a289c7ea3c467f14d Cr-Commit-Position: refs/heads/master@{#412420} |
