Bug 155988 – parsing arrow function expressions slows down the parser by 8% lets recoup some loss

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

[Saam Barati]

Specifically, if you remove the
```
if (isArrowFunctionParamters) ...
```
branch inside parseAssignmentExpression, we have an 8% progression on octane code load.
I'm sure we can make this faster.

[Saam Barati]

Created attachment 275222 [details]
patch

[Saam Barati]

VMs tested:
"og" at /Volumes/Data/WK/a/OpenSource/WebKitBuild/Release/jsc (r198848)
"arrow" at /Volumes/Data/WK/b/OpenSource/WebKitBuild/Release/jsc (r198848)

Collected 10 samples per benchmark/VM, with 10 VM invocations per benchmark. Emitted a call to
gc() between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used
the jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark
execution times with 95% confidence intervals in milliseconds.

                            og                      arrow                                       

closure              0.76198+-0.00157    ^     0.72355+-0.00140       ^ definitely 1.0531x faster
jquery               9.87951+-0.31561    ^     9.10787+-0.03122       ^ definitely 1.0847x faster

<geometric>          2.74314+-0.04285    ^     2.56709+-0.00460       ^ definitely 1.0686x faster

[Build Bot]

Comment on attachment 275222 [details]
patch

Attachment 275222 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1071510

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 275225 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275222 [details]
patch

Attachment 275222 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1071532

New failing tests:
inspector/runtime/parse.html

[Build Bot]

Created attachment 275227 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275222 [details]
patch

Attachment 275222 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1071550

New failing tests:
inspector/runtime/parse.html

[Build Bot]

Created attachment 275228 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Saam Barati]

Created attachment 275234 [details]
patch

fix inspector test failure

[Build Bot]

Comment on attachment 275234 [details]
patch

Attachment 275234 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1072029

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 275239 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Benjamin Poulain]

Comment on attachment 275234 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275234&action=review

Looks good to me.
Can you please check the test coverage before landing. We often have good coverage of valid syntax and poor coverage of various invalid syntax.

> Source/JavaScriptCore/ChangeLog:36
> +        `({x = 30}) => x;`

Do you have test coverage for those failing cases?

[Saam Barati]

(In reply to comment #12)
> Comment on attachment 275234 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275234&action=review
> 
> Looks good to me.
> Can you please check the test coverage before landing. We often have good
> coverage of valid syntax and poor coverage of various invalid syntax.
> 
> > Source/JavaScriptCore/ChangeLog:36
> > +        `({x = 30}) => x;`
> 
> Do you have test coverage for those failing cases?

Thanks for the review. I'll add a bunch of tests to our
parser syntax check tests.

[Saam Barati]

Created attachment 275255 [details]
patch for landing

I'm going to let EWS take a look.

[Saam Barati]

Created attachment 275256 [details]
patch for landing

a few more tests

[Build Bot]

Comment on attachment 275256 [details]
patch for landing

Attachment 275256 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1072652

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 275257 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Saam Barati]

The reason the debug tests are very interesting. Look at the following program:
```
function foo() {
    var y = (x) => x;
}
```
because we initially successfully parse "(x)", we assume that we're
*using* the variable "x". This is done because if the line were
written "var y = (x);" we would really be using "x". However,
we aren't using "x" here because it's a parameter to the arrow
function. But after we notice the "=>" it's too late to backtrack
on this use. And because we store these uses in a hash table, it's
not obvious on how to backtrack this. The biggest consequence that this
has is that we might claim that certain variables are captured when
they really aren't. But, because the outer-most function in a builtin
isn't supposed to close over any variables, we crash from a debug
assertion.

I have an idea for how we might be able to do this. I think we can
create a dummy used variable set and swap it in if we think
we might be parsing an arrow function. If we are indeed parsing
an arrow function, we can swap back in the original hash table.
If we're not parsing an arrow function, we can add back any
contents from the new hash table to the old one, and swap back in
the old one. Maybe this is too crazy. I'm going to try it out.
Another possibility is we can make the concession that our capture
analysis is imprecise and that we won't have debugging code inside
builtins to prevent global capture. Lets see if the hash table
swapping degrades the perf progression.

[Saam Barati]

(In reply to comment #18)
> The reason the debug tests are very interesting. Look at the following
> program:
> ```
> function foo() {
>     var y = (x) => x;
> }
> ```
> because we initially successfully parse "(x)", we assume that we're
> *using* the variable "x". This is done because if the line were
> written "var y = (x);" we would really be using "x". However,
> we aren't using "x" here because it's a parameter to the arrow
> function. But after we notice the "=>" it's too late to backtrack
> on this use. And because we store these uses in a hash table, it's
> not obvious on how to backtrack this. The biggest consequence that this
> has is that we might claim that certain variables are captured when
> they really aren't. But, because the outer-most function in a builtin
> isn't supposed to close over any variables, we crash from a debug
> assertion.
> 
> I have an idea for how we might be able to do this. I think we can
> create a dummy used variable set and swap it in if we think
> we might be parsing an arrow function. If we are indeed parsing
> an arrow function, we can swap back in the original hash table.
> If we're not parsing an arrow function, we can add back any
> contents from the new hash table to the old one, and swap back in
> the old one. Maybe this is too crazy. I'm going to try it out.
> Another possibility is we can make the concession that our capture
> analysis is imprecise and that we won't have debugging code inside
> builtins to prevent global capture. Lets see if the hash table
> swapping degrades the perf progression.
So the naive move() solution results in no perf progression, which
means it's too high of overhead. I tried a few other solutions
that involve marking variables used in such contexts. It takes away
about half the progression. I was seeing about 2.5-3% progression instead
of 5.5%-6%. I'll think about this more and see if we can recoup all of the loss.

[Saam Barati]

So I think we need to make the concession that we're
not going to get a 6% progression from this. It seems
pretty lame to slow down the runtime execution properties
of the code based on how you name your parameters
in an arrow function.

I'm seeing a 3% progression while maintaining good capture analysis.

[Saam Barati]

We will probably get higher than 3% progression. I'm actually seeing 4-5%.

VMs tested:
"og" at /Volumes/Data/WK/a/OpenSource/WebKitBuild/Release/jsc (r198861)
"parser" at /Volumes/Data/WK/b/OpenSource/WebKitBuild/Release/jsc (r198861)

Collected 14 samples per benchmark/VM, with 14 VM invocations per benchmark. Emitted a call to
gc() between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used
the jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark
execution times with 95% confidence intervals in milliseconds.

                            og                      parser                                      

closure              0.53500+-0.00306    ^     0.51041+-0.00275       ^ definitely 1.0482x faster
jquery               6.75072+-0.02721    ^     6.42015+-0.01876       ^ definitely 1.0515x faster

<geometric>          1.90039+-0.00654    ^     1.81020+-0.00474       ^ definitely 1.0498x faster

[Saam Barati]

Created attachment 275336 [details]
patch for review/ews

I'm putting this up for review again because this has changed enough since Ben's previous review.

[WebKit Commit Bot]

Comment on attachment 275336 [details]
patch for review/ews

Clearing flags on attachment: 275336

Committed r198927: <http://trac.webkit.org/changeset/198927>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.