Opened 8 years ago

Closed 7 years ago

#14684 closed feature (fixed)

[PATCH][CLA] uglify optimizer for dojo build

Reported by: liucougar Owned by: Colin Snover
Priority: high Milestone: 1.9
Component: BuildSystem Version: 1.7.1
Keywords: Cc: Patrick Ruzand
Blocked By: Blocking:

Description (last modified by liucougar)

the attached patch add uglify-js as a optimizer for dojo build system

with uglify, no java is required to build dojo (only nodejs and uglify-js are required)

some numbers for comparison:

dojo.js size gzipped size time (s)
uglify 89973 32695 4.01
shrinksafe 117976 40547 3.49
closure 87544 32228 10.26

Attachments (3)

14684.patch (14.9 KB) - added by liucougar 8 years ago.
updated patch to allow passing uglify options to various staged from build profile
optimizer_plugin.patch (27.3 KB) - added by liucougar 8 years ago.
refactor optimize transform to multiple modules
comments.diff (1.1 KB) - added by Kenneth G. Franqueiro 7 years ago.
Patch which resolves both comments and comments.keepLines

Download all attachments as: .zip

Change History (34)

comment:1 Changed 8 years ago by liucougar

Description: modified (diff)

comment:2 Changed 8 years ago by liucougar

Description: modified (diff)

comment:3 Changed 8 years ago by Rawld Gill

Status: newassigned

nice job (as usual) cougar! I'll get this in.

comment:4 Changed 8 years ago by Rawld Gill

Milestone: tbd1.8
Priority: undecidedlow

Changed 8 years ago by liucougar

Attachment: 14684.patch added

updated patch to allow passing uglify options to various staged from build profile

Changed 8 years ago by liucougar

Attachment: optimizer_plugin.patch added

refactor optimize transform to multiple modules

comment:5 Changed 7 years ago by Rawld Gill

Milestone: 1.82.0

comment:6 Changed 7 years ago by Colin Snover

If someone doesn’t land one of these patches soon I am just going to lob it into trunk myself. :)

comment:7 Changed 7 years ago by dylan

Milestone: 2.01.9
Priority: lowhigh

Now that we're doing a 1.9 release, this should make it into 1.9.

comment:8 Changed 7 years ago by tardyp

Hopefully I'm not being incorrect, but I wish to +1 this patch. Having to drop java as a build dependancy for our dojo based new web UI for buildbot is very important for us.

comment:9 Changed 7 years ago by bill

I'm not necessarily against adding support for uglify, but I think you can avoid the Java dependency by using the closure optimizer.

comment:10 Changed 7 years ago by Colin Snover

Closure Compiler is written in Java.

comment:11 Changed 7 years ago by Colin Snover

Owner: changed from Rawld Gill to Colin Snover

I’m in charge here now.

comment:12 Changed 7 years ago by Colin Snover

Resolution: fixed
Status: assignedclosed

In [30044]:

Add Uglify optimizer to build system. Fixes #14684. Also introduces some ability to pass options using the optimizeOptions key though it is unclear how well this works at this point. Refs #16196. !strict

comment:13 Changed 7 years ago by Colin Snover

Note that currently the uglify optimizer actually runs more slowly than the closure compiler because there is no concurrency and it is CPU-bound. An enhancement ticket should be opened to try to add concurrency to improve uglify speed if someone wants to tackle that.

On an Atom D510, standard.profile.js compilation times are:

161.97s node/ss
281.61s node/closure
380.55s node/uglify
469.93s java/ss
519.86s java/closure
n/a     java/uglify

I was able to shave off about 20s of the closure time just by fixing the bad default logging levels; everyone will benefit from that.

Last edited 7 years ago by Colin Snover (previous) (diff)

comment:14 in reply to:  12 Changed 7 years ago by liucougar

Replying to csnover:

In [30044]:

Add Uglify optimizer to build system. Fixes #14684. Also introduces some ability to pass options using the optimizeOptions key though it is unclear how well this works at this point. Refs #16196. !strict

thanks for landing this, I have been waiting for this day for about 1 year

about the commit: it seems the options to uglify is never passed into uglify-js

comment:15 Changed 7 years ago by Colin Snover

In [30046]:

Make sure options object is actually passed to uglify. Refs #14684.

comment:16 Changed 7 years ago by liucougar

if bc.optimizeOptions.gen_options is specified in profile, and only one of optimizer/layerOptimizer is set to uglify.keeplines and the other is uglify, bc.optimizeOptions.gen_options will be modified (with beautify=true and indent=0)

i think changes made to options.gen_options is better on a dojo.delegate-ed object to avoid side-effects

comment:17 Changed 7 years ago by bill

Resolution: fixed
Status: closedreopened

The doc needs to be updated for this, reopening the ticket so you don't forget. buildScripts.rst and the release notes for 1.9.

comment:18 Changed 7 years ago by bill

Also, looks like you broke --optimize comments. After [30044] this doesn't work for me anymore:

$ cd util/buildscripts
$ ./build.sh --release --releaseDir /ws/webkitBuild --profile profiles/webkitMobile.profile.js --copyTests true --optimize comments

comment:19 in reply to:  16 Changed 7 years ago by Colin Snover

Replying to liucougar:

if bc.optimizeOptions.gen_options is specified in profile, and only one of optimizer/layerOptimizer is set to uglify.keeplines and the other is uglify, bc.optimizeOptions.gen_options will be modified (with beautify=true and indent=0)

i think changes made to options.gen_options is better on a dojo.delegate-ed object to avoid side-effects

I suppose, though I dunno why someone would do that in reality. I will make this change I suppose.

Replying to bill:

Also, looks like you broke --optimize comments. After [30044] this doesn't work for me anymore:

Why would just removing comments ever be a good idea? I am sure this feature made sense to someone once ages ago but I feel like it should just be a dropped thing (like how clean action was dropped). (It was not my intent for it to not work any more but I don’t know why expend effort to put it back.)

Last edited 7 years ago by Colin Snover (previous) (diff)

comment:20 Changed 7 years ago by bill

Presumably people want to use --optimize comments so the code is compressed as much as practical while still being easily debuggable. Regardless, adding support for uglify doesn't give you license to break the existing features.

comment:21 Changed 7 years ago by Colin Snover

The solution to debugging optimised code is fixing #15232. If you need to debug, don’t build with optimisations enabled. If your build is broken by the optimiser, using a different optimiser isn’t going to help debug that. So, assuming #15232 is done, is there some other reason why a comments optimize option is something that should continue to exist, other than this backwards compatibility (which we already stopped doing when removing the clean action)?

comment:22 Changed 7 years ago by Kenneth G. Franqueiro

FWIW, I think it might still work if you set optimize to shrinksafe.comments - if that's the case, seems like something we could release note. But I agree that there's no point to that option - if you're debugging, either use source maps (assuming we add support for it) or debug against either source or a build with optimize turned off completely. If you want performance, you'll want to minify.

comment:23 Changed 7 years ago by Colin Snover

Status: reopenedassigned

It’s good this was reopened anyway because UglifyJS2 doesn’t work, and is now the default version when getting uglify-js from npm, so it needs to be fixed.

comment:24 Changed 7 years ago by Kenneth G. Franqueiro

FWIW if we're _really_ going to be anal about still supporting optimize=comments under the same exact name, here's a really stupid patch that will do it. Note however that I wholeheartedly am -1 in applying this patch. I'd much sooner doc in release notes that if for some reason you use optimize=comments, it is now optimize=shrinksafe.comments (even though that admittedly doesn't make sense), but you would be better off not optimizing at all for debug purposes, and you should always optimize with an actual minifier for actual production.

comment:25 Changed 7 years ago by Kenneth G. Franqueiro

Note that my ridiculously silly patch wouldn't "restore" support for comments.keepLines, but given the following points:

  • comments.keepLines and comments both did the same thing anyway by the looks of it
  • comments.keepLines may have never worked as intended (or rather, comments w/o keepLines never stripped newlines), and indeed comments.keepLines wasn't even a documented option prior to 1.7
  • both options don't hold a lot of water

...I'd still sooner release note this given the state of affairs, limited resources, and that there are more important things to do like support Uglify v2.

Last edited 7 years ago by Kenneth G. Franqueiro (previous) (diff)

comment:26 Changed 7 years ago by dylan

Calling an existing feature or syntax stupid is immediately polarizing and serves no purpose, as it insults the people that raised the issue of breaking compatibility.

Rather, here's what I see:

  • there's a syntax that is currently supported that the earlier patches of this ticket breaks
  • the patch to "fix" it, is incredibly simple
  • as a toolkit, the goal is not to arbitrarily break our users when they upgrade versions, just for the sake of it (we change APIs when it matters, not because we feel like a user is being stupid for relying on a documented API)
  • adding a release note for something is not as simple as keeping something that works, especially when we're talking about going from 1.8 to 1.9.

As such, Ken's "ridiculously stupid" patch, or something comparable, should be merged as we're not in the business of breaking our users intentionally.

comment:27 Changed 7 years ago by Patrick Ruzand

Cc: Patrick Ruzand added

comment:28 Changed 7 years ago by Kenneth G. Franqueiro

Sorry for the overly-opinionated posts last night - I clearly should've just gotten the heck away from IRC rather than burning an hour on this, which clearly had me frustrated (as I don't think the feature that broke is worth even that much of someone's time), but at the same time, I must say I found it somewhat unfair to basically "shoot the committer" when I'm pretty sure even the original patch here had the issue.

I'll provide a patch later that'll cover both comments and comments.keepLines...and add comments to make sure we totally remove such code in 2.0 (since I'd imagine the shrinksafe optimizer would go away as a whole then).

Changed 7 years ago by Kenneth G. Franqueiro

Attachment: comments.diff added

Patch which resolves both comments and comments.keepLines

comment:29 Changed 7 years ago by Kenneth G. Franqueiro

Attached new patch which resolves both comments and comments.keepLines. Note that both behave the same as each other (neither one entirely strips newlines), but this is how they were even in 1.8.

comment:30 Changed 7 years ago by Rawld Gill

The reason I did not commit the original patch was that I was not satisfied with the refactor proposed. I sincerely appreciate cougar's help, and equally sincerely apologize for the delay. That said,

  • I was unaware of a great demand for the patch
  • cougar, the one guy I new really wanted it, already had it
  • the additional capability, while nice, wasn't breathtaking to most

..and that is why I have always pushed this to the back burner.

Which is all to say, this refactor may change yet again; something I wanted to avoid.

But that's water under the bridge.

csnover: thanks for the help on this. could you please integrate the patch on #15413 to work with these changes.

comment:31 Changed 7 years ago by Colin Snover

Resolution: fixed
Status: assignedclosed

In [30778]:

Restore 'comments' optimizer which was broken with refactoring optimizers. Fixes #14684. !strict due to existing code

Note: See TracTickets for help on using tickets.