Our current JSOP_THIS implementation is pretty complicated, due to the lazy "this computation". I think this would be simpler: * Every function that uses |this| or eval has a JSOP_CREATETHIS in the prologue. This op stores the "computed" |this| value in the frame's this-slot. * Then, every JSOP_THIS simply loads from that slot. There are a few cases to consider: (1) Often, functions that use |this| are called with a this-value that is already an object. Ion should know this statically and JSOP_CREATETHIS is a no-op. (2) If |this| is undefined, we need to use the global object's outer-object hook. This is a bit expensive, but once we do bug 1125423 we can make it super fast (Ion could inline the compartment's outer object). (3) If |this| is a primitive and we're not in strict mode, we have to box it. In this case lazy-this computation could potentially win, but OTOH: when you define a function on, say, String.prototype *and* it uses |this|, it's likely to end up computing |this| anyway.

Also: (4) The debugger requesting |this| in a function that doesn't use |this| or eval. This could use the DebugScopeProxy.

I'm trying to remove the inner/outer/thisObj-hooks in bug 1125423 and I've been thinking more about fixing `this`. To make `this` a binding like `arguments`, functions that use `this` or `eval` could have the following code in the prologue: JSOP_CREATETHIS // boxes/pushes the `this` value for the script JSOP_SETLOCAL x (or JSOP_SETALIASEDVAR ".this" if aliased) Then we can remove JSOP_THIS and simply emit JSOP_GETLOCAL or JSOP_GETALIASEDVAR instead. We can also remove the `this` slot from arrow functions. Derived class constructors can initialize the slot to the TDZ MagicValue in the prologue and JSOP_SETTHIS can be replaced with JSOP_SETALIASEDVAR. If we're not inside a function, we could emit JSOP_GLOBALTHIS or something wherever `this` is used, after bug 1125423 this can just push a constant I think. AFAICS this covers everything?

What I described in comment 2 seems fine for functions: we will store `this` either in the frame or in the CallObject. Then if a function contains an arrow function that uses `this`, we'll use a GETALIASEDVAR to get it from the CallObject. I think I can make this (pun intended) work. I'm not sure how to handle non-function scripts with eval or arrow functions though. Ideally we'd store the `this` value in some scope object, so that `this` inside arrow functions etc can get it from there. But where? Can we use the lexical scope for this? Can you think of a better way?

(In reply to Jan de Mooij [:jandem] from comment #3) > What I described in comment 2 seems fine for functions: we will store `this` > either in the frame or in the CallObject. Then if a function contains an > arrow function that uses `this`, we'll use a GETALIASEDVAR to get it from > the CallObject. I think I can make this (pun intended) work. > > I'm not sure how to handle non-function scripts with eval or arrow functions > though. Ideally we'd store the `this` value in some scope object, so that > `this` inside arrow functions etc can get it from there. But where? Can we > use the lexical scope for this? Can you think of a better way? All 'eval' in ES6, regardless of strictness and directness, have an invisible, non-extensible lexical scope around the string to be evaluated so lexical bindings don't escape to the enclosing scope. Presumably this scope can store a 'this' binding. Though that implementation might not be as clean as you hope: since CallObjects will get a 'this' slot, and strict eval frames are going to have both a CallObject environment + that lexical environment. Which would be the canonical holder of 'this'? Regardless, ISTM we can emit JSOP_GETALIASEDVAR for eval. Global scripts could store 'this' in a special slot on the global lexical scope. The problem is how do we get to it? Global lexical bindings are never looked up via slot, because even a script compiled WITHOUT non-syntactic scope support can be cloned as-is and run under non-syntactic scope chains. So, all global lexical binding lookup, even via JSOP_GETGNAME, need to walk the scope chain. We can't emit JSOP_GETALIASEDVAR. It'd have to be a NAME op. But what name? Some internal 'this' symbol? It seems dirty. I don't know how to do cleaner than JSOP_THIS for global scripts at the moment. I don't know the implementation of arrow functions well. Wouldn't JSOP_CREATETHIS for arrow functions just pull it off the callee?

(In reply to Shu-yu Guo [:shu] from comment #4) > I don't know the implementation of arrow functions well. Wouldn't > JSOP_CREATETHIS for arrow functions just pull it off the callee? Right now, when defining an arrow function, we store `this` in a slot on the JSFunction. Then JSOP_THIS just loads the value from that slot. This worked fine so far, but it's incompatible and wrong inside derived class constructors, because `this` can change between defining and calling the arrow function. Also, the arrow function can contain `super()` and this has to initialize the `this` slot of the constructor. To fix it, `this` must become a real binding and I want to remove the `this` slot from arrow functions. Sure, we could still use the slot for arrow functions that are not defined inside a function (or not inside a derived class constructor), but it seems weird to have two different mechanisms (binding vs arrow function slot) depending on where the arrow was defined. (In reply to Shu-yu Guo [:shu] from comment #4) > Global scripts could store 'this' in a special slot on the global lexical > scope. The problem is how do we get to it? Global lexical bindings are never > looked up via slot, because even a script compiled WITHOUT non-syntactic > scope support can be cloned as-is and run under non-syntactic scope chains. > So, all global lexical binding lookup, even via JSOP_GETGNAME, need to walk > the scope chain. We can't emit JSOP_GETALIASEDVAR. It'd have to be a NAME > op. But what name? Some internal 'this' symbol? It seems dirty. I don't know > how to do cleaner than JSOP_THIS for global scripts at the moment. That's a good point. Can we say the global lexical scope stores the `this` binding always in the first slot or something? Then we can have a JSOP_GETGLOBALTHIS with a numHops operand and it should work more or less like a GETALIASEDVAR... Btw, it doesn't have to be an internal symbol, we can just call it "this" (it's an invalid variable name) or ".this". Generators do something similar (".generator" refers to the generator object). Also note that JSC copied or reinvented our 'store this in an arrow function slot' trick, but they are also in the process of removing it (and making `this` a real binding everywhere).

(In reply to Jan de Mooij [:jandem] from comment #5) > (In reply to Shu-yu Guo [:shu] from comment #4) > > That's a good point. Can we say the global lexical scope stores the `this` > binding always in the first slot or something? Then we can have a > JSOP_GETGLOBALTHIS with a numHops operand and it should work more or less > like a GETALIASEDVAR... > The hops are unknown ahead of time in the nonsyntactic case. You'd still need a special op that says "go straight to the global lexical scope." > Btw, it doesn't have to be an internal symbol, we can just call it "this" > (it's an invalid variable name) or ".this". Generators do something similar > (".generator" refers to the generator object). > Ah okay, that's good news.

Thanks shu! I think I'll start prototyping and see how it works out. `this` is used everywhere so I'm sure there will be a lot of interesting performance/correctness issues.

Works in the shell, passes all jit-tests and jstests (except for one debugger jit-test that fails for a silly reason). At first glance, performance seems more or less the same as without the patch, even though it gets rid of the lazy-this computation. It's a large patch but for the most part it's not that complicated. For instance, removing the this-slot from arrow functions is a lot of code churn but pretty trivial. I spent a lot of time dealing with the debugger and also fixed a number of issues with derived class constructors. After it lands, we should be able to allow arrow functions and eval in derived class constructors and it should Just Work though. TODO: send to Try, fix a lot of minor style/duplication/comment issues, add a ton of (debugger) tests, split the patch up.

Linux64 is green on Try. I'll split this in some chunks for review; unfortunately there are a lot of changes but most of them should be easy to review and it's all related to each other.

Just the frontend/ part. Requesting review from efaust because this touches classes and from shu because of the binding/scope changes. The patch makes the following changes: * In function scripts with their own 'this', we emit JSOP_FUNCTIONTHIS in the prologue and then emit JSOP_SETLOCAL or JSOP_SETALIASEDVAR to store it. We only do this if the function uses |this| or eval. * In global scripts (or eval/arrows in the global scope) we emit JSOP_GLOBALTHIS whenever `this` is used. * The this-checks and return-checks in derived class constructors are factored out as separate JSOP_CHECKTHIS and JSOP_CHECKRETURN. This was necessary for a number of reasons: (1) JSOP_CHECKTHIS is also required in nested eval and arrow functions, so testing script->isDerivedClassConstructor() is not sufficient. (2) The return-check needs the `this` binding. The easiest way to pass it is by pushing it on the stack (GETLOCAL or GETALIASEDVAR) and have JSOP_CHECKRETURN pop it. * I changed super{prop,call,...} to use a new PNK_SUPERBASE node instead of PNK_POSHOLDER. This holds a reference to the '.this' name. * PNK_SETTHIS is a new node used to assign the result of a super call to 'this'.

Shu, please take a careful look at the debugger changes. The DebugScopeProxy changes I mostly cargo culted from the similar 'arguments' code already there.

We don't Ion-compile the ops for derived class constructors yet, but that shouldn't be too hard to add later.

Comment on attachment 8686209 [details] [diff] [review] Part 1 - Frontend Review of attachment 8686209 [details] [diff] [review]: ----------------------------------------------------------------- Very exciting. The non-class stuff looks fine to me, with comments addressed. I don't really understand the class stuff so I skipped them for the most part. ::: js/src/frontend/BytecodeEmitter.cpp @@ +1041,5 @@ > // JSOP_GETNAME etc, to bypass |with| objects on the scope chain. > + // It's safe to emit .this lookups though because |with| objects skip > + // those. > + MOZ_ASSERT_IF(op == JSOP_GETNAME || op == JSOP_GETGNAME, > + !sc->isDotVariable(atom) || atom == cx->names().dotThis); Your changes to isDotVariable already checks for dotThis. @@ +3394,5 @@ > + > + if (!emit1(JSOP_FUNCTIONTHIS)) > + return false; > + > + BindingIter bi = Bindings::thisBinding(cx, script); I don't see Bindings::thisBinding anywhere. Maybe it's in a later part, I'll assume it does the obvious thing here. @@ +3540,5 @@ > // slot. We just emit an explicit return in this case. > if (hasTryFinally) { > if (!emit1(JSOP_UNDEFINED)) > return false; > + if (!emit1(JSOP_SETRVAL)) I don't understand this change. Does this have to do with class stuff? @@ +6175,5 @@ > + MOZ_ASSERT(pn->name() == cx->names().dotThis); > + > + if (!emitTree(pn)) > + return false; > + if (sc->needsThisTDZChecks() && !emit1(JSOP_CHECKTHIS)) When would a function this need TDZ checks? If we're an arrow function nested inside a constructor or something? ::: js/src/frontend/ParseNode.h @@ +474,5 @@ > * PNK_TRUE, nullary pn_op: JSOp bytecode > * PNK_FALSE, > + * PNK_NULL > + * > + * PNK_THIS, unary pn_kid: '.this' Name I would note that pn_kid is non-null only for function this. ::: js/src/frontend/Parser.cpp @@ +1079,5 @@ > + // Create a declaration for '.this' if there are any unbound uses in the > + // function body. > + for (AtomDefnRange r = pc->lexdeps->all(); !r.empty(); r.popFront()) { > + if (r.front().key() == dotThis) { > + Definition* dn = r.front().value().get<FullParseHandler>(); Maybe assert here that dn is a placeholder. @@ +1080,5 @@ > + // function body. > + for (AtomDefnRange r = pc->lexdeps->all(); !r.empty(); r.popFront()) { > + if (r.front().key() == dotThis) { > + Definition* dn = r.front().value().get<FullParseHandler>(); > + pc->lexdeps->remove(dotThis); You shouldn't need to manually remove from lexdeps here, ParseContext::define should do that, plus linking up the use-def chains. @@ +1082,5 @@ > + if (r.front().key() == dotThis) { > + Definition* dn = r.front().value().get<FullParseHandler>(); > + pc->lexdeps->remove(dotThis); > + pc->sc->asFunctionBox()->setHasThisBinding(); > + return pc->define(tokenStream, dotThis, dn, Definition::VAR); Wow, a real binding! ::: js/src/frontend/SharedContext.h @@ +177,5 @@ > }; > > +// The kind of this-binding for the current scope. Note that arrow functions > +// (and generator expression lambdas) don't have their own this-binding, so > +// ThisBinding can be Global or Module in that case. I'm confused about what this is saying for the arrow function case. Is it that for arrow functions, it doesn't matter what the ThisBinding is, or that the ThisBinding value will reflect what the nearest enclosing scope's ThisBinding is? I think it's the latter; if so, I'd clarify by saying that arrow functions' ThisBinding can be any value.

Comment on attachment 8686210 [details] [diff] [review] Part 2 - VM changes Review of attachment 8686210 [details] [diff] [review]: ----------------------------------------------------------------- The non-Debugger non-class VM parts look good. The Debugger parts don't seem right to me, with specific comments below. ::: js/src/builtin/Eval.cpp @@ +333,5 @@ > } > > + // It doesn't matter what we pass as thisv, the eval script gets `this` > + // from the scope chain (or JSOP_GETGLOBALTHIS if global eval). > + RootedValue thisv(cx); |thisv| doesn't seem to be used anymore. ::: js/src/vm/Debugger.cpp @@ +6308,5 @@ > THIS_FRAME_ITER(cx, argc, vp, "get this", args, thisobj, _, iter); > + > + Debugger* debug = Debugger::fromChildJSObject(thisobj); > + > + // First, if this frame is an eval frame, make sure its a debuggee frame. Typo: it's @@ +6310,5 @@ > + Debugger* debug = Debugger::fromChildJSObject(thisobj); > + > + // First, if this frame is an eval frame, make sure its a debuggee frame. > + // This ensures its DebugScope will be in the liveScopes map and allows the > + // DebugScopeProxy to handle more cases. The comment isn't very clear. The code below is ensuring that the *caller* frame of eval frames are marked as DEBUGGEE. @@ +6319,5 @@ > + if (iter2.isIon() && !iter2.ensureHasRematerializedFrame(cx)) > + return false; > + if (!debug->getScriptFrame(cx, iter2, &dummy)) > + return false; > + } This code doesn't seem right. It's not just the immediate caller that needs to be marked as DEBUGGEE, but all caller frames between the eval frame and the nearest caller frame that has a 'this'. That is, what if the immediate caller is an arrow function frame and it doesn't have a 'this' either? @@ +6332,5 @@ > + return false; > + > + if (!GetThisValueForDebuggerMaybeSentinel(cx, env, &thisv)) > + return false; > + } The DEBUGGEE-marking loop above and this code are strange. For Debugger.Frame.this I think using DebugScopes here is actually more trouble than it's worth, given the comment above and below in GetThisValueForDebuggerMaybeSentinel. Also, I'm not sure I like the idea of D.F.this deoptimizing a bunch of caller frames + making DebugScopeProxies despite there being no Debugger.Environments being instantiated. Here's what I suggest: do manual walks of the stack and the scope chain without going through GetDebugScope. That is, 1) Let the current frame be the frame pointed to by the D.F. 2) If the current frame has a 'this' binding, return it. 3) If not, walk the scope chain to the nearest scope that has an aliased 'this' binding, return it. 4a) If there is no nearest scope that has an aliased 'this' binding (e.g., only function scripts that did not have an aliased 'this'), and the current frame is an eval frame, walk the stack to find the nearest parent frame with a 'this'. Let that frame be the current frame and go to step 2. 4b) Otherwise, return <optimized out> For the current getMissingThis code that you've already written, you can probably leave it in and we can expose a "lexical this" getter on Debugger.Environment to distinguish when an environment does not have a 'this' (like arrow functions), but its corresponding frame does. How does that sound? Ping me on IRC on Monday if you have comments. ::: js/src/vm/Interpreter.cpp @@ +2444,5 @@ > + if (!GetNonSyntacticGlobalThis(cx, REGS.fp()->scopeChain(), REGS.stackHandleAt(-1))) > + goto error; > + } else { > + ClonedBlockObject* lexicalScope; > + lexicalScope = &cx->global()->lexicalScope(); Nit: merge this line into the previous one. @@ +3080,5 @@ > + // Only the .this slot can hold the TDZ MagicValue. > + if (IsUninitializedLexical(val)) { > + PropertyName* name = ScopeCoordinateName(cx->runtime()->scopeCoordinateNameCache, > + script, REGS.pc); > + MOZ_ASSERT(name == cx->names().dotThis); Also assert that the next op is JSOP_CHECKTHIS. ::: js/src/vm/ScopeObject.cpp @@ +1019,4 @@ > MOZ_ASSERT(isGlobal() || !isSyntactic()); > + Value v = getReservedSlot(THIS_VALUE_SLOT); > + if (v.isObject()) > + return ObjectValue(*ToWindowProxyIfWindow(&v.toObject())); What is ToWindowProxyIfWindow? @@ +3081,5 @@ > + while (true) { > + if (IsExtensibleLexicalScope(scope)) { > + res.set(scope->as<ClonedBlockObject>().thisValue()); > + return true; > + } Since the scopeChain that's passed in is always the result of GetDebugScopeForFrame, how does this path get taken? GetDebugScopeFrame returns either a non-ScopeObject or a DebugScopeObject wrapping a ScopeObject, never a bare ScopeObject. @@ +3094,5 @@ > + > + if (scope->is<ModuleEnvironmentObject>()) { > + res.setUndefined(); > + return true; > + } Same here, I don't think this path will get taken? ::: js/src/vm/TypeInference.cpp @@ +3876,5 @@ > > if (!iter.isConstructing() || !iter.matchCallee(cx, function)) > continue; > > + Value thisv = iter.originalFunctionThis(cx); I don't understand rollbackPartiallyInitializedObjects enough to say if this is okay. What happens here for derived constructors?

Comment on attachment 8686215 [details] [diff] [review] Part 4 - Tests Review of attachment 8686215 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/debug/Frame-this-05.js @@ +19,5 @@ > +// And inside arrow functions. > +g.eval("x = 1; (() => { debugger; })()"); > +assertEq(c, 2); > +g.eval("x = 5; (() => { eval('debugger'); })()"); > +assertEq(c, 10); Instead of using a counter, I find the logging style of tests easier to reason about for these things, e.g., var log = ""; dbg.onDebuggerStatement = function (f) { log += "d"; } ... assertEq(log, "dddd");

(In reply to Shu-yu Guo [:shu] from comment #15) > This code doesn't seem right. It's not just the immediate caller that needs > to be marked as DEBUGGEE, but all caller frames between the eval frame and > the nearest caller frame that has a 'this'. That is, what if the immediate > caller is an arrow function frame and it doesn't have a 'this' either? It was just an heuristic. Eval frames have the property that the (non-eval) caller frame is the frame we're interested in. Arrow functions don't work that way because arrow functions can be returned/assigned and called somewhere else. > Here's what I suggest: do manual walks of the stack and the scope chain > without going through GetDebugScope. Fair enough, let me try that next week so we can see how it works. I agree it's simpler. > For the current getMissingThis code that you've already written, you can > probably leave it in and we can expose a "lexical this" getter on > Debugger.Environment to distinguish when an environment does not have a > 'this' (like arrow functions), but its corresponding frame does. We also need the getMissingThis DebugScopeProxy code for evalInFrame("this") ... > What is ToWindowProxyIfWindow? I'll add a comment. > I don't understand rollbackPartiallyInitializedObjects enough to say if this > is okay. What happens here for derived constructors? They shouldn't end up there. I'll add some comments/checks/asserts. Thanks for the reviews!

(In reply to Jan de Mooij [:jandem] from comment #17) > > For the current getMissingThis code that you've already written, you can > > probably leave it in and we can expose a "lexical this" getter on > > Debugger.Environment to distinguish when an environment does not have a > > 'this' (like arrow functions), but its corresponding frame does. > > We also need the getMissingThis DebugScopeProxy code for evalInFrame("this") > ... Ah yes, very good point.

Combined patch. Rebased and with comments from Shu addressed.

Thanks Shu, Debugger.Frame.this is way nicer and simpler now! (In reply to Shu-yu Guo [:shu] from comment #15) > 4a) If there is no nearest scope that has an aliased 'this' binding (e.g., > only function scripts that did not have an aliased 'this'), and the current > frame is an eval frame, walk the stack to find the nearest parent frame with > a 'this'. Let that frame be the current frame and go to step 2. It turned out I didn't need any special handling for eval frames: function frames with direct eval always have an aliased this-binding, so we can always get it from the scope chain and the ScopeIter loop does the right thing :)

Some minor changes, sorry for the noise.

Comment on attachment 8687911 [details] [diff] [review] Part 2 - VM changes (v2) Review of attachment 8687911 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/ScopeObject.cpp @@ +3103,5 @@ > + res.set(frame.unaliasedLocal(bi.frameIndex())); > + else > + res.setMagic(JS_OPTIMIZED_OUT); > + return true; > + } Great, this loop is much easier for me to understand.

Comment on attachment 8686209 [details] [diff] [review] Part 1 - Frontend Review of attachment 8686209 [details] [diff] [review]: ----------------------------------------------------------------- This looks great. Just the stuff shu mentioned. Maybe another comment to clear up that SETRVAL vs. RETURN nest. ::: js/src/frontend/BytecodeEmitter.cpp @@ +3540,5 @@ > // slot. We just emit an explicit return in this case. > if (hasTryFinally) { > if (!emit1(JSOP_UNDEFINED)) > return false; > + if (!emit1(JSOP_SETRVAL)) I suspect this has to do with CHECKRETURN stuff below. ::: js/src/frontend/SharedContext.h @@ +177,5 @@ > }; > > +// The kind of this-binding for the current scope. Note that arrow functions > +// (and generator expression lambdas) don't have their own this-binding, so > +// ThisBinding can be Global or Module in that case. Agreed with shu here. I too had to think about this twice.

Comment on attachment 8687911 [details] [diff] [review] Part 2 - VM changes (v2) Review of attachment 8687911 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/ReflectParse.cpp @@ +3348,5 @@ > identifier(targetStr, &pn->pn_right->pn_pos, &targetIdent) && > builder.metaProperty(newIdent, targetIdent, &pn->pn_pos, dst); > } > > + case PNK_SETTHIS: Traditionally, this hunk (and anything else necessary to make the reflect.parse tests pass) would land with the parser changes. ::: js/src/vm/Interpreter.cpp @@ +631,5 @@ > return true; > } > > + // It doesn't matter what we pass as thisv, global/eval scripts get |this| > + // from the scope chain. TODO: remove thisv from ExecuteState. Does this get addressed later/can you file the trivial followup? @@ +3180,5 @@ > + // before a super() call. > + if (IsUninitializedLexical(REGS.sp[-1])) { > + MOZ_ASSERT(script->isDerivedClassConstructor()); > + JSOp next = JSOp(*GetNextPc(REGS.pc)); > + MOZ_ASSERT(next == JSOP_CHECKTHIS || next == JSOP_CHECKRETURN || next == JSOP_CHECKTHISREINIT); Maybe factor this out with the call above. @@ +3203,5 @@ > + // Derived class constructors store the TDZ Value in the .this slot > + // before a super() call. > + MOZ_ASSERT_IF(!script->isDerivedClassConstructor(), > + !IsUninitializedLexical(REGS.fp()->unaliasedLocal(i))); > + Can't we do one stronger? If it /is/ a derived class constructor, it /must/ not be a reinit, right? ::: js/src/vm/ScopeObject.cpp @@ +1024,5 @@ > + // If `v` is a Window, return the WindowProxy instead. We called > + // GetThisValue (which also does ToWindowProxyIfWindow) when storing > + // the value in THIS_VALUE_SLOT, but it's possible the WindowProxy was > + // attached to the global *after* we set THIS_VALUE_SLOT. > + return ObjectValue(*ToWindowProxyIfWindow(&v.toObject())); Nice.

Comment on attachment 8686213 [details] [diff] [review] Part 3 - JIT changes Review of attachment 8686213 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. This is so exciting. ::: js/src/jit/BaselineCompiler.cpp @@ +1324,5 @@ > + prepareVMCall(); > + pushArg(R1); > + if (!callVM(ThrowBadDerivedReturnInfo)) > + return false; > + masm.assumeUnreachable("Should throw on bad derived constructor return"); nice. ::: js/src/jit/BaselineIC.cpp @@ +541,4 @@ > TypeScript::SetArgument(cx, script, argument, value); > } else { > + if (value.isMagic(JS_UNINITIALIZED_LEXICAL)) > + TypeScript::Monitor(cx, script, pc, TypeSet::UnknownType()); Do we want any things here that there shouldn't be UNINITIALIZED_LEXICAL other than through this? I believe that's still an invariant. Nice catch, though. This looks like it used to be wrong. ::: js/src/jit/IonBuilder.cpp @@ +12763,5 @@ > > bool > +IonBuilder::jsop_functionthis() > +{ > + MOZ_ASSERT(info().funMaybeLazy()); MOZ_ASSERT(!info().funMaybeLazy()->isArrow());

So we can add all kinds of depraved tests with arrow functions and derived class constructors, now, right? Is there anything else blocking allowing eval and arrow functions in derived class constructors? I will take care of this in another bug, if that's true.

Thanks for the reviews! (In reply to Eric Faust [:efaust] from comment #26) > So we can add all kinds of depraved tests with arrow functions and derived > class constructors, now, right? Is there anything else blocking allowing > eval and arrow functions in derived class constructors? Yes I think that should Just Work now. I didn't test it though so you might run into some (hopefully minor) issues. > I will take care of this in another bug, if that's true. Great, I was hoping you'd do that ;) (In reply to Eric Faust [:efaust] from comment #24) > Traditionally, this hunk (and anything else necessary to make the > reflect.parse tests pass) would land with the parser changes. Yeah I'll land this as one big patch because each part doesn't build/work on its own. I just split it up to make it easier to review. > Does this get addressed later/can you file the trivial followup? Sure. I also want to make some other (minor) followup changes after this lands.

(In reply to Eric Faust [:efaust] from comment #25) > Do we want any things here that there shouldn't be UNINITIALIZED_LEXICAL > other than through this? I added an assert to the UNINITIALIZED_LEXICAL case at the start of this function: MOZ_ASSERT(stub->monitorsThis() || *GetNextPc(pc) == JSOP_CHECKTHIS || *GetNextPc(pc) == JSOP_CHECKRETURN); (In reply to Eric Faust [:efaust] from comment #24) > Can't we do one stronger? If it /is/ a derived class constructor, it /must/ > not be a reinit, right? It's not easy to check that inside SETLOCAL, because we have to know whether this SETLOCAL is storing to the .this slot and getting that information from BindingIter is pretty expensive.

The patch regressed Octane-richards. There's a function that uses 'this' 7 times and we emit more bytecode now (JSOP_GETLOCAL is larger than JSOP_THIS), so it's no longer treated as "small function" when inlining. This patch increases smallFunctionMaxBytecodeLength_ from 100 to 120. With this patch we inline the same set of functions on richards as before (with --no-threads).

Hm this follow up patch did not fix the richards regression. I'll take another look.

(In reply to Jan de Mooij [:jandem] from comment #33) > Hm this follow up patch did not fix the richards regression. I'll take > another look. But it is a win: 4,5% on Octane-Box2D 2% on kraken-parse-financial 3,5% on ss-unpack-code 6% on asmjs-ubench-fbirds-polyfill 4% on asmjs-ubench-mandelbrot-polyfill

(In reply to Guilherme Lima from comment #35) > (In reply to Jan de Mooij [:jandem] from comment #33) > > Hm this follow up patch did not fix the richards regression. I'll take > > another look. > > But it is a win: And just for the record, the second follow-up patch did fix the richards regression.

https://hg.mozilla.org/mozilla-central/rev/31c4223041fb https://hg.mozilla.org/mozilla-central/rev/b3a18d76bf2b