new Function() does not use GNAME opcodes

RESOLVED FIXED in mozilla23

Status

()

Core
JavaScript Engine
--
minor
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: jorendorff)

Tracking

(Blocks: 3 bugs)

Trunk
mozilla23
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 9 obsolete attachments)

18.57 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
v11
10.79 KB, patch
luke
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
js> dis(new Function(""))
flags: LAMBDA
...

js> dis(function(){})
flags: LAMBDA NULL_CLOSURE
...
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?
Attachment #552747 - Flags: review?
(Reporter)

Updated

6 years ago
Attachment #552747 - Flags: review? → review?(jorendorff)
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.)
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
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.
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?
(Assignee)

Comment 6

6 years ago
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.
Summary: new Function() never gets the NULL_CLOSURE optimization flag → new Function() does not use GNAME opcodes
(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
Blocks: 608733
(Assignee)

Updated

6 years ago
Attachment #552747 - Flags: review?(jorendorff)
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.
Attachment #552747 - Attachment is obsolete: true
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
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.
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".
Attachment #554546 - Attachment is obsolete: true
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.
Attachment #554559 - Attachment is obsolete: true
Attachment #557235 - Flags: review?(dmandelin)
Attachment #557235 - Flags: review?(dmandelin) → review+
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.

Updated

6 years ago
Duplicate of this bug: 659941
Requesting tracking-firefox9. See bug 659941 comment 4.
Blocks: 619423
tracking-firefox9: --- → ?
Keywords: testcase → checkin-needed
In my queue :-)
Assignee: general → terrence
Status: NEW → ASSIGNED
Flags: in-testsuite+
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86_64 → All
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!
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!
(Assignee)

Comment 19

6 years ago
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.
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.
(Assignee)

Comment 21

6 years ago
(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.
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.
(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.
(Assignee)

Comment 24

6 years ago
(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.
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.
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.
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?
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.
(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.
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?
Attachment #557235 - Attachment is obsolete: true
Depends on: 688646
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.
Attachment #562537 - Flags: review?(bhackett1024)
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?
Attachment #562537 - Flags: review?(bhackett1024) → review+
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..
Attachment #559352 - Attachment is obsolete: true
Attachment #562540 - Flags: review?(jorendorff)
(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
tracking-firefox9: ? → +
(Assignee)

Comment 35

6 years ago
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.
Attachment #562540 - Flags: review?(jorendorff) → review+
Blocks: 694143
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.
Would things work if 'new Function()' only made a compileAndGo script if called from a script which itself is compileAndGo?
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.
How's this going? If we want it for 9, we only have a week left. How important is it?
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.
tracking-firefox9: + → ---
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).
Attachment #562537 - Attachment is obsolete: true
Attachment #562540 - Attachment is obsolete: true
Attachment #599665 - Flags: review?(jorendorff)
(Assignee)

Comment 42

5 years ago
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.
Attachment #599665 - Flags: review?(jorendorff) → review+
Will this be able to land soon? I'd be very interested to see it happen.

Thanks,
Dave
(Assignee)

Comment 44

5 years ago
Note that we now have the shell function I asked for in comment 42:
  evaluate(code, {compileAndGo: false})

Updated

5 years ago
Blocks: 804933
Brian, can we rebase and land this?
(Assignee)

Comment 46

5 years ago
Created attachment 676802 [details] [diff] [review]
v9

The old patch was completely bitrotted; nothing could be saved. This is from scratch.
Assignee: terrence → jorendorff
Attachment #676802 - Flags: review?(n.nethercote)
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?
Attachment #676802 - Flags: review?(n.nethercote)
(Assignee)

Comment 48

5 years ago
(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

4 years ago
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.
Blocks: 861071
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.

Updated

4 years ago
Duplicate of this bug: 861071

Comment 52

4 years ago
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.
Attachment #676802 - Attachment is obsolete: true
Attachment #745442 - Flags: review?(luke)

Comment 53

4 years ago
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.
Attachment #745442 - Attachment is obsolete: true
Attachment #745442 - Flags: review?(luke)
Attachment #745447 - Flags: review?(luke)

Comment 54

4 years ago
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.
Attachment #745447 - Flags: review?(luke) → review+

Comment 55

4 years ago
(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 56

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed26fdbe8444

try: https://tbpl.mozilla.org/?tree=Try&rev=2f24b2f8df6a
https://hg.mozilla.org/mozilla-central/rev/ed26fdbe8444
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Depends on: 879723
Depends on: 891156
Depends on: 902744
No longer depends on: 902744
You need to log in before you can comment on or make changes to this bug.