Prototype virtualization doesn't happen inside wrapper function closure

[GoogleCodeExporter]

Not sure if this has been raised before.

The following:

  /** @constructor */
  var foo = function() {};

  foo.prototype.hello = function(x) { alert(x); }

  var xyz = new foo();
  xyz.hello("hello");

will virtualize the prototype method "hello" and inline the whole thing to:

  alert("hello");

However, if wrapped inside a function closure:

(function() {
  /** @constructor */
  var foo = function() {};

  foo.prototype.hello = function(x) { alert(x); }

  var xyz = new foo();
  xyz.hello("hello");
})();

The virtualization no longer happens, and we're left with:

(function() {
  function a() {
  }
  a.prototype.a = function() {
    alert("hello")
  };
  (new a).a()
})();

Is there any reason why prototype virtualization only happens outside a 
functional scope?

Original issue reported on code.google.com by schu...@gmail.com on 19 May 2011 at 2:39

[GoogleCodeExporter]

I think Alan had an idea on how to init prototypes without using a temporary 
variable.

Original comment by concavel...@gmail.com on 18 Aug 2011 at 4:43

• Added labels: Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

Thanks for taking a look at this.  A lot of coding style has prototypes defined 
within wrapper closures (for captured local variables and other nicities) and 
just exposing the class to the outside.  All these classes are not virtualized 
currently.

Original comment by schu...@gmail.com on 19 Aug 2011 at 1:21

[GoogleCodeExporter]

Wait. Are you thinking of something else, John?

What I had in mind was the PrototypeMemberExtraction. What he was suggesting 
requires simple definition finder to understand local namespaces....

Original comment by acle...@gmail.com on 20 Aug 2011 at 7:31

[GoogleCodeExporter]

Hi all, wonder if anybody is working on this issue?  A lot of libraries these 
days wrap class definitions in function closures, and this is really preventing 
a lot of optimizations -- unless the libraries are modified to move prototype 
definitions outside...

Original comment by schu...@gmail.com on 18 Nov 2011 at 5:05

[GoogleCodeExporter]

@schungx:

Can you give me examples of such libraries? I would like to have a look.

Original comment by acle...@gmail.com on 18 Nov 2011 at 5:14

[GoogleCodeExporter]

Well, jQuery for one.  Also earlier versions of Dojo have occasional class 
definitions with direct prototype sets -- although mostly they have moved into 
using dojo.extend's and dojo.declare's.

Other examples:

- The linq.js library: http://linqjs.codeplex.com/
- iScroll: http://cubiq.org/iscroll

Actually, if you look at JavaScript scripts out there, a lot these days start 
with (function() { and ends with })();

Original comment by schu...@gmail.com on 18 Nov 2011 at 5:28

[GoogleCodeExporter]

Outside of Closure-Library @schungx is correct - this is a common pattern. I 
would love to see this issue fixed.

At one point we had discussed removing anonymous function wrappers that were 
purely for scoping to allow other optimizations to run. However since this 
pattern can occur in nested scopes, that solution would only handle some of the 
cases.

Original comment by chadkill...@missouristate.edu on 18 Nov 2011 at 12:26

[GoogleCodeExporter]

Some how I thought this was for ExtractPrototypeMemberDeclaration to work with 
a wrapper.

So I see what the problem is. The simple name graph doesn't understand anything 
when the whole program is warped in a anonymous function.

I had a quick look at the libraries listed:

JQuery itself is another beast. Chad and I are have another discussion on this 
with the team so let us leave this out.

iScroll: declares prototype with: prototype = { a: ... } etc. I think 
AnalyizeProtoytypeProperty does understand that but not the simple name graph.

linq.js is an interesting one. It looks like there are some opportunities in 
there.

I have some ideas on how we can fix this. I'll experiment with it later today.

@schungx: BTW, do you have example of code *using* linq.js?

Original comment by acle...@google.com on 18 Nov 2011 at 7:16

[GoogleCodeExporter]

Actually, I do use linq.js regularly, but the codes are for proprietary apps.  
I can put some sniplets here though.

The linq.js library makes writing data-processing in JavaScript much less 
painful by allowing you to write data queries in Microsoft's LINQ format.  
However, it incurs overhead due to its layered structure.  That's where Closure 
adds tremendous value.  I have a modified version of linq.js (attached) that 
works great with Closure -- and Closure eliminates most of the performance 
penalties involved in using it.  In particular, I found prototype 
virtualization to be great in unwrapping the multiple layers of chaining.

I've sent my modifications to the author of linq.js, but not many people 
outside uses Closure and it doesn't really register with him.  So I constantly 
have to maintain my own version that syncs with the newest version he puts out. 
 IMHO, there are a lot of "quick-and-dirty" libraries or small scripts that are 
written with direct prototype manipulations (although the larger libraries all 
do them inside a simulated class declaration structure).  We don't really want 
the users of these scripts to constantly have to modify each new version just 
to make them compatible with Closure.

Some code sniplets:

Enumerable.FromArray(data["devices"])
    .Where(function(x) { return x["item"] === "T" })
    .Select(function(x) {
        x["properties"] = ihs.ListToObject(x["properties"], "property");
        return x;
    })
    .ToArray();

Enumerable.FromArray(data)
    .Where(function(x) { return x["id"] && x["value"] })
    .ForEach(function(zone) {
        dojo.publish(ihs.EVENTS.securityZoneChanged, [ zone["id"], zone["value"] ]);
    });

var exists = Enumerable.FromArray(fields).FirstOrDefault(null, function(x) { 
return x["key"] === devicekey && x["property"] === property });

var defzone = Enumerable.FromArray(ihs.dataStore.zones)
    .Select(function (x) { return x.Value })
    .Where(function(x) { return x["isSettable"] === undefined || x["isSettable"] === null || x["isSettable"] })
    .OrderBy(function(x) { return (x["sort"] === undefined || x["sort"] === null) ? ihs.MAX_SORT : x["sort"] })
    .Select(function(x) { return x["id"] })
    .FirstOrDefault(null);

Original comment by schu...@gmail.com on 19 Nov 2011 at 3:28

Attachments:

• linq.js

[GoogleCodeExporter]

Some more sniplets:

var pending = Enumerable.FromArray(ihs.network._pendingXHRs)
    .OrderByDescending(function(x) { return x.priority || 0 })
    .ThenBy(function(x) { return x.timestamp })
    .ToArray();

Enumerable.FromArray(ihs.dataStore.devices)
    .Select(function(x) { return x.Value })
    .Where(function(x) { return x["zone"] === ihs.currentZone.id })
    .Where(function(x) { return x["application"] && x["application"] === appl })
    .OrderBy(function(x) { return x["sort"] })
    .ForEach(function(dev) {...});

var devcount = Enumerable.FromArray(ihs.dataStore.devices)
    .Select(function(x) { return x.Value })
    .Where(function(x) { return x["zone"] === ihs.currentZone.id })
    .Where(function(x) { return x["application"] === appl })
    .Count();

var list = Enumerable.FromArray(this._childWidgets)
    .Where(function(x) { return x.get("value") !== null })
    .Where(function(x) { return x.devicelink })
    .Where(function(x) { return x.proplink && x.proplink["canSet"] })
    .OrderBy(function(x) { return (x.proplink["profileSort"] === undefined || x.proplink["profileSort"] === null) ? ihs.MAX_SORT : x.proplink["profileSort"] })
    .Select(function(widget) {
        return ele.getSaveProfileItem(widget);
    })
    .Where(function(x) { return x !== null })
    .ToArray();

Original comment by schu...@gmail.com on 19 Nov 2011 at 3:37

[GoogleCodeExporter]

Ok. I didn't some quick investigation on this. As far as enabling this 
optimization, there shouldn't be a huge issue.

It does change the semantics of the code a bit but it isn't violating anything 
more than the non-wrapped case. I have no idea if this will be a problem or not.

@schungx if I send you a patch, could you run it and verify if it still works?

Original comment by acle...@google.com on 21 Nov 2011 at 8:21

[GoogleCodeExporter]

No problem.  I'll love to test it and report back!  Thanks!

Original comment by schu...@gmail.com on 22 Nov 2011 at 3:53

[GoogleCodeExporter]

Actually if you just sync to head. Comment out this line:

http://code.google.com/p/closure-compiler/source/browse/trunk/src/com/google/jav
ascript/jscomp/DevirtualizePrototypeMethods.java#158

It should start to work on wrapped functions.

Original comment by acle...@gmail.com on 23 Nov 2011 at 7:00

[GoogleCodeExporter]

Sorry, I don't really do Java. Would it be possible for you to build a version 
without that inGlobalScope check?  Thanks!

Original comment by schu...@gmail.com on 24 Nov 2011 at 3:18

[GoogleCodeExporter]

Hi there, will the next version contain this change (or a flag to turn it on)?

Or a compiled version of the compiler with this change so that I can test it?

Thanks!

Original comment by schu...@gmail.com on 14 Jan 2012 at 2:36

[GoogleCodeExporter]

Finally! Installed Java compiler and tried out the modification (i.e. 
commenting out line 158 of DevirtualizePrototypeMethods.java).

Test code:

/** @constructor */
var foo = function() {};

(function() {
foo.prototype.bar = function(x) { alert(x); };
})();

var xyz = new foo();
xyz.bar("Hello");

Results:

test.js:9: ERROR - variable JSCompiler_StaticMethods_bar is undeclared
xyz.bar("Hello");
^

1 error(s), 0 warning(s), 86.7% typed
java.lang.RuntimeException: java.lang.RuntimeException: INTERNAL COMPILER ERROR.
Please report this problem.
Unexpected variable JSCompiler_StaticMethods_bar
  Node(NAME JSCompiler_StaticMethods_bar): test.js:9:0
xyz.bar("Hello");
  Parent(CALL): test.js:9:0
xyz.bar("Hello");

    at com.google.javascript.jscomp.Compiler.runCallable(Compiler.java:639)
    at com.google.javascript.jscomp.Compiler.runInCompilerThread(Compiler.java:584)
    at com.google.javascript.jscomp.Compiler.compile(Compiler.java:566)
    at com.google.javascript.jscomp.Compiler.compile(Compiler.java:525)
    at com.google.javascript.jscomp.AbstractCommandLineRunner.doRun(AbstractCommandLineRunner.java:719)
    at com.google.javascript.jscomp.AbstractCommandLineRunner.run(AbstractCommandLineRunner.java:332)
    at com.google.javascript.jscomp.CommandLineRunner.main(CommandLineRunner.java:871)
Caused by: java.lang.RuntimeException: INTERNAL COMPILER ERROR.
Please report this problem.
Unexpected variable JSCompiler_StaticMethods_bar
  Node(NAME JSCompiler_StaticMethods_bar): test.js:9:0
xyz.bar("Hello");
  Parent(CALL): test.js:9:0
xyz.bar("Hello");

    at com.google.javascript.jscomp.VarCheck.visit(VarCheck.java:159)
    at com.google.javascript.jscomp.NodeTraversal.traverseBranch(NodeTraversal.java:504)
    at com.google.javascript.jscomp.NodeTraversal.traverseBranch(NodeTraversal.java:497)
    at com.google.javascript.jscomp.NodeTraversal.traverseBranch(NodeTraversal.java:497)
    at com.google.javascript.jscomp.NodeTraversal.traverseBranch(NodeTraversal.java:497)
    at com.google.javascript.jscomp.NodeTraversal.traverseBranch(NodeTraversal.java:497)
    at com.google.javascript.jscomp.NodeTraversal.traverseRoots(NodeTraversal.java:304)
    at com.google.javascript.jscomp.NodeTraversal.traverseRoots(NodeTraversal.java:464)
    at com.google.javascript.jscomp.VarCheck.process(VarCheck.java:102)
    at com.google.javascript.jscomp.PhaseOptimizer$PassFactoryDelegate.processInternal(PhaseOptimizer.java:273)
    at com.google.javascript.jscomp.PhaseOptimizer$NamedPass.process(PhaseOptimizer.java:250)
    at com.google.javascript.jscomp.PhaseOptimizer.process(PhaseOptimizer.java:168)
    at com.google.javascript.jscomp.Compiler.optimize(Compiler.java:1761)
    at com.google.javascript.jscomp.Compiler.compileInternal(Compiler.java:674)
    at com.google.javascript.jscomp.Compiler.access$000(Compiler.java:70)
    at com.google.javascript.jscomp.Compiler$1.call(Compiler.java:569)
    at com.google.javascript.jscomp.Compiler$1.call(Compiler.java:566)
    at com.google.javascript.jscomp.Compiler$2.run(Compiler.java:611)
    at java.lang.Thread.run(Unknown Source)
Caused by: java.lang.IllegalStateException: Unexpected variable 
JSCompiler_StaticMethods_bar
    ... 19 more

Original comment by schu...@gmail.com on 6 Feb 2012 at 4:08

[GoogleCodeExporter]

So it seems that the prototype method has been successfully virtualized into a 
static method.  However, somehow that static method cannot be found when it was 
being accessed -- probably because it was declared inside a wrapper closure.

Original comment by schu...@gmail.com on 6 Feb 2012 at 4:10

[GoogleCodeExporter]

I think I found out where the error is.

The standard prototype virtualization mechanism rewrites (in rewriteDefinition) 
the statement "a.prototype.b = function(a, b, c) {...}" into "var b = 
function(self, a, b, c) {...}".

However, this doesn't work if the statement is wrapped inside a closure.  
Example:

(function() {
  a.prototype.b = function() {};
})();

gets turned into:

(function() {
  var b = function(self) {};
})();

The variable "b", instead of a global variable, will become a local variable 
inside the closure.  When it is time to call the virtualized function, it won't 
find it, thus the error.

A slight modification is to use the alternative rewrite rule "b = 
function(self,a,b,c) {...}" when defSite.inGlobalScope is false, thus causing b 
to become a global variable.  It may, however, break ES5 though...

Original comment by schu...@gmail.com on 6 Feb 2012 at 4:23

[GoogleCodeExporter]

I have attached a patched version of DevirtualizePrototypeMethods.java which 
successfully handles virtualization of prototype methods within function 
closures.  Major change positions are marked with // MOD (schungx)

It translates such statements into "b = function(...) {...}" as well as adding 
a var definition to the beginning of the global scope.

So far it has passed most of my trials, but I am not sure whether it will never 
break anything else...

Original comment by schu...@gmail.com on 6 Feb 2012 at 2:52

Attachments:

• DevirtualizePrototypeMethods.java

[GoogleCodeExporter]

The attached file adds the ability to devirtualize prototype methods when the 
prototype is set directly to an object literal.

For example:

foo.prototype = { bar:function(x,y,z){...}, baz:function(x,y,z){...}, abc:123 
... }

gets rewritten as:

foo.prototype = { abc:123 };
foo.prototype.bar = function(x,y,z){...};
foo.prototype.baz = function(x,y,z){...};

Then the pass gets run multiple times until all the methods devirtualized.

It seems to work fine with most of my tests, but again I am not sure if this 
will break anything subtle...  It shouldn't...

Original comment by schu...@gmail.com on 7 Feb 2012 at 4:36

Attachments:

• DevirtualizePrototypeMethods.java

[GoogleCodeExporter]

Dear all,

Do you think this mod can get into the regular release?  I've tested it with 
all my scripts that have prototype definitions within functional closure 
wrappers and so far I have not encountered any side effects...

Thanks!

Original comment by schu...@gmail.com on 26 Apr 2012 at 2:33

[GoogleCodeExporter]

The first step is a CLA:
http://code.google.com/p/closure-compiler/wiki/Contributors

Original comment by concavel...@gmail.com on 26 Apr 2012 at 3:44

[GoogleCodeExporter]

Thanks, great!

Read the wiki, and ran "ant test".  Got five failures, four of them are 
expected since this is added functionality: testNoRewriteIfNotInGlobalScope2, 
testNoRewritePrototypeObjectLiterals1, testNoRewritePrototypeObjectLiterals2, 
testRewriteImplementedMethodInObj.

However, they all come back with JSC_MISSING_LINE_INFO errors, indicating that 
the rewrites I did are probably missing line info.  Not sure how I can add 
those info to the new nodes...

One of them -- testNoRewriteNestedFunction -- I need to test whether the 
prototype definition is within a nested function body, but not within a 
function body that is immediately executed (i.e. a wrapper function closure).  
I am scratching my head now to do that...  Any pointers?

Thanks!

Original comment by schu...@gmail.com on 26 Apr 2012 at 4:41

[GoogleCodeExporter]

I ran this against jQuery and it didn't have any affect (not that it 
necessarily should have - I need to investigate further). But it didn't break 
anything either.

As for the JSC_MISSING_LINE_INFO errors, everytime you use an IR.xxx method to 
construct a new node, you should use .srcref(node) to set the line information.

For the other four failing unit tests, I suggest making changes so that they 
all succeed (and this may mean removing some of them because they are testing 
for situations that you now handle differently). Also, you'll want to add 
several of your own unit tests demonstrating exactly what you do and do not 
expect to happen.

You don't need to mark items with //MOD either - a diff highlights that very 
nicely. Comments are appreciated though.

Someone else will have to chime in on the best way to test for closure.

Original comment by chadkill...@missouristate.edu on 26 Apr 2012 at 2:52

[GoogleCodeExporter]

A unconditional immediately executed function should look like this:

SCRIPT                <-- the call must be unconditional so no other parent 
should be allowed
   CALL                <-- immediate execution, function must be the call target (the first child), not a parameter
       FUNCTION    <-- function
           BLOCK      <-- function body, the assignment must be unconditional so no other parent should be allowed
              EXPR_STMT   <-- The assignment should be unconditional so no other parent should be allowed
                  ASSIGN  <-- prototype assignment

So you can write a function that verifies that this structure exists using the 
node.getParent() calls.

Node assign = ...;
Node maybeExpr = assign.getParent();
if (maybeExpr.isExprStmt()) {
  Node maybeBlock = maybeExpr.getParent();
  ...
}

Original comment by concavel...@gmail.com on 26 Apr 2012 at 3:52

[GoogleCodeExporter]

I don't see that you have submitted a CLA?

Original comment by concavel...@gmail.com on 26 Apr 2012 at 3:54

[GoogleCodeExporter]

Original comment by concavel...@gmail.com on 26 Apr 2012 at 6:56

[GoogleCodeExporter]

Great.  I'll fix my code and retest.

On the lack of apparent changes in jQuery, it is expected because my addition 
only handles a very restricted set of cases, in the form of:

(function(){ class.prototype.foo = ... })();

and

class.prototype = { ..., ..., ... };

IMHO, direct prototype sets are mostly used only for small utility libraries -- 
the larger ones probably use a mixin function or a class-creation function. 
Many of the small utility libraries are wrapped up in functional closures for 
non-intrusive inclusion, thus my interest in this.

Original comment by schu...@gmail.com on 28 Apr 2012 at 5:15

[GoogleCodeExporter]

I've fixed the line info errors (new file attached).

I am actually wondering whether it is necessary to prevent optimization when 
the prototype method assignment is inside a function.  I have not been able to 
think of a scenario where the following:

... function() { class.prototype.foo = ... };

will cause different behavior if "foo" is devirtualized.  Same with 
conditionals -- why is devirtualization disallowed when it is inside a tree 
under conditional statements?

Original comment by schu...@gmail.com on 28 Apr 2012 at 6:02

Attachments:

• DevirtualizePrototypeMethods.java

[GoogleCodeExporter]

I would really like to look at this but I can not until I can verify that a CLA 
has been submitted.   Have you submitted it?  If so, is it under another 
account?

Original comment by concavel...@gmail.com on 30 Apr 2012 at 3:21

[GoogleCodeExporter]

Not yet. Sort of goofed off. Will do it immediately!  :-)

Original comment by schu...@gmail.com on 30 Apr 2012 at 3:30

[GoogleCodeExporter]

Just electronically signed the CLA. See if you can find it. Thanks!

Original comment by schu...@gmail.com on 30 Apr 2012 at 3:33

[GoogleCodeExporter]

I see it under a different account.

Original comment by concavel...@gmail.com on 30 Apr 2012 at 6:05

[GoogleCodeExporter]

Oops. I used a different email account. I've put in a CLA with 
schungx@gmail.com.

Original comment by schu...@gmail.com on 1 May 2012 at 1:44

[GoogleCodeExporter]

got it thanks.

Original comment by concavel...@gmail.com on 1 May 2012 at 8:46

[GoogleCodeExporter]

You patch proposed two changes:
1) to allow rewriting of methods defined on object literals directly assigned 
to prototypes.  This is find, but the method used is unnecessarily complex.  I 
put out a change to do this in a simpler manner.

2) to allow the rewriting of prototypes methods defined outside global scope.  
Here we want to limit this to functions immediately and unconditionally called 
in global scope otherwise we are broadening the already strong assumptions this 
pass is making.

NodeUtil.isExpressionUnconditionallyExecuted will help here.

Also, before this patch can be accepted additional test cases covering the new 
functionality will be needed.

Original comment by concavel...@gmail.com on 8 May 2012 at 6:24

[GoogleCodeExporter]

Thanks! How can I see your change so I can learn how to do #1 simpler?

As for #2, I'll check out NodeUtil.isExpressionUnconditionallyExecuted.

I'll look into writing a few tests, but then I'll need to find out how the 
tests are structured.  So much to learn...

Original comment by schu...@gmail.com on 9 May 2012 at 4:30

[GoogleCodeExporter]

With the tests, just add a new test function in the appropriate tests file 
(usually mirrors the source). Other tests are about all the example you need. 
If you use eclipse, you can simply right click on the test in the outline 
window and run or debug it - it's very handy.

Original comment by chadkill...@missouristate.edu on 9 May 2012 at 1:36

[GoogleCodeExporter]

Recent related changes:
object literal support for devirtualize methods: 
http://code.google.com/p/closure-compiler/source/detail?r=1963
isExpressionConditionallyExecuted: 
http://code.google.com/p/closure-compiler/source/detail?r=1961 (renamed 
isExecutedExactlyOnce)

Original comment by concavel...@gmail.com on 9 May 2012 at 10:53

[GoogleCodeExporter]

I wonder if anyone wants to finish this off?

Original comment by concavel...@gmail.com on 13 Mar 2014 at 9:28

[GoogleCodeExporter]

Issue tracking has been migrated to github. Please make any further comments on 
this issue through https://github.com/google/closure-compiler/issues

Original comment by blic...@google.com on 1 May 2014 at 6:31

• Added labels: Restrict-AddIssueComment-Nobody

[GoogleCodeExporter]

Original comment by blic...@google.com on 1 May 2014 at 6:34

• Changed state: Duplicate