Bug 157908 - arrow function lexical environment should reuse the same environment as the function's lexical environment where possible
Summary: arrow function lexical environment should reuse the same environment as the f...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-19 12:11 PDT by Saam Barati
Modified: 2016-05-20 08:44 PDT (History)
11 users (show)

See Also:


Attachments
patch (4.63 KB, patch)
2016-05-19 12:24 PDT, Saam Barati
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-05-19 12:11:10 PDT
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.
Comment 1 Saam Barati 2016-05-19 12:24:50 PDT
Created attachment 279409 [details]
patch
Comment 2 Geoffrey Garen 2016-05-19 12:28:08 PDT
Sunspider/tofte uses arrow functions?
Comment 3 Saam Barati 2016-05-19 13:10:47 PDT
(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.
Comment 4 Saam Barati 2016-05-19 13:10:58 PDT
landed in:
http://trac.webkit.org/changeset/201176
Comment 5 Saam Barati 2016-05-19 13:13:06 PDT
(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(...)
    ...
}
Comment 6 Chris Dumez 2016-05-20 08:44:04 PDT
(In reply to comment #4)
> landed in:
> http://trac.webkit.org/changeset/201176

Confirmed 5% JetStream progression from this change on iOS.