Bug 164808 – [JSC] speed up parsing of async functions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Caitlin Potter (:caitp)]

[JSC] speed up parsing with ES2017_ASYNCFUNCTION_SYNTAX enabled

[Caitlin Potter (:caitp)]

Created attachment 294913 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 294913 [details]
Patch

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

Could you paste the performance numbers with this change?

> Source/JavaScriptCore/parser/Parser.cpp:2698
> +                if (UNLIKELY(isAsync)) {

I think it is not correct. `!m_lexer->prevTerminator()` check seems missing.
And can you add a test for this line terminator case?

> Source/JavaScriptCore/parser/Parser.cpp:-3464
> -#if !ENABLE(ES2017_ASYNCFUNCTION_SYNTAX)
>      if (wasOpenParen) {
> -#else
> -    if (wasOpenParen || (wasIdentifierOrKeyword && *m_token.m_data.ident == m_vm->propertyNames->async)) {
> -#endif

Why is this changed? If it is meaningful change, can you add a test for that?

> Source/JavaScriptCore/parser/Parser.cpp:3758
> +        if (isAsync)

After performing `goto parseProperty;`, we can reach here with the condition `isAsync=true`, but `ident != "async"`.
It is very tricky. Can you fix it?

> Source/JavaScriptCore/parser/Parser.cpp:4266
>  #if ENABLE(ES2017_ASYNCFUNCTION_SYNTAX)
>          if (m_parserState.functionParsePhase == FunctionParsePhase::Parameters)
>              failIfFalse(m_parserState.allowAwait, "Cannot use await expression within parameters");
> -        FALLTHROUGH;
> +        goto identifierExpression;
> +#endif

How about moving AWAIT clause before IDENT clause and using FALLTHROUGH?

> Source/JavaScriptCore/parser/Parser.h:139
> +ALWAYS_INLINE static bool isSafeContextualKeyword(const JSToken& token)

Can you use more detailed word instead of "safe"?

> Source/JavaScriptCore/parser/ParserTokens.h:87
> +    AWAIT, // FirstSafeContextualKeywordToken

Ditto.

> Source/JavaScriptCore/parser/ParserTokens.h:93
> +    FirstSafeContextualKeywordToken = AWAIT,
> +    LastSafeContextualKeywordToken = LastContextualKeywordToken,

Ditto.

[Caitlin Potter (:caitp)]

Comment on attachment 294913 [details]
Patch

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

some performance numbers at https://gist.github.com/caitp/0af527f0981483d318f474315ec8d86e, which from my local tests is more neutral than what is in the tree currently.

>> Source/JavaScriptCore/parser/Parser.cpp:2698
>> +                if (UNLIKELY(isAsync)) {
> 
> I think it is not correct. `!m_lexer->prevTerminator()` check seems missing.
> And can you add a test for this line terminator case?

I don't think a test is really needed, but you're right that the prevTerminator check is needed. Not sure why it got removed, probably just eagerly deleting stuff.

>> Source/JavaScriptCore/parser/Parser.cpp:-3464
>> -#endif
> 
> Why is this changed? If it is meaningful change, can you add a test for that?

The check on the right was potentially doing redundant work --- the logic on the right of the `||` is now moved to parsePrimaryExpression instead, where the open paren for async arrow functions is actually parsed

>> Source/JavaScriptCore/parser/Parser.cpp:3758
>> +        if (isAsync)
> 
> After performing `goto parseProperty;`, we can reach here with the condition `isAsync=true`, but `ident != "async"`.
> It is very tricky. Can you fix it?

I believe the way `isAsync` is set should mitigate that (either `isAsyncMethod` or `isGenerator` is set, so `isAsync` should be false in that case)

>> Source/JavaScriptCore/parser/Parser.cpp:4266
>> +#endif
> 
> How about moving AWAIT clause before IDENT clause and using FALLTHROUGH?

I think FALLTHROUGH is riskier than the explicit goto, so it's probably safer to just do the explicit goto (same as is done for the other contextual keywords below).

> Source/JavaScriptCore/parser/Parser.cpp:4270
> +            JSTextPosition start = tokenStartPosition();

I realize this is all a bit redundant here, but it does allow us to not save and restore parser state, and reduces branching a little bit, I seemed to get a little bit of a win out of it. If we prefer smaller code over less branching and state saving, I'll change it (would be good to have someone try longer running perf jobs to see if this is actually a win in practice, though it seems to be locally)

> Source/JavaScriptCore/parser/Parser.cpp:-4268
> -        if (match(ARROWFUNCTION))

This seems to have been completely unneeded

>> Source/JavaScriptCore/parser/Parser.h:139
>> +ALWAYS_INLINE static bool isSafeContextualKeyword(const JSToken& token)
> 
> Can you use more detailed word instead of "safe"?

I'm not sure I have a very good word to describe this. How about just a comment indicating what "safe" means?

>> Source/JavaScriptCore/parser/ParserTokens.h:87
>> +    AWAIT, // FirstSafeContextualKeywordToken
> 
> Ditto.

Again, how about just a comment explaining what it means?

> Source/JavaScriptCore/runtime/CommonIdentifiers.h:278
> +    macro(async) \

TODOL remove the other "async" common identifier, since it's not needed now

[Build Bot]

Comment on attachment 294913 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 294920 [details]
Archive of layout-test-results from ews117 for mac-yosemite

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

[Caitlin Potter (:caitp)]

Created attachment 294921 [details]
Patch

[Caitlin Potter (:caitp)]

Created attachment 294922 [details]
Patch

rebase, commit uncommitted changes that were omitted before, possibly fix EFL on ews

[Build Bot]

Comment on attachment 294922 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 294925 [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

[Yusuke Suzuki]

It seems like EFL and mac-debug are still red.

[Caitlin Potter (:caitp)]

Created attachment 294934 [details]
Patch

[Caitlin Potter (:caitp)]

(In reply to comment #10)
> It seems like EFL and mac-debug are still red.

EFL is fixed, and it looks like the remaining test failures on the current builds are either flakes, or unrelated breakage

[Build Bot]

Comment on attachment 294934 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 294937 [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

[Caitlin Potter (:caitp)]

Created attachment 294939 [details]
Patch

re-add the `if (match(ARROWFUNCTION)) return 0;` line to parsePrimaryExpression to avoid the assertion failure in debug builds

[Caitlin Potter (:caitp)]

Comment on attachment 294913 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.cpp:-4268
>> -        if (match(ARROWFUNCTION))
> 
> This seems to have been completely unneeded

I guess it actually is needed to avoid adding arrow function parameters to used variables. re-added with a comment.

[Caitlin Potter (:caitp)]

Some benchmark numbers for the fixed build:

```
Collected 200 samples per benchmark/VM, with 200 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.

                       WithoutAsync               WithAsync                                     

closure              0.50837+-0.00430          0.50666+-0.00399       
jquery               6.74429+-0.04994          6.74237+-0.05579       

<geometric>          1.85033+-0.01108          1.84672+-0.01048         might be 1.0020x faster
```

[Saam Barati]

Comment on attachment 294939 [details]
Patch

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

Do you know which changes made the performance better?

> Source/JavaScriptCore/parser/Parser.cpp:4283
> +            // Avoid using variable if it is an arrow function parameter

Why is this needed? I thought there was code that handled this when parsing arrow functions

> Source/JavaScriptCore/parser/Parser.cpp:4287
> +            const bool isEval = false;

Can you create a helper for this so this code and the code below won't ever fall out of sync with each other?

> Source/JavaScriptCore/parser/Parser.cpp:4553
> +                size_t usedVariablesSize = baseIsAsyncKeyword ? currentScope()->currentUsedVariablesSize() : 0;

This branch seems unnecessary since you only backtrack on the set when it's true below.

> Source/JavaScriptCore/parser/Parser.h:131
> +// _Any_CntextualKeyword includes keywords such as "let" or "yield", which have a specific meaning depending on the current parse mode

Typo "Cntextual"

> Source/JavaScriptCore/parser/Parser.h:139
> +    return token.m_type == IDENT || (token.m_type >= FirstContextualKeywordToken && token.m_type <= LastContextualKeywordToken);

This could call isAnyContextualKeyword for the rhs of the "||". Otherwise, you should just remove that function since it's unused.

> Source/JavaScriptCore/parser/Parser.h:141
> +// _Safe_ContextualKeyword includes only contextual keywords which are independent from parse mode.

I'm confused by this. Why is this not a contradiction with your above definition of a contextual keyword?

> Source/JavaScriptCore/parser/ParserTokens.h:85
> +    LET, // FirstContextualKeywordToken

This comment isn't needed given the definitions below.

> Source/JavaScriptCore/parser/ParserTokens.h:87
> +    AWAIT, // FirstSafeContextualKeywordToken

Ditto

[Caitlin Potter (:caitp)]

Comment on attachment 294939 [details]
Patch

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

My approach to writing this patch, was to enable individual flagged sections with `|| 1`, benchmark, and make changes that seemed appropriate until the benchmark runs looked ~neutral. The biggest differences came from tokenizing "async" as a keyword (it's not 100% clear to me why `match(ASYNC)` is so much cheaper than the comparison with an intern'd string, but it seems to make a big difference), and branch removal in various areas (with some extra branch  removal added now per your comments).

The branch removal is a tradeoff (see my comment in parsePrimaryExpression() where IDENT: stuff is duplicated for ASYNC to avoid branching). It might be worth it to have the branching rather than increased code size.

(Marked comments as done, but obviously I've submitted these comments before uploading the patch).

>> Source/JavaScriptCore/parser/Parser.cpp:4283
>> +            // Avoid using variable if it is an arrow function parameter
> 
> Why is this needed? I thought there was code that handled this when parsing arrow functions

The stuff in parseAssignmentExpression() is only for parenthesized arrow functions, this is done for the single unparenthesized parameter case

>> Source/JavaScriptCore/parser/Parser.cpp:4287
>> +            const bool isEval = false;
> 
> Can you create a helper for this so this code and the code below won't ever fall out of sync with each other?

done

>> Source/JavaScriptCore/parser/Parser.cpp:4553
>> +                size_t usedVariablesSize = baseIsAsyncKeyword ? currentScope()->currentUsedVariablesSize() : 0;
> 
> This branch seems unnecessary since you only backtrack on the set when it's true below.

true, removed the ternary op

>> Source/JavaScriptCore/parser/Parser.h:131
>> +// _Any_CntextualKeyword includes keywords such as "let" or "yield", which have a specific meaning depending on the current parse mode
> 
> Typo "Cntextual"

Good catch, fixed

>> Source/JavaScriptCore/parser/Parser.h:139
>> +    return token.m_type == IDENT || (token.m_type >= FirstContextualKeywordToken && token.m_type <= LastContextualKeywordToken);
> 
> This could call isAnyContextualKeyword for the rhs of the "||". Otherwise, you should just remove that function since it's unused.

Changed to a call to `isAnyContextualKeyword()`

>> Source/JavaScriptCore/parser/Parser.h:141
>> +// _Safe_ContextualKeyword includes only contextual keywords which are independent from parse mode.
> 
> I'm confused by this. Why is this not a contradiction with your above definition of a contextual keyword?

I'm just not sure what to call it. It's basically "contextual keywords that aren't 'let' or 'yield'", but I don't have a good generalized way of saying that. The reason is so that `matchSpecIdentifier()` can eliminate `let` and `yield` by testing the parse modes, and then use this for the other contextual keyword cases.

Do you have an idea for a word that could describe this?

>> Source/JavaScriptCore/parser/ParserTokens.h:85
>> +    LET, // FirstContextualKeywordToken
> 
> This comment isn't needed given the definitions below.

convention picked up from v8, but will remove it here --- it's gone.

[Caitlin Potter (:caitp)]

Created attachment 294951 [details]
Patch

[Saam Barati]

Comment on attachment 294951 [details]
Patch

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

r=me

> Source/JavaScriptCore/parser/Parser.cpp:2679
> +UNUSED_PARAM(isAsync);

Can you add parsing tests for the various properties like:
async [computed]
async 20
etc.

> Source/JavaScriptCore/parser/Parser.cpp:2701
> +                if (UNLIKELY(isAsync) && !m_lexer->prevTerminator()) {

lets add a test for the newline case.

> Source/JavaScriptCore/parser/Parser.cpp:3380
> +            semanticFailIfFalse(match(FUNCTION) && !m_lexer->prevTerminator(), "Expected 'function' keyword following 'async' keyword with no preceding line terminator");

lets add a test for this.

[Caitlin Potter (:caitp)]

Created attachment 295055 [details]
Patch

[Caitlin Potter (:caitp)]

rebased this patch and added linefeed tests to async-function-syntax.js (which all pass).

Going to see if there's still a positive perf difference now.

[Yusuke Suzuki]

(In reply to comment #23)
> rebased this patch and added linefeed tests to async-function-syntax.js
> (which all pass).
> 
> Going to see if there's still a positive perf difference now.

Nice!

[Build Bot]

Comment on attachment 295055 [details]
Patch

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

New failing tests:
js/parser-syntax-check.html
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A8.1_T3.html

[Build Bot]

Created attachment 295063 [details]
Archive of layout-test-results from ews101 for mac-yosemite

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

[Build Bot]

Comment on attachment 295055 [details]
Patch

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

New failing tests:
js/parser-syntax-check.html
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A8.1_T3.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 295055 [details]
Patch

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

New failing tests:
js/parser-syntax-check.html
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A8.1_T3.html

[Build Bot]

Created attachment 295065 [details]
Archive of layout-test-results from ews115 for mac-yosemite

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

[Build Bot]

Comment on attachment 295055 [details]
Patch

Attachment 295055 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2532772

New failing tests:
js/parser-syntax-check.html
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A8.1_T3.html

[Build Bot]

Created attachment 295067 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Caitlin Potter (:caitp)]

Created attachment 295068 [details]
Patch

Jump directly to 'STRING' case if property name is a keyword that isn't explicitly handled

[Caitlin Potter (:caitp)]

I'm still seeing up to 1% boosts on closure|jquery, but it's not a huge difference. Up to you guys

[Caitlin Potter (:caitp)]

Created attachment 295072 [details]
Patch

[Build Bot]

Comment on attachment 295072 [details]
Patch

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

New failing tests:
js/parser-syntax-check.html
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A8.1_T3.html

[Build Bot]

Created attachment 295081 [details]
Archive of layout-test-results from ews102 for mac-yosemite

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

[Yusuke Suzuki]

Let's fix EWS failures.

[Build Bot]

Comment on attachment 295072 [details]
Patch

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

New failing tests:
js/parser-syntax-check.html
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A8.1_T3.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 295072 [details]
Patch

Attachment 295072 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2533208

New failing tests:
js/parser-syntax-check.html
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A8.1_T3.html

[Build Bot]

Created attachment 295083 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 295072 [details]
Patch

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

New failing tests:
js/parser-syntax-check.html
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A8.1_T3.html

[Build Bot]

Created attachment 295084 [details]
Archive of layout-test-results from ews116 for mac-yosemite

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

[Caitlin Potter (:caitp)]

Created attachment 295086 [details]
Patch

Fix the rest of parser-syntax-check.html

[Yusuke Suzuki]

Comment on attachment 295086 [details]
Patch

r=me, nice.

[Caitlin Potter (:caitp)]

(In reply to comment #46)
> Comment on attachment 295086 [details]
> Patch
> 
> r=me, nice.

Before landing this, would be good if anyone else sees an improvement in pare times. If it's neutral, it's not worth much.

[Saam Barati]

(In reply to comment #47)
> (In reply to comment #46)
> > Comment on attachment 295086 [details]
> > Patch
> > 
> > r=me, nice.
> 
> Before landing this, would be good if anyone else sees an improvement in
> pare times. If it's neutral, it's not worth much.

If you land this, I can monitor the bots to see if we measure an improvement. If it's neutral we could roll it out.

[Caitlin Potter (:caitp)]

(In reply to comment #48)
> (In reply to comment #47)
> > (In reply to comment #46)
> > > Comment on attachment 295086 [details]
> > > Patch
> > > 
> > > r=me, nice.
> > 
> > Before landing this, would be good if anyone else sees an improvement in
> > pare times. If it's neutral, it's not worth much.
> 
> If you land this, I can monitor the bots to see if we measure an
> improvement. If it's neutral we could roll it out.

Alright, will give it a try. If this does get rolled out, ping me and I'll add the new tests in a separate bug

[Caitlin Potter (:caitp)]

Created attachment 295276 [details]
Patch for landing

Rebased for clean merge

[WebKit Commit Bot]

Comment on attachment 295276 [details]
Patch for landing

Rejecting attachment 295276 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 295276, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in JSTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/2546142

[Caitlin Potter (:caitp)]

Created attachment 295277 [details]
Patch for landing

Rebased for clean merge #2

[WebKit Commit Bot]

Comment on attachment 295277 [details]
Patch for landing

Clearing flags on attachment: 295277

Committed r208933: <http://trac.webkit.org/changeset/208933>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.