Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an option to preserve program completion value #640

Closed
rvanvelzen opened this issue Feb 11, 2015 · 18 comments · Fixed by #1522
Closed

Add an option to preserve program completion value #640

rvanvelzen opened this issue Feb 11, 2015 · 18 comments · Fixed by #1522

Comments

@rvanvelzen
Copy link
Collaborator

See #543, #625, #628.

@avdg
Copy link
Contributor

avdg commented Jul 10, 2016

rollup/rollup#774 seems to have a nice summary what all the fuzz is about.

@avdg
Copy link
Contributor

avdg commented Jul 10, 2016

Test cases would be nice.

@kzc
Copy link
Contributor

kzc commented Jul 10, 2016

If I interpret this thread correctly it suggests putting parens around IIFEs and any inline function because the Chrome browser - and only the Chrome browser - is slower to parse otherwise?

@nolanlawson
Copy link

This issue affects Edge/IE as well. I did some benchmarks (rollup/rollup#774 (comment)) and spoke with Chakra engineers to confirm.

@kzc
Copy link
Contributor

kzc commented Sep 15, 2016

Firefox and Safari do not have this problem. It's odd that two other major browsers don't optimize this common IIFE idiom. Chrome's parser is reportedly being rewritten to increase its speed which would make this less of an issue.

In any case, people can set the compress option negate_iife=false if they choose.

@kzc
Copy link
Contributor

kzc commented Sep 15, 2016

Based on this remark I think Microsoft would want to optimize the common !function(){...}(); idiom in Chakra:

The experiment confirmed that usage of minified code is extremely popular on the web as it exists and amongst others, UglifyJS is very commonly used in today’s web. So in Windows 10 and Microsoft Edge, we’ve added new fast paths, improved inlining and optimized some heuristics in Chakra’s JIT compiler to ensure that minified code runs as fast, if not faster than the non-minified versions. With these changes, the performance of individual code patterns minified using UglifyJS that we tested, improved between 20-50%.

https://blogs.windows.com/msedgedev/2015/05/20/delivering-fast-javascript-performance-in-microsoft-edge

@rvanvelzen
Copy link
Collaborator Author

This specific issue is regarding completion value. Inner IIFEs would still receive the !.

I can't immediately find a representative benchmark (e.g. profiles of a full application running for a few seconds) in the referenced task. If it turns out to really matter in real world applications it is worth considering removing negate_iife (or defaulting to off).

On the other hand, the performance issue is more easily solved by browser vendors - as it seems it should be a trivial change.

@kzc
Copy link
Contributor

kzc commented Sep 15, 2016

How does one access the "completion value" of the program through an IIFE anyway?

$ node -e '(function(){console.log("completed");return 3})();'
completed

$ echo $?
0

I can't get it to work.

(Edited to mention IIFE)

@nolanlawson
Copy link

nolanlawson commented Sep 16, 2016

@kzc Just ran a benchmark to test (see more comments in #886 (comment)), and both Chakra and V8 optimize for the !function(){}() style. In fact both engines optimize for !function(){}(), (function(){})(), and (function(){}()). Made a mistake, see #886 (comment), only Chakra optimizes for this

What's unoptimized is the UMD-style (or Browserify-style/Webpack-style/etc.) case where the function is passed in to another function. If you wrap parens around the function, though, you can get the optimization.

So in short this has nothing to do with IIFEs and everything to do with parens around immediately-invoked functions (however they may get invoked).

@rvanvelzen
Copy link
Collaborator Author

@kzc an example would be inlined event handlers, which can be minified:

<a onclick="(function () { return false; })()">

After minification with negate_iife, the return value here would be true instead of false.

Granted, this is easily mitigated by disabled negate_iife, but I believe that our default configuration should work for these cases.

@kzc
Copy link
Contributor

kzc commented Sep 16, 2016

@rvanvelzen Thanks for explanation.

@alexlamsl does html-minifier uglify inlined JS code within HTML as per comment above?

@alexlamsl
Copy link
Collaborator

@kzc yes it does, by wrapping the code around !function(){...} before passing to uglify-js, then unwrap the minifier result afterwards.

So whilst the example above would produce the wrong result as stated, the more common form of <a onclick="return false"> works as expected, i.e. <a onclick=return!1>

@kzc
Copy link
Contributor

kzc commented Sep 16, 2016

@alexlamsl Thanks for confirming that uglify is used in the wild in that scenario.

@kzc
Copy link
Contributor

kzc commented Sep 16, 2016

Using the example cited above:

<a onclick="(function () { return false; })()">

It may be better if the negate_iife optimization were to use + rather than ! as in:

+function(){return!1}()

because although the return value is converted to a number, at least the "truthiness" of the return value remains the same:

+false == 0

+true == 1

@kzc
Copy link
Contributor

kzc commented Sep 16, 2016

Ignore my last post above. A lesser wrong is still wrong. :-)

And Microsoft Edge browser reportedly optimizes the ! IIFEs.

@nolanlawson
Copy link

nolanlawson commented Sep 17, 2016

Yes, based on my updated benchmark (#886 (comment)), only Chakra optimizes for ! IIFEs. The blog post you linked to about Chakra specifically optimizing for Uglify patterns is correct.

Also, according to the new benchmark it seems ! IIFEs hurt performance in SpiderMonkey and V8 (not sure about JSC). So you could see this either as a bug in those engines or a bug in Uglify.

In my opinion bundlers/minifiers ought to try harder to give the engine hints, but in this case, doing a good job would be hard. You'd basically have to parse the whole JS file and try to determine which functions are immediately-invoked and which ones aren't, and then wrap the immediately-invoked ones in parens to "trick" the engines into not preparsing. Wrapping non-immediately-invoked functions in parens would be a performance regression because it would increase startup time by fully parsing functions that are only executed later (or never executed).

@kzc
Copy link
Contributor

kzc commented Sep 17, 2016

Saying it's a bug is a bit harsh, as the generated code still produces the correct result in all but one pathological case above: #640 (comment) and the option can be disabled by the user should they choose.

But for the few bytes negate_iife saves in a typical bundle it's probably not worth the trouble:

$ uglifyjs -m -c negate_iife=1,warnings=0 --self  | wc -c
  131282
$ uglifyjs -m -c negate_iife=0,warnings=0 --self  | wc -c
  131285

Uglify is a fairly representative javascript program and only 3 bytes are saved.

Disabling negate_iife by default is reasonable.

@alexlamsl
Copy link
Collaborator

@kzc just read #640 (comment) and thought I might be able to answer that...

$ node
> (function(){console.log("completed");return 3})();
completed
3
>

alexlamsl added a commit to alexlamsl/UglifyJS that referenced this issue Mar 1, 2017
Things like Angular Expression and Bookmarklet do not specify `return`, but implicitedly assumes the evaluated value from the final statement to be the return value.

fixes mishoo#354
fixes mishoo#543
fixes mishoo#625
fixes mishoo#628
fixes mishoo#640
closes mishoo#1293
alexlamsl added a commit to alexlamsl/UglifyJS that referenced this issue Mar 2, 2017
Bookmarklet for instance implicitedly assumes a "completion value" without using `return`.
The `expression` option now supports such use cases.
Optimisations on IIFEs also enhanced.

fixes mishoo#354
fixes mishoo#543
fixes mishoo#625
fixes mishoo#628
fixes mishoo#640
closes mishoo#1293
alexlamsl added a commit to alexlamsl/UglifyJS that referenced this issue Mar 3, 2017
Bookmarklet for instance implicitedly assumes a "completion value" without using `return`.
The `expression` option now supports such use cases.
Optimisations on IIFEs also enhanced.

fixes mishoo#354
fixes mishoo#543
fixes mishoo#625
fixes mishoo#628
fixes mishoo#640
closes mishoo#1293
alexlamsl added a commit that referenced this issue Mar 3, 2017
Bookmarklet for instance implicitedly assumes a "completion value" without using `return`.
The `expression` option now supports such use cases.
Optimisations on IIFEs also enhanced.

fixes #354
fixes #543
fixes #625
fixes #628
fixes #640
closes #1293
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants