blickly / closure-compiler-issues

0 stars 0 forks source link

Additional optimizations #36

Open blickly opened 9 years ago

blickly commented 9 years ago
There are few more optimizations that Closure Compiler currently doesn't do:

1) 1000 --> 1e3
   10000 --> 1e4
   etc.

2) 0.1 --> .1

3) throw 0.1 --> throw.1
   return 0.1 --> return.1
   etc.

4) x += 1 --> x++
   x -= 1 --> x--

5) /[\w]/ --> /\w/
   /[\s]/ --> /\s/
   etc.

6) (might be too obtrusive)

   (function(){ var f; ... })();
   -->
   (function(f){ ... })();

   (function(){ var f = 10; ... })();
   -->
   (function(f){ .. })(10);

   (function(){ var f, g = 10; ... })();
   -->
   (function(g, f){ ... })(10);

   etc.

7) (function(){ ... }).call(undefined, 5);
   -->
   (function(){ ... }).call(null, 5);

8) new Array(1,2,3);
   -->
   [1,2,3];

Original issue reported on code.google.com by kangax on 2009-11-15 07:17:07

blickly commented 9 years ago
1, 2, 3 seem like strong wins.

6 is quite dangerous.  Given that callees are free to pass extra arguments to functions

without error, and there's the `arguments` expression to worry about, not to mention

`arguments.caller.arguments`

Original issue reported on code.google.com by rictic on 2009-11-15 08:04:28

blickly commented 9 years ago
@rictic

I'm not quite following your #6 concerns. What is it about arguments that worries
you? Could you demonstrate a problem/failure with a simple example? 

`arguments.caller` is actually deprecated in favor of <function>.caller and both are
non-standard, of course.

The reason I'm bringing up #6 type of optimizations is because Closure Compiler
already does something along these lines. It performs optimization that is not 100%
safe; optimization that could actually alter program behavior.

Take a look at this:

(function(){ return arguments[0]; })();

which currently munges to:

(function(a){return a})();

It's easy to come up with example that results in different behavior before and after
munging:

(function() {
  alert(arguments[0] + '; ' + arguments['callee'].length);
})();

compresses to:

(function(a){alert(a+"; "+arguments.callee.length)})();

Note that all of this happens in simple — seemingly safe — mode.

Original issue reported on code.google.com by kangax on 2009-11-15 15:58:32

blickly commented 9 years ago
To address these points in a random order:

we've looked into #6 before. it's not a win over gzip. "var " and "function(){" get
really well-
compressed under gzip. you should file a bug for the "callee" issue. We should be noticing
that and 
backing out the optimization.

#7 is negligible for similar reasons. Also, it's a lot better to just inline the whole
function in 
ADVANCED mode. John should have this out soon.

The command-line compiler already does #1.

#2-#5 are neat, but there are probably more high-impact ways we could spend our time.
We should 
instrument the compiler to count how many times this actually happens in a large JS
library.

#4 is trickier than it sounds
javascript:var x = '1'; x++; var y = '1'; y += 1; alert([x,y]);
-> "2,11"

because ++ and += have different type conversion rules. yay inconsistency.

Original issue reported on code.google.com by Nicholas.J.Santos on 2009-11-15 18:54:45

blickly commented 9 years ago
(No text was entered with this change)

Original issue reported on code.google.com by Nicholas.J.Santos on 2009-11-15 19:11:52

blickly commented 9 years ago
> you should file a bug for the "callee" issue. We should be noticing that and 
backing out the optimization.

Done: http://code.google.com/p/closure-compiler/issues/detail?id=40
Another issue is with func. expressions:
http://code.google.com/p/closure-compiler/issues/detail?id=39

> #7 is negligible for similar reasons. Also, it's a lot better to just inline the
whole function in ADVANCED mode. John should have this out soon.

So changing `undefined` to `null` is generally negligible? Regarding func. inlining,
I noticed that it happens in global code, but in function one (even if it's safe to
do so).

> #2-#5 are neat, but there are probably more high-impact ways we could spend our
time. We should instrument the compiler to count how many times this actually happens
in a large JS library.

That would be nice. I'm curious, which other high-impact optimizations do you have
in
mind?

> because ++ and += have different type conversion rules. yay inconsistency.

Jeez, you're right. `++` applies `ToNumber` and compound operators don't. Interesting
that this "part" is completely unchanged in ES5.

What about #8?

Original issue reported on code.google.com by kangax on 2009-11-15 19:53:45

blickly commented 9 years ago
> you should file a bug for the "callee" issue. We should be noticing that and 
backing out the optimization.

Done: http://code.google.com/p/closure-compiler/issues/detail?id=40
Another issue is with func. expressions:
http://code.google.com/p/closure-compiler/issues/detail?id=39

> #7 is negligible for similar reasons. Also, it's a lot better to just inline the
whole function in ADVANCED mode. John should have this out soon.

So changing `undefined` to `null` is generally negligible? Regarding func. inlining,
I noticed that it happens in global code, but not in function one (even if it's safe
to do so).

> #2-#5 are neat, but there are probably more high-impact ways we could spend our
time. We should instrument the compiler to count how many times this actually happens
in a large JS library.

That would be nice. I'm curious, which other high-impact optimizations do you have
in
mind?

> because ++ and += have different type conversion rules. yay inconsistency.

Jeez, you're right. `++` applies `ToNumber` and compound operators don't. Interesting
that this "part" is completely unchanged in ES5.

What about #8?

Original issue reported on code.google.com by kangax on 2009-11-15 19:54:39

blickly commented 9 years ago
Sorry, my comment was unclear because I accidentally swapped callee for caller.

In general you can't move local variables into the parameters of a function unless
you can control every place that the function is called 
(and you can be certain that certain `arguments` fiddling doesn't take place).  Callers
are free to pass in additional arguments, 
initializing the local-variables-turned-parameters and changing the behavior of the
function.

e.g. this:
var f;
(function() {var a = "hello"; alert(a.split()); f = arguments.callee;})()

if remapped to:
var f;
(function(a) {alert(a.split()); f = arguments.callee;})("hello")

Then external code (which for whatever reason the compiler can't touch) which called
f (e.g. f() or f(10)) could raise an error.

As for #8, I don't know of any problems with this rewrite in practice, nor do I see
any differences in the ECMA-262 specification (p 40-
41 and 88-89).  I didn't comment on it above because literals for primitives (string,
number, boolean) is distinct from the analogous 
constructor call (e.g. `typeof new String('abc') === "object"`) but I don't see any
such problems with arrays.

Original issue reported on code.google.com by rictic on 2009-11-15 21:42:46

blickly commented 9 years ago
Great, thanks for filing those!

Yes, #8 would be easy to add to FoldConstants.

As far as high-impact optimizations go, most of the things we think about these days

involve moving code around to make it smaller/faster. For example, if you have

var x = f();
g(x);

if you can prove that calling f() will not modify g, then this can become:
g(f());

Original issue reported on code.google.com by Nicholas.J.Santos on 2009-11-15 21:58:13

blickly commented 9 years ago
#4 wouldn't work. E.g.

var x = 5;
while ( x ) { alert(x-=1); } // alerted => 4,3,2,1,0
x = 5;
while ( x ) { alert(x--); }  // alerted => 5,4,3,2,1

Changing it to the prefix increment operator should work (--x/++x).

Original issue reported on code.google.com by jamespadolsey on 2009-11-15 23:27:39

blickly commented 9 years ago
@jamespadolsey

Yep, I should have known better :) `x--` evaluates `x`, returns its value and only
then does subtraction, whereas `x-=1` does subtraction and evaluates to already new
value. As a result, two evaluate to different values.

It would be possible to work around it by mapping 'x+=1' to prefix ('++x'), not
postfix, but the fact that `++` applies `ToNumber` on its operand (as explained by
Nicholas) kind of kills this optimization (unless we can be sure that operand value
is numeric one).

On the other hand, we can still transform `new Array(x,x,x)` to `[x,x,x]` as long as
number of array items passed to `new Array` is > 1 (for obvious reasons) ;)

Original issue reported on code.google.com by kangax on 2009-11-15 23:41:46

blickly commented 9 years ago
#2 and #3 I am interested in changing.

We have the tools to do 4. Like Nick mentioned already, it seems like a smaller impact

compare to some of the current optimizations we are trying to push out.

I thought of doing 6 myself until I learned that it has been done and proven not worth

it.

Original issue reported on code.google.com by acleung on 2009-11-17 00:48:22

blickly commented 9 years ago
Another possible optimization would be to transform `new Error(...)` to `Error(...)`
since they are functionally identical (see 15.11.1).

Unfortunately, built-in `Error` constructor can be overwritten/shadowed, and dropping
`new` might then result in a different behavior:

  Error = function(){ this.foo = 'bar'; };

  new Error(); // {foo: 'bar'}
  Error(); // assigns to "foo" property of a global object, returns `undefined`

It should be possible to detect if `Error` is not reassigned/shadowed, and only then
perform this optimization; or maybe it's not worth it.

Speaking of reassigned built-ins, it looks like `new Object()` --> `{}` case suffers
from the very same problem, after all:

  Object = function(){ this.foo = 'bar' };
  alert(new Object().foo);

currently munges to:

  Object=function(){this.foo="bar"};alert({}.foo);

You can probably see the problem here.

And finally, similar optimization can be performed with `new Array(x)`. It can not
be
translated to [x] (since `Array` called with 1 argument sets length of an array to
the value of that argument), but instead could be easily shortened to `Array(x)`
(which is functionally identical).

So (unless `Array` is overwritten/shadowed):

  new Array(x, y, z);  --> [x,y,z]
  new Array(x);        --> Array(x);

Original issue reported on code.google.com by kangax on 2009-11-19 13:58:32

blickly commented 9 years ago
this.a = 1;
this.b = 2;
this.c = 3;
this.d = 4;

should get into
var a = this;
a.a = 1;
a.b = 1;
a.c = 1;
a.d = 1;

It could be even better when there are already other "vars" declared ;)

Original issue reported on code.google.com by alonecomp on 2009-11-19 20:54:05

blickly commented 9 years ago
For just:

this.a = 1;
this.b = 2;
this.c = 3;
this.d = 4;

to:

var a = this;
a.a = 1;
a.b = 1;
a.c = 1;
a.d = 1;

We tried this one time. It turned out the gain is small (or even negative) because
gzip 
pretty good when your code has tons of "this".

However I do have plans for some common subexpression elimination optimizations.

Original issue reported on code.google.com by acleung on 2009-11-19 21:34:16

blickly commented 9 years ago
How about:

From:
  Math.floor(n);

To:
  ~~n;

From:
  Math.ceil(n):

To:
  1+~~n;

Original issue reported on code.google.com by Olmo.Maldonado on 2009-12-16 21:42:49

blickly commented 9 years ago
Comment 15 is no good because it uses ToInt32, resulting in over/undeflow, and
because it has different results with negative numbers.

Math.floor(-1.6); // -2
~~-1.6; // -1.

Optimization can come from building a scope tree and, where possible, replacing
function calls with inline statements (as a compiler). The advanced optimizations
fails miserably in this regard.

For example:

// Input, with advanced optimizations
function x(){
  var y = 3;
  return a;
  function a(){
    return b();
  }
  function b(){
    return y;
  }
}

Output:
Your code compiled to down to 0 bytes. Perhaps you should export some functions?
Learn more

This is a bug because now it is no longer possible to call x.

x()(); // TypeError

Instead, the scope tree should be built internally, so that function removal can be
determined. I would expext something uch as the following intermediate result:-

function x(){
  var y = 3;
  return a;
  function a(){
    return y;
  }
}
- leading to:-

function x(){
  return a;
  function a(){
    return 3;
  }
}

.

Building a scope tree can be done safely. Google closure compilre should build a
scope tree.

Original issue reported on code.google.com by dhtmlkitchen on 2009-12-18 03:18:17

blickly commented 9 years ago
Openned issue 102 for comment 16.

Original issue reported on code.google.com by concavelenz on 2010-02-04 02:19:18

blickly commented 9 years ago
Two more simple yet effective (even after gzip!) optimizations:

true → !0
false → !1

Original issue reported on code.google.com by mathias%qiwi.be@gtempaccount.com on 2010-02-04 06:39:37

blickly commented 9 years ago
Any of these would be nice starter projects for someone wanting to try contributing
to the compiler.  To summarize the comments above, Optimziations that would be nice
to see:
* Shorten objects with alternate construction:
  new Array(x, y, z);  --> [x,y,z]
  new Array(x);        --> Array(x);
  new Error   --> Error

* Normalize the first parameter to .call/.apply:
   (function(){ ... }).call(undefined, 5);
   (function(){ ... }).call(null, 5);

* Normalize regex expressions:
   /[\w]/ --> /\w/
   /[\s]/ --> /\s/
   etc.

* Shorten constants:
  true → !0
  false → !1
  Infinite → 1/0

Note: Alan thought "10000 --> 1e4" was so cool that he implemented it.

Original issue reported on code.google.com by concavelenz on 2010-02-04 16:32:10

blickly commented 9 years ago
@concavelenz (comment #19): You made a small typo there: it’s ‘Infinity’, not ‘Infinite’.

Original issue reported on code.google.com by mathias%qiwi.be@gtempaccount.com on 2010-02-04 19:56:34

blickly commented 9 years ago
@dhtmlkitchen (comment #16): I can see why ~~n isn’t a safe replacement for
Math.floor(n) (because it gives faulty results for negative numbers). But how about
parseInt() with radix = 10?

var n = Math.PI;
parseInt(n, 10); // 3
~~n; // 3
parseInt(-n, 10); // 3
~~(-n); // 3

Original issue reported on code.google.com by mathias%qiwi.be@gtempaccount.com on 2010-02-04 20:06:20

blickly commented 9 years ago
@dhtmlkitchen (comment #16): I can see why ~~n isn’t a safe replacement for
Math.floor(n) (because it gives faulty results for negative numbers). But how about
parseInt() with radix = 10?

var n = Math.PI;
parseInt(n, 10); // 3
~~n; // 3
parseInt(-n, 10); // -3
~~(-n); // -3

@concavelenz (comment #19): You made a small typo there: it’s ‘Infinity’, not ‘Infinite’.

Original issue reported on code.google.com by mathias%qiwi.be@gtempaccount.com on 2010-02-04 20:07:52

blickly commented 9 years ago
Another optimization (thanks to Stoyan Stefanov):

isNaN(n) → n!==n

From 8 bytes to 5.

Original issue reported on code.google.com by mathias%qiwi.be@gtempaccount.com on 2010-03-07 11:18:45

blickly commented 9 years ago
Some more optimizations (thanks to Stoyan Stefanov):

isNaN(n) → n!==n
new Date().getTime() → +new Date()
Number(new Date()) → +new Date()

Original issue reported on code.google.com by mathias%qiwi.be@gtempaccount.com on 2010-03-08 22:29:45

blickly commented 9 years ago
Some more optimizations (thanks to Stoyan Stefanov):

isNaN(n) → n!==n
new Date().getTime() → +new Date()
Number(new Date()) → +new Date()

Original issue reported on code.google.com by mathias%qiwi.be@gtempaccount.com on 2010-03-08 22:29:46

blickly commented 9 years ago
> Number(new Date()) → +new Date()
Only when `Number` references built-in `Number` constructor.

> new Date().getTime() → +new Date()
Same here. Only when `Date.prototype.getTime` references built-in `getTime` method.

> isNaN(n) → n!==n
Not safe either.

var o = { valueOf: function(){ return NaN; } };

o !== o; // false (they reference same objects)
isNaN(o); // true (`NaN` value is returned from `valueOf`, after applying ToNumber
on
operand)

Original issue reported on code.google.com by kangax on 2010-03-08 22:45:28

blickly commented 9 years ago
I do wish that people would stop adding additional potential optimizations to this
defect. Please file new issues for additional optimizations so that
rejected/implemented ones can be closed.

Mailing closure-compiler-discuss@googlegroups.com might be worthwhile before filing
these types of defects as well.

Original issue reported on code.google.com by concavelenz on 2010-03-09 01:52:33

blickly commented 9 years ago
Many of these optimizations are so small that it would not be worth the development

time to actually implement them. Or worth people's time to file them.

I vote to close this bug as WontFix.

Original issue reported on code.google.com by Nicholas.J.Santos on 2010-03-09 04:29:48

blickly commented 9 years ago
I'm willing to close the bug as "won't fix", but these kinds of optimizations are a
great way to get started on 
Closure Compiler.  We ought to write up a quick "here's how to add a new fold constants
pass", let folks try 
running those passes on real code, and see what numbers they get out.

Original issue reported on code.google.com by bowdidge@google.com on 2010-03-09 18:07:58

blickly commented 9 years ago
I would definitely welcome outside contributions for the suggested ideas.

However we would need a better place to add these peep hole optimizations. I am 
against appending to FoldConstants.java as it is getting way to big to be tested /
maintained.

Original issue reported on code.google.com by acleung on 2010-03-09 18:45:02

blickly commented 9 years ago
Small bug: -1000 isn't yet replaced by -1e3

Original issue reported on code.google.com by sjoerd.visscher on 2010-08-07 16:18:04

blickly commented 9 years ago
@sjoerd.visscher
This works as expected with the top-of-tree source.  Seems like it is time for a release.

Original issue reported on code.google.com by johnlenz@google.com on 2010-08-09 15:35:57

blickly commented 9 years ago
Closing this.  Individual optimizations should be filed as individual issues as Enhancements.
 Inappropriate ones will be closed, small easy ones with obvious wins will be labelled
as "Starter Project" (or equivalent), etc.

Original issue reported on code.google.com by concavelenz on 2010-10-01 15:29:13