Bug 157908 – arrow function lexical environment should reuse the same environment as the function's lexical environment where possible

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

I removed this recently, but it causes a 7x regression in Sunspider/tofte which caused 5% regression in JetStream.
It's probably valuable to reuse the same environment when possible for functions that use eval and have simple parameter lists.

[Saam Barati]

Created attachment 279409 [details]
patch

[Geoffrey Garen]

Sunspider/tofte uses arrow functions?

[Saam Barati]

(In reply to comment #2)
> Sunspider/tofte uses arrow functions?

It uses eval.
For example:
function foo(x, y, z // Note: simple parameter list) {
    ... do some stuff
    eval(...) // Note must be direct eval
    ... do more stuff
}

My patch made such 'foo' functions slower.

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/201176

[Saam Barati]

(In reply to comment #3)
> (In reply to comment #2)
> > Sunspider/tofte uses arrow functions?
> 
> It uses eval.
> For example:
> function foo(x, y, z // Note: simple parameter list) {
>     ... do some stuff
>     eval(...) // Note must be direct eval
>     ... do more stuff
> }
> 
> My patch made such 'foo' functions slower.

To elaborate more, because we don't know what the eval will do, the eval
may use arrow functions. So any function with a direct eval uses arrow functions.
My previous patch made us always allocate a new environment for an arrow function's
'this', new.target, etc. But, this patch makes it so that we reuse the already
created activation when doing so is sound. It's sound to do this when the function
'foo' has a simple parameter list.
It's not sound to do this if it doesn't have a simple parameter list, i.e:

function foo({a = ()=>this}) {
    ...
    eval(...)
    ...
}

[Chris Dumez]

(In reply to comment #4)
> landed in:
> http://trac.webkit.org/changeset/201176

Confirmed 5% JetStream progression from this change on iOS.