Last Comment Bug 646597 - new Function() does not use GNAME opcodes
: new Function() does not use GNAME opcodes
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- minor with 3 votes (vote)
: mozilla23
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
: 659941 861071 (view as bug list)
Depends on: 688646 879723 891156
Blocks: jsfunfuzz 619423 804933 608733 694143 861071
  Show dependency treegraph
 
Reported: 2011-03-30 12:45 PDT by Jesse Ruderman
Modified: 2013-08-08 10:29 PDT (History)
21 users (show)
emorley: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Mark (new Function()) as NULL_CLOSURE when appropriate. (1.30 KB, patch)
2011-08-12 13:24 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Review
Setup compilation such that getgname gets used instead of name opcode. (1.66 KB, patch)
2011-08-19 14:09 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Review
Provide the current globals and mark for compile-and-go on new Function() compilation. (1001 bytes, patch)
2011-08-19 14:57 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Review
Provide the global object and mark Functions as compile-n-go when compiling. (1.94 KB, patch)
2011-08-31 10:41 PDT, Terrence Cole [:terrence]
dmandelin: review+
Details | Diff | Review
Set compile-n-go mode when we have a global object in compileFunctionBody. (2.10 KB, patch)
2011-09-08 17:54 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Review
Removes tests that use clone on a Function. (10.13 KB, patch)
2011-09-26 14:32 PDT, Terrence Cole [:terrence]
bhackett1024: review+
Details | Diff | Review
Make Functions as Compile-n-Go so that we use GNAME opcodes. (2.35 KB, patch)
2011-09-26 14:44 PDT, Terrence Cole [:terrence]
jorendorff: review+
Details | Diff | Review
updated (8610a9da1742) (18.57 KB, patch)
2012-02-22 09:52 PST, Brian Hackett (:bhackett)
jorendorff: review+
Details | Diff | Review
v9 (8.56 KB, patch)
2012-10-30 15:17 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
v10 (11.07 KB, patch)
2013-05-03 18:01 PDT, Shu-yu Guo [:shu]
no flags Details | Diff | Review
v11 (10.79 KB, patch)
2013-05-03 18:08 PDT, Shu-yu Guo [:shu]
luke: review+
Details | Diff | Review

Description Jesse Ruderman 2011-03-30 12:45:38 PDT
js> dis(new Function(""))
flags: LAMBDA
...

js> dis(function(){})
flags: LAMBDA NULL_CLOSURE
...
Comment 1 Terrence Cole [:terrence] 2011-08-12 13:24:09 PDT
Created attachment 552747 [details] [diff] [review]
Mark (new Function()) as NULL_CLOSURE when appropriate.

No performance change on SunSpider (which doesn't use (new Function())).  The attached test will only fail on debug builds: is there a better way to implement this test?
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-16 09:12:20 PDT
Well, Function(code) per spec creates a function in the global scope, so code can only have dependencies on the global object, never upon upvars.  As I recall (I can find no clear confirmation of this in null-closure documentation in jsfun.h), null closures are permitted to depend upon the global object.  So I think every function created by Function can and should be marked as a null closure.

That's my two cents going from memory.  You should confirm with people more certain of their knowledge before deciding this is the absolute right course of action.  (And while ordinarily I'd be happy to review a patch, two things require I ask you not do that here.  First, I'm not quite certain of my understanding.  And second, I'll be gone for a couple weeks very shortly, and I don't want to hold up your patch while I'm gone.)
Comment 3 Jason Orendorff [:jorendorff] 2011-08-16 10:33:44 PDT
Terrence, thanks for the patch. This looks correct to me (except for the |jit-test| comment in the test, which I will just delete).

But it's hard to be sure. Here's the kind of thing that bothers me:

  function f(x) {
      eval(x);
  }
  dis(f);  // not a null closure. why not?

If there is a reason this function can't be a null closure, then probably the same reasoning applies to Function("x", "eval(x)").

I need to be know more about scoping in order to do JS modules anyway. So I'm going to take this opportunity to review all the JSFUN_NULL_CLOSURE code and improve the comment in jsfun.h.

So, stay tuned. It won't take long, but I won't finish today.
Comment 4 Jason Orendorff [:jorendorff] 2011-08-17 17:08:27 PDT
I'm writing a patch for bug 669369 to simplify Parser::setFunctionKinds, the code where we decide if a function can be a null closure or not, for all functions other than the ones at issue in this bug.

I still have to read some more code before I can complete this review.
Comment 5 Terrence Cole [:terrence] 2011-08-18 00:22:01 PDT
Jeff, Jason, thank you both for taking a look.  I'm still quite hesitant to ask a specific person for a review since I'm not sure where exactly such a request sits on the request/demand spectrum.

As to eval, good catch!  I think any code that calls eval cannot be a null closure.  Consider:
////////
var x = 24;
function g() {
    var xx = 42;
    return function(some_code) {
        eval(some_code);
    }
}
f = g();
dis(f);
f("print(x);");
f("print(xx);");
f("var xxx=84; print(xxx);");
////////

The patch I submitted is broken as is, because the equivalent Function is still marked as a null closure, causing us to crash horribly when we access xx on the now non-existing frame for g.  It's not clear to me yet why the code above isn't marked as a null closure as well, so clearly I'm still missing something.  Both f are marked as HEAVYWEIGHT, however -- I think this is probably a good place to start looking.

Perhaps it would be best to abstract setFunctionKinds all the way out of the parser so that it can be used for Function() functions as well?
Comment 6 Jason Orendorff [:jorendorff] 2011-08-18 12:18:24 PDT
It turns out there's no benefit of marking Function() functions as null closures after compilation.

The null closure bit affects JSOP_DEFFUN/DEFLOCALFUN/LAMBDA, i.e. all the other ways of creating functions. It also affects one other optimization that also isn't relevant here (the method optimization).

However, we do pass up optimization opportunities with Function:

js> var x = 1;
js> function f() { return x; }
js> dis(f)
flags: NULL_CLOSURE
00000:  getgname "x"
00003:  return
00004:  stop

js> dis(function () { return x; })
flags: LAMBDA NULL_CLOSURE
00000:  getgname "x"
00003:  return
00004:  stop

js> var g = Function("return x;");
js> dis(g)
flags: LAMBDA
00000:  name "x"
00003:  return
00004:  stop

Changing this bug's summary to focus on that.
Comment 7 Jan de Mooij [:jandem] 2011-08-18 12:32:13 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #6)
> However, we do pass up optimization opportunities with Function:

FWIW, this affects the trace compiler in bug 608733 (the benchmark is also on AWFY assorted as misc-bugs-608733-trace-compiler) The trace is created using "new Function" and uses global variables for the PC, memory etc
Comment 8 Terrence Cole [:terrence] 2011-08-19 14:09:54 PDT
Created attachment 554546 [details] [diff] [review]
Setup compilation such that getgname gets used instead of name opcode.

According to ECMA-262 15.3.2.1 step 11, using the Function constructor always sets the scope to the Global Environment.  We should be able to emit getglobal "x" in Jason's example.  As of this morning, both of the functions above it also emit getglobal "x" (rather than getgname "x").

The reason that compilation with with Function() emits full name accesses is that TryConvertToGName checks if the context is in compileAndGo mode and that a global scope is set, both of which are false.  At the very least we need to set the global scope when compiling.  I do not know if the compileAndGo check is correct; I think it is only used to protect access to the null globalScope.

If we relax this check and set a global scope, we end up emitting getgname "x" instead of name "x" -- a partial success.  I'm sure there is a better way to do this -- I'm dropping this patch here so others can give me some feedback.
Comment 9 Brendan Eich [:brendan] 2011-08-19 14:35:35 PDT
Catching up on bugmail, missed this one till now.

Function() should use the compile-and-go option, since it creates an object scoped by a specific global object. That is what compile-and-go signifies: that the script (function or top-level) to compile will be executed with at most one global, which is presented to the compiler.

This is in contrast to pre-compiling and then executing against many globals, where one must not set compile-and-go and the compiler's global is null.

/be
Comment 10 Terrence Cole [:terrence] 2011-08-19 14:47:20 PDT
Thanks for the explanation!  The comment next to TCF_COMPILE_N_GO was less than helpful.  I'll see how much that new knowledge simplifies things.
Comment 11 Terrence Cole [:terrence] 2011-08-19 14:57:40 PDT
Created attachment 554559 [details] [diff] [review]
Provide the current globals and mark for compile-and-go on new Function() compilation.

This still emits getgname "x", rather than getglobal "x".
Comment 12 Terrence Cole [:terrence] 2011-08-31 10:41:51 PDT
Created attachment 557235 [details] [diff] [review]
Provide the global object and mark Functions as compile-n-go when compiling.

When parsing, we mark top-level variables as GVAR on the parse node.  When we parse references to a variable, we emit getglobal if the parse node of the target is marked as a GVAR.  Naturally, when we get around to compiling the Function text at run time, the parse nodes for the global vars we are accessing are long since cleaned up.  Thus, we cannot emit getglobal.  

Without a major refactoring of the way we compile (i.e. IonMonkey) we will probably never be able to do better than getgname in a Function (or eval) context.  Thus, I think we should just apply the attached patch to make Function compile-n-go and put off further enhancement until the landscape has changed in a few months.
Comment 13 David Anderson [:dvander] 2011-08-31 11:02:05 PDT
Ideally we want to remove GETGLOBAL entirely and have compilers be smarter about figuring out whether the lookup can be cached, so the parser doesn't have to do that work. So GETGNAME is fine.
Comment 14 Jan de Mooij [:jandem] 2011-08-31 11:14:23 PDT
*** Bug 659941 has been marked as a duplicate of this bug. ***
Comment 15 Jan de Mooij [:jandem] 2011-08-31 11:16:31 PDT
Requesting tracking-firefox9. See bug 659941 comment 4.
Comment 16 Ed Morley [:emorley] 2011-08-31 15:09:55 PDT
In my queue :-)
Comment 17 Ed Morley [:emorley] 2011-09-01 00:24:23 PDT
Ok, sorry for the delay - the first try run was busted by bug 682677's patch, and now the second has failed as well, but I think this bug is to blame:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=ddfa68e986d8

I get a bunch of errors along the lines of "JavaScript error: , line 0: bad cloned function scope chain", and then:
TEST-UNEXPECTED-FAIL | automation.py | Exited with code -2147483645 during test run
PROCESS-CRASH | automation.py | application crashed (minidump found) 

Please can you make sure this builds & also run the tests locally (or send to try), before requesting checkin-needed again. Also, when the updated patch is upload, can you include the bug number in the commit message and the reviewer (in the format: "Bug 123456 - Message; r=foo"). Bonus points for adding |nodates = True| to your hgrc under [diff], so that things like the hg pushlog display the commit time and not an incorrect date, and so I don't need to manually remove that line before importing :-)

Thanks!
Comment 18 Terrence Cole [:terrence] 2011-09-01 09:27:01 PDT
You are correct, this is the offending patch.  I seem to have badly misunderstood the environment in which tests run on the tinderbox :-).  The new globals/compartments work also appears to have also impacted this bug heavily and I need to track down where exactly our globals should come from now.

Also, thank you for the detailed instructions on submitting patches; I promise I'll do a better job in future!
Comment 19 Jason Orendorff [:jorendorff] 2011-09-01 09:59:08 PDT
js/src/jsapi-tests/testCloneScript.cpp fails with the patch, for two reasons:

First, this affects JS_CompileFunction as well as Function(). I think that's OK but I'm not totally sure.

Second, compile-and-go functions can't be cloned:

  js> clone(Function())
  typein:2: TypeError: bad cloned function scope chain

The change in behavior is probably fine, but I guess some tests will have to be fixed up.
Comment 20 Terrence Cole [:terrence] 2011-09-01 10:40:24 PDT
Losing the ability to clone Functions would make me sad.  In any case, I think the problem is a bit worse than that, really.

js/src/jit-test/tests/debug/onNewScript-0{1|2}.js also fails -- in this case we call Function on a newGlobal('new-compartment') object.  Not only am I not sure what globals we should be using with this Compile-n-Go, the system throws a TypeError when we try to compile because it explicitly checks we are not in compile-n-go mode.
Comment 21 Jason Orendorff [:jorendorff] 2011-09-02 09:02:11 PDT
(In reply to Terrence Cole from comment #20)
> Losing the ability to clone Functions would make me sad.

Why?

Cloning a function returned by Function() is something we should never do, don't worry about it.

> js/src/jit-test/tests/debug/onNewScript-0{1|2}.js also fails -- in this case
> we call Function on a newGlobal('new-compartment') object.  Not only am I
> not sure what globals we should be using with this Compile-n-Go, the system
> throws a TypeError when we try to compile because it explicitly checks we
> are not in compile-n-go mode.

Oh! Definitely not cx->globalObject. That's a bug in the patch. 

(In fact, approximately nothing should ever use cx->globalObject. The name is really misleading, which is our fault, not yours. A better name would be
cx->jorendorffWillPersonallyThrottleYouIfYouUseThis.alsoYourCodeWontWork.)

The Function constructor in jsfun.cpp already has a local variable 'global' that contains the correct global object. That's the one you want. You just need to give js::Compiler::compileFunctionBody an extra parameter and pass it in.

CompileUCFunctionForPrincipalsCommon in jsapi.cpp should pass GetGlobalForScopeChain(cx) for that parameter.
Comment 22 Terrence Cole [:terrence] 2011-09-04 12:27:08 PDT
NOTE: there was a malfunction.  If you are here looking for "Remove unneeded #ifdef DEBUG: use DebugOnly instead" the bug you are really looking for is 682717.
Comment 23 Terrence Cole [:terrence] 2011-09-04 12:46:42 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #21)
> (In reply to Terrence Cole from comment #20)
> > Losing the ability to clone Functions would make me sad.
> 
> Why?

Because I didn't know that was even theoretically possible before and now I want to try it.  Is f2=Function('foo='+f1.toString()+';foo();'); significantly less bonkers?
 
> Cloning a function returned by Function() is something we should never do,
> don't worry about it.
> 
> > js/src/jit-test/tests/debug/onNewScript-0{1|2}.js also fails -- in this case
> > we call Function on a newGlobal('new-compartment') object.  Not only am I
> > not sure what globals we should be using with this Compile-n-Go, the system
> > throws a TypeError when we try to compile because it explicitly checks we
> > are not in compile-n-go mode.
> 
> Oh! Definitely not cx->globalObject. That's a bug in the patch. 
> 
> (In fact, approximately nothing should ever use cx->globalObject. The name
> is really misleading, which is our fault, not yours. A better name would be
> cx->jorendorffWillPersonallyThrottleYouIfYouUseThis.alsoYourCodeWontWork.)

Guess I'd better buy some turtlenecks. :-)

> The Function constructor in jsfun.cpp already has a local variable 'global'
> that contains the correct global object. That's the one you want. You just
> need to give js::Compiler::compileFunctionBody an extra parameter and pass
> it in.

In retrospect, I should have found a per-context globals object weirder than I did.

> CompileUCFunctionForPrincipalsCommon in jsapi.cpp should pass
> GetGlobalForScopeChain(cx) for that parameter.

I'll take a look at it as soon as I get some time.
Comment 24 Jason Orendorff [:jorendorff] 2011-09-08 13:09:30 PDT
(In reply to Terrence Cole from comment #23)
> (In reply to Jason Orendorff [:jorendorff] from comment #21)
> > (In reply to Terrence Cole from comment #20)
> > > Losing the ability to clone Functions would make me sad.
> > 
> > Why?
> 
> Because I didn't know that was even theoretically possible before and now I
> want to try it.  Is f2=Function('foo='+f1.toString()+';foo();');
> significantly less bonkers?

No, the kind of cloning we're talking about here is not anything visible to JS code (apart from the shell-only clone function, which is just for testing). We should not keep this functionality, if it's standing in the way of making Function() functions fast.
Comment 25 Brian Hackett (:bhackett) 2011-09-08 13:52:59 PDT
I remember changing those tests which use clone() to call Function, instead of defining a normal global function like they used to.  Using JS_CloneFunctionObject on a compileAndGo script breaks a host of JM and TI assumptions and the tests were hitting assertions.  Using Function() was just a workaround though because they were the only way I know of to get non-compileAndGo code in the shell.  But that behavior was itself broken (and fixed in this bug).  I think we should disable these tests for now, wait for all this business to get fixed by compartment-per-global (which should make everything compileAndGo) and then reenable the tests.
Comment 26 Terrence Cole [:terrence] 2011-09-08 16:36:03 PDT
The globals object that jorendorff refers to is attached to fun as parent in the Function constructor.  I believe we can get it back out safely through fun->getGlobal() without having to add a new parameter to compileFunctionObject.  Doing this, we fail on 5 tests:
    js1_5/extensions/regress-300079.js
    js1_5/Function/15.3.4.4.js
    js1_5/Regress/regress-127557.js
    js1_8_5/extensions/findReferences-01.js
    js1_8_5/extensions/findReferences-02.js

regress-300079.js and regress-127557.js fail with "TypeError: bad cloned function scope chain" when they try to clone.  Consensus is these are busted.

15.3.4.4.js fails because its_bindMethod in shell/js.cpp specifically asserts that the callable it is passed is not compile-n-go.  I'm not sure why its_bindMethod has that assertion, so I hope someone will comment on that.

js1_8_5/extensions/findReferences-01.js and js1_8_5/extensions/findReferences-02.js fail with 
"""
referent is not referenced via: "edge: type_proto"
but it is referenced via:       ["edge: type; type_proto"]
"""
and
"""
referent is not referenced via: "edge: parent; type_proto"
but it is referenced via:       ["edge: parent; **UNKNOWN SLOT 1**", "edge: parent; type; type_proto"]
"""
respectively.  These tests work without this change -- I will need to investigate further why they are falling over with those particular errors.
Comment 27 Terrence Cole [:terrence] 2011-09-08 17:12:32 PDT
The 15.3.4.4.js failure is not entirely spurious: its_bindMethod calls JS_SetParent, which would indeed break all compile-n-go optimizations.  However, this appears to be the only test that calls bindMethod.  Not sure how to proceed.

There are an additional 2 tests in jit-tests that fail when they call clone.
	debug/onNewScript-01.js
	debug/onNewScript-02.js

Grepping in SunSpider and v8 did not find any Function usage.  Does anyone know of an existing (micro)benchmark that uses Function?
Comment 28 Brian Hackett (:bhackett) 2011-09-08 17:25:55 PDT
I don't know why the findReferences tests pass for you without the patch, but these tests don't run in the browser and have been failing in the shell since the TI merge.  I fixed them once before merging TI, but then they broke again with no notification because they are not covered by any automated tests.  Filed bug 685751 to get this stuff fixed.
Comment 29 Brian Hackett (:bhackett) 2011-09-08 17:30:21 PDT
(In reply to Terrence Cole from comment #27)
> Grepping in SunSpider and v8 did not find any Function usage.  Does anyone
> know of an existing (micro)benchmark that uses Function?

A good one to look at is the shell test in bug 608733.  This is a slick program that emulates a CPU and 'compiles' traces by generating code and then calling Function().  The lack of compileAndGo-ness really hurts JM here.
Comment 30 Terrence Cole [:terrence] 2011-09-08 17:54:01 PDT
Created attachment 559352 [details] [diff] [review]
Set compile-n-go mode when we have a global object in compileFunctionBody.

Timing on roc's emu.
Before: 1655 ms
After:  1037 ms

Initial patch doesn't address the failing tests.  What should I do with 15.3.4.4.js and the tests using clone, in particular?
Comment 31 Terrence Cole [:terrence] 2011-09-26 14:32:43 PDT
Created attachment 562537 [details] [diff] [review]
Removes tests that use clone on a Function.

I split the test removals out into this separate patch so that we can revert it easily later after CPG.
Comment 32 Brian Hackett (:bhackett) 2011-09-26 14:35:22 PDT
Comment on attachment 562537 [details] [diff] [review]
Removes tests that use clone on a Function.

Can you file a followup bug to keep track of the test reinsertion?
Comment 33 Terrence Cole [:terrence] 2011-09-26 14:44:36 PDT
Created attachment 562540 [details] [diff] [review]
Make Functions as Compile-n-Go so that we use GNAME opcodes.

This should be baking in the correct global object now.  The code path we care about here sets the passed in globals as the function object's globals.  We then pull them off the function object in compileFunctionBody.  I do not believe there are other paths that set those globals that would not want to be compiled as compile-n-go.  CompileUCFunctionForPrincipalsCommon, for instance, does not set them on the function object..
Comment 34 Terrence Cole [:terrence] 2011-09-26 15:43:01 PDT
(In reply to Brian Hackett from comment #32)
> Comment on attachment 562537 [details] [diff] [review] [diff] [details] [review]
> Removes tests that use clone on a Function.
> 
> Can you file a followup bug to keep track of the test reinsertion?

https://bugzilla.mozilla.org/show_bug.cgi?id=689343
Comment 35 Jason Orendorff [:jorendorff] 2011-10-10 15:29:07 PDT
Comment on attachment 562540 [details] [diff] [review]
Make Functions as Compile-n-Go so that we use GNAME opcodes.

fun->getGlobal() never returns NULL. So remove these four lines and reindent:

>     GlobalObject *global = fun->getGlobal();
>-    if (global) {
>         GlobalScope globalScope(cx, global, &funcg);
>         funcg.setScopeChain(global);
>         compiler.globalScope = &globalScope;
>         funcg.flags |= TCF_IN_FUNCTION | TCF_COMPILE_N_GO;
>-    } else {
>-        funcg.flags |= TCF_IN_FUNCTION;
>-    }

r=me with that fixed.
Comment 36 Terrence Cole [:terrence] 2011-10-12 16:03:00 PDT
This patch, as it stands, does not work in the full browser because it effectively breaks JS_CloneFunctionObject for many paths we care about.  The explicit check for compile-n-go on the function fails now for these users:

  1) Attaching event handlers to chrome DOM.  In this case, the passed parent == funobj->parent, so we could ignore the explicit compile-n-go check fairly safely.
  2) nsXBLProtoImplProperty::InstallMember, which specifically wants to attach a different parent to the new function.  There are several hundred calls here that we cannot easily sweep under the rug.
  3) There are some other failures inside JS_CloneFunctionObject, but it is not clear if they are caused by prior clone failures from the first two sources.
Comment 37 Brian Hackett (:bhackett) 2011-10-12 16:33:49 PDT
Would things work if 'new Function()' only made a compileAndGo script if called from a script which itself is compileAndGo?
Comment 38 Terrence Cole [:terrence] 2011-10-12 19:15:50 PDT
I think this will work, I'm just having trouble finding the right flag.  The scripts we compile from the shell are marked as compile-n-go, but I'm not sure how to get to that script from the global object.  My first try looks like this:

    GlobalObject *global = fun->getGlobal();
    if (global->getFunctionPrototype()->getFunctionPrivate()->script()->compileAndGo) {

From the performance numbers and my tracing, it's pretty clear that the actual script object never gets set on the global, at least not in this spot.  I will look at this more tomorrow, but if anyone knows the correct way to get the global script from the globals and feels inclined to give me a hint, my sanity and I would be most appreciative.
Comment 39 David Mandelin [:dmandelin] 2011-11-01 17:29:17 PDT
How's this going? If we want it for 9, we only have a week left. How important is it?
Comment 40 Terrence Cole [:terrence] 2011-11-02 11:34:51 PDT
I think this is fairly low impact: it's only on new Function code, and since it breaks chrome code fairly extensively, we have to strongly constrain where we can use it.

The two options we have to make it work at all are: only enable compile-n-go for Function objects created in compile-n-go scripts or only enable when we are in content code.  It was not obvious in the few minutes I spent looking how to answer either of these questions -- I've been spending my time on the higher impact work in the GC.

I'll see what I can do before Tuesday, but even if it doesn't land before then, I believe Brian has plans to eliminate Compile-n-Go completely once CPG lands.  Once that happens, this functionality will irrelevant/always-on.
Comment 41 Brian Hackett (:bhackett) 2012-02-22 09:52:14 PST
Created attachment 599665 [details] [diff] [review]
updated (8610a9da1742)

Updated patch that passes mochitests etc.  I'm unsure now about removing these tests, is there another way to test what they're trying to do?  There really isn't any way I know of to get non-CNG code in the shell, maybe the thing to do here is allow the shell 'clone' method to clone CNG scripts (but not JS_CloneFunctionObject).
Comment 42 Jason Orendorff [:jorendorff] 2012-03-07 10:56:33 PST
Comment on attachment 599665 [details] [diff] [review]
updated (8610a9da1742)

If we still clone functions, we need to be able to test that. That is, we need a way to create cloneable functions. We just need a shell function that runs a script as non-compile-n-go code.

If we don't clone functions, by all means, blow away the tests.

In jit-test/tests/debug/onNewScript-01.js:
> // cloning functions across compartments
> var g2 = newGlobal('new-compartment');
> dbg.addDebuggee(g2, dbg);
> hits = 0;
>-g2.clone(fn);
>+fn = g2.Function("a", "return 5 + a;");
> assertEq(hits, 1);

If we have no way to clone functions in the shell, might as well delete this whole chunk of code. The rest of the test functions fine (and is still useful) without it.

r=me with that.
Comment 43 Dave Herman [:dherman] 2012-07-27 15:32:31 PDT
Will this be able to land soon? I'd be very interested to see it happen.

Thanks,
Dave
Comment 44 Jason Orendorff [:jorendorff] 2012-07-27 15:32:57 PDT
Note that we now have the shell function I asked for in comment 42:
  evaluate(code, {compileAndGo: false})
Comment 45 Jan de Mooij [:jandem] 2012-10-24 10:31:33 PDT
Brian, can we rebase and land this?
Comment 46 Jason Orendorff [:jorendorff] 2012-10-30 15:17:42 PDT
Created attachment 676802 [details] [diff] [review]
v9

The old patch was completely bitrotted; nothing could be saved. This is from scratch.
Comment 47 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-10-30 15:24:35 PDT
Comment on attachment 676802 [details] [diff] [review]
v9

Review of attachment 676802 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I'm not the right person for this review -- I barely understand compile-and-go.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +333,5 @@
>      if (!funpc.generateFunctionBindings(cx, bindings))
>          return false;
>  
>      BytecodeEmitter funbce(/* parent = */ NULL, &parser, funbox, script, /* callerFrame = */ NULL,
> +                           options.compileAndGo, options.lineno);

Why are you passing |options.compileAndGo| for the |hasGlobalScope| formal argument?
Comment 48 Jason Orendorff [:jorendorff] 2012-11-01 16:24:49 PDT
(In reply to Nicholas Nethercote [:njn] from comment #47)
> Sorry, I'm not the right person for this review -- I barely understand
> compile-and-go.

OK.

> >      BytecodeEmitter funbce(/* parent = */ NULL, &parser, funbox, script, /* callerFrame = */ NULL,
> > +                           options.compileAndGo, options.lineno);
> 
> Why are you passing |options.compileAndGo| for the |hasGlobalScope| formal
> argument?

hasGlobalScope is actually very simple. The one and only thing it controls is whether or not GNAME opcodes are emitted.

Maybe it would be better to use fun->environment()->isGlobal() here. I'll do that and repost.

This patch has another problem I didn't anticipate: it causes Function-constructed functions to run in Ion, which causes jit-test/tests/basic/expression-autopsy.js to fail. I don't think there is a straightforward way to make it pass. Not sure what to do. decompiler_tco++ from beyond the grave. :-P
Comment 49 K. Gadd (:kael) 2013-03-19 03:09:43 PDT
After a discussion in #jsapi, it sounds like it would be incredibly valuable for me if this were to land. To summarize:

If you run the test case from bug #852421 in a debug jsshell with IONFLAGS=aborts set, it turns out compiles are aborting all over the place with the reason string 'not compile-and-go'. Some of these compiles are for relatively hot functions in the test case.

Some digging around suggests that new Function tags every function it constructs as not-compile-and-go, and this propagates to child functions. In my case, I have a pattern where I use new Function to lazily initialize specialized constructors for my types based on information available at runtime. This seems to be resulting in all of those constructors being not-compile-and-go, which results in aborts. I can't know what the real performance consequences of this are, but it seems reasonable to assume that there are performance consequences from aborts.

I condensed a demonstration of this into something you can paste into a debug jsshell with IONFLAGS=aborts set, as you can see in this pastebin:

http://www.pastebin.mozilla.org/2227846

The pastebin first sets up two (functionally) identical 'creator functions', that take argument values and return a function instance which has closed over the values. A creator is constructed using eval, and another is constructed using new Function. the eval generates an 'eval frame' abort, as you'd expect.

The important part is at the bottom, where evaldInnerFunction and newdInnerFunction are created by calling the creator functions and passing in closure values. In this case, you can see that creating the closure does not produce an abort if it went through the eval path, but creating the closure through the new Function path generates an abort because the closure has also been tainted with 'not-compile-and-go' status.

At first I thought perhaps I could use eval to work around this, but aside from the fact that eval is nasty, the other implications of eval mean that the resulting functions are slower, even if they get compiled.

Let me know if I can provide more detail on why this is important for my use case. I'd certainly welcome a workaround, but it doesn't seem like there is one.
Comment 50 Jan de Mooij [:jandem] 2013-04-12 06:24:42 PDT
Jason, do you have time to land this in the coming weeks? If not I can try to rebase and get this landed, we should really be able to Ion-compile these functions.
Comment 51 Jan de Mooij [:jandem] 2013-04-12 12:08:12 PDT
*** Bug 861071 has been marked as a duplicate of this bug. ***
Comment 52 Shu-yu Guo [:shu] 2013-05-03 18:01:18 PDT
Created attachment 745442 [details] [diff] [review]
v10

Taking this since it's hurting Shumway performance.

Changes since jorendorff's v9:

 - Removed jit-test/tests/auto-regress/bug677977.js for being a fuzztest that tries to clone a Function ctor'd function.
 - Adds a |jit-test| flag 'no-ion' so expression-autopsy.js passes.
 - Checks fun->environment() && fun->environment()->isGlobal() for hasGlobalScope.
Comment 53 Shu-yu Guo [:shu] 2013-05-03 18:08:35 PDT
Created attachment 745447 [details] [diff] [review]
v11

I was hasty in straight out removing bug677977.js. This version replaces call to Function with evaluate like the other tests.
Comment 54 Luke Wagner [:luke] 2013-05-03 18:16:46 PDT
Comment on attachment 745447 [details] [diff] [review]
v11

hah, I assumed this was some big gnarly patch which is why it kept not being landed.

One question: in the test "fun->environment() && fun->environment()->isGlobal()", how can fun->environment() be NULL?  I know it can be for compiler-generated uncloned functions, but this function we're returning is a real callable function and fun->environment() is to be its scope when called.
Comment 55 Shu-yu Guo [:shu] 2013-05-03 18:47:52 PDT
(In reply to Luke Wagner [:luke] from comment #54)
> Comment on attachment 745447 [details] [diff] [review]
> v11
> 
> hah, I assumed this was some big gnarly patch which is why it kept not being
> landed.
> 
> One question: in the test "fun->environment() &&
> fun->environment()->isGlobal()", how can fun->environment() be NULL?  I know
> it can be for compiler-generated uncloned functions, but this function we're
> returning is a real callable function and fun->environment() is to be its
> scope when called.

For posterity, the reason, as explained to me by bz, is that event handlers (from nsEventListenerManager::CompileEventHandlerInternal) are compiled by passing in a NULL 'obj' argument to JS::CompileFunction. The compiled function is never used, but is instead immediately cloned onto the right scope chain.
Comment 57 Phil Ringnalda (:philor) 2013-05-05 17:18:54 PDT
https://hg.mozilla.org/mozilla-central/rev/ed26fdbe8444

Note You need to log in before you can comment on or make changes to this bug.