Bug 158228 – [JSC] Recover parser performance regression by async support

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

[Yusuke Suzuki]

:D

[Yusuke Suzuki]

clang also compiles our patch as expected!

The same parseInner case,

oneOfSourceParseModes<SourceParseMode::GeneratorBodyMode>(parseMode) || isAsyncFunctionBodyParseMode(parseMode)

    597b:       66 f7 c3 82 01          test   $0x182,%bx
    5980:       74 0a                   je     598c <JSC::Parser<JSC::Lexer<unsigned char> >::parseInner(JSC::Identifier const&, JSC::SourceParseMode)+0x1bc>

[Yusuke Suzuki]

parseMode == SourceParseMode::GeneratorBodyMode || isAsyncFunctionBodyParseMode(parseMode) is compiled by clang as follows,

    5967:       41 83 fc 02             cmp    $0x2,%r12d
    596b:       74 28                   je     5995 <JSC::Parser<JSC::Lexer<unsigned char> >::parseInner(JSC::Identifier const&, JSC::SourceParseMode)+0x1c5>
    596d:       44 89 e0                mov    %r12d,%eax
    5970:       25 80 01 00 00          and    $0x180,%eax
    5975:       66 85 c0                test   %ax,%ax
    5978:       75 b                    jne    5995 <JSC::Parser<JSC::Lexer<unsigned char> >::parseInner(JSC::Identifier const&, JSC::SourceParseMode)+0x1c5>

[Yusuke Suzuki]

Created attachment 280165 [details]
Patch

[Caitlin Potter (:caitp)]

Comment on attachment 280165 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:25
> +        <geometric>          2.32184+-0.01346    ^     2.24755+-0.00786       ^ definitely 1.0331x faster

Nice

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

I am pretty sure this will make variables named "await" illegal in normal functions, which is a web-breaking change.

[Saam Barati]

Comment on attachment 280165 [details]
Patch

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

r=me with a few comments

> Source/JavaScriptCore/parser/Parser.cpp:1933
> +    RELEASE_ASSERT(!(isOneOfSourceParseModes<SourceParseMode::ProgramMode, SourceParseMode::ModuleAnalyzeMode, SourceParseMode::ModuleEvaluateMode>(mode)));

I think this can be a debug assert

> Source/JavaScriptCore/parser/Parser.cpp:2113
> +    if (UNLIKELY((isOneOfSourceParseModes<SourceParseMode::ArrowFunctionMode, SourceParseMode::AsyncArrowFunctionMode>(mode)))) {

I'm not a big fan of the UNLIKELY here because it will penalize people using ArrowFunctions. Does removing the UNLIKELY here hurt us?
(The same goes for other isOneOfSourceParseModes<...> calls where ... includes arrow functions).

As a general principle, I'm also against all the UNLIKELY for async functions parsing going into the future. I'm OK with it now just because it's not in the specification yet. But as the spec is finalized and we turn it on by default, we shouldn't rely on UNLIKELY for helping us solve our perf needs. Hopefully we will find other optimizations by then.

> Source/JavaScriptCore/parser/ParserModes.h:96
> +    return isOneOfSourceParseModes<
> +        SourceParseMode::NormalFunctionMode,
> +        SourceParseMode::GeneratorBodyMode,
> +        SourceParseMode::GeneratorWrapperFunctionMode,
> +        SourceParseMode::GetterMode,
> +        SourceParseMode::SetterMode,
> +        SourceParseMode::MethodMode,
> +        SourceParseMode::ArrowFunctionMode,
> +        SourceParseMode::AsyncFunctionBodyMode,
> +        SourceParseMode::AsyncFunctionMode,
> +        SourceParseMode::AsyncMethodMode,
> +        SourceParseMode::AsyncArrowFunctionMode,
> +        SourceParseMode::AsyncArrowFunctionBodyMode>(parseMode);

Nit suggestion: This might be nicer if expressed this using a Set metaphor:
SourceParseModeSet(X, Y, Z).contains(parseMode)
(I don't know if we should name it SourceParseModeSet, but just an idea).

[Yusuke Suzuki]

Comment on attachment 280165 [details]
Patch

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

>> Source/JavaScriptCore/parser/ParserTokens.h:85
>> +    AWAIT,
> 
> I am pretty sure this will make variables named "await" illegal in normal functions, which is a web-breaking change.

Really? I can execute `function hello() { var await = 300; return await; }`  and `function hello() { 'use strict'; var await = 3000; return await; }` in JSC because JSC uses `matchSpecIdentifier()` for variable names.

[Caitlin Potter (:caitp)]

(In reply to comment #6)
> Comment on attachment 280165 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280165&action=review
> 
> >> Source/JavaScriptCore/parser/ParserTokens.h:85
> >> +    AWAIT,
> > 
> > I am pretty sure this will make variables named "await" illegal in normal functions, which is a web-breaking change.
> 
> Really? I can execute `function hello() { var await = 300; return await; }` 
> and `function hello() { 'use strict'; var await = 3000; return await; }` in
> JSC because JSC uses `matchSpecIdentifier()` for variable names.

There is (or used to be?) code which tests the KeywordFlag of the token, somewhere --- I know because I had to work around it when I was trying this the first time.

Maybe it's not necessary anymore, or it depends on context or something?

Try `function await() {}`

[Caitlin Potter (:caitp)]

I see on master, semanticFailureDueToKeyword() is guarded behind matchSpecIdentifier(), so it doesn't matter anymore! Should be okay.

[Yusuke Suzuki]

(In reply to comment #8)
> I see on master, semanticFailureDueToKeyword() is guarded behind
> matchSpecIdentifier(), so it doesn't matter anymore! Should be okay.

Great! Thanks for your notice and investigation, that's super nice to keep JSC's spec-compliance!!

[Yusuke Suzuki]

Comment on attachment 280165 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.cpp:1933
>> +    RELEASE_ASSERT(!(isOneOfSourceParseModes<SourceParseMode::ProgramMode, SourceParseMode::ModuleAnalyzeMode, SourceParseMode::ModuleEvaluateMode>(mode)));
> 
> I think this can be a debug assert

Right. Changed :)

>> Source/JavaScriptCore/parser/Parser.cpp:2113
>> +    if (UNLIKELY((isOneOfSourceParseModes<SourceParseMode::ArrowFunctionMode, SourceParseMode::AsyncArrowFunctionMode>(mode)))) {
> 
> I'm not a big fan of the UNLIKELY here because it will penalize people using ArrowFunctions. Does removing the UNLIKELY here hurt us?
> (The same goes for other isOneOfSourceParseModes<...> calls where ... includes arrow functions).
> 
> As a general principle, I'm also against all the UNLIKELY for async functions parsing going into the future. I'm OK with it now just because it's not in the specification yet. But as the spec is finalized and we turn it on by default, we shouldn't rely on UNLIKELY for helping us solve our perf needs. Hopefully we will find other optimizations by then.

I've just measured the performance with the version removing this UNLIKELY, L1937, and L2070's UNLIKELY.
The results are the following.

Generating benchmark report at /home/yusukesuzuki/dev/WebKit/baseline_patched_Octane_hanayamata_20160601_0451_report.txt
And raw data at /home/yusukesuzuki/dev/WebKit/baseline_patched_Octane_hanayamata_20160601_0451.json

Benchmark report for Octane on hanayamata.

VMs tested:
"baseline" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/async-target4/Release/bin/jsc
"patched" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/async-target5/Release/bin/jsc

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.

                         baseline                  patched                                      

closure              0.62675+-0.00295          0.62671+-0.00285       
jquery               8.06932+-0.04177          8.04615+-0.04079       

<geometric>          2.24885+-0.00787          2.24555+-0.00521         might be 1.0015x faster

It seems there is no perf regression, interesting. I guess that this is because these SourceParseMode ops (like isOneOfSourceParseModes and isAsync***) are now optimized and parseFunctionInfo's contribution becomes small... So I'll drop these "unlikely".

>> Source/JavaScriptCore/parser/ParserModes.h:96
>> +        SourceParseMode::AsyncArrowFunctionBodyMode>(parseMode);
> 
> Nit suggestion: This might be nicer if expressed this using a Set metaphor:
> SourceParseModeSet(X, Y, Z).contains(parseMode)
> (I don't know if we should name it SourceParseModeSet, but just an idea).

OK, thanks. I'll look into this metaphor, and once we figure out that it can be achieved with the expected code generation (in clang and GCC), I'll land with this change :D

[Yusuke Suzuki]

Comment on attachment 280165 [details]
Patch

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

>>> Source/JavaScriptCore/parser/ParserModes.h:96
>>> +        SourceParseMode::AsyncArrowFunctionBodyMode>(parseMode);
>> 
>> Nit suggestion: This might be nicer if expressed this using a Set metaphor:
>> SourceParseModeSet(X, Y, Z).contains(parseMode)
>> (I don't know if we should name it SourceParseModeSet, but just an idea).
> 
> OK, thanks. I'll look into this metaphor, and once we figure out that it can be achieved with the expected code generation (in clang and GCC), I'll land with this change :D

OK, I can achieve the good code generation with SourceParseModeSet().contains code. I'll land with this change.

[Yusuke Suzuki]

Committed r201523: <http://trac.webkit.org/changeset/201523>

[Caitlin Potter (:caitp)]

So for the remaining code load hit,

isDisallowedIdentifierAwait() is probably not generating concise code if it's ever inlined.

Maybe getting rid of boolean variables in Parser::Scope would enable using the improved codegen from that SourceParseModeSet tool, which would probably make that method a lot cheaper?

Some ideas, but will look at it more tomorrow.