| Summary: | [JSC] Recover parser performance regression by async support | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||
| Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | caitp, commit-queue, keith_miller, mark.lam, msaboff, saam | ||||
| Priority: | P2 | ||||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Yusuke Suzuki
2016-05-31 11:29:18 PDT
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>
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>
Created attachment 280165 [details]
Patch
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. 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). 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. (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() {}` I see on master, semanticFailureDueToKeyword() is guarded behind matchSpecIdentifier(), so it doesn't matter anymore! Should be okay. (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!! 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 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. Committed r201523: <http://trac.webkit.org/changeset/201523> 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. |