WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
156147
[JSC] implement async functions proposal
https://bugs.webkit.org/show_bug.cgi?id=156147
Summary
[JSC] implement async functions proposal
Caitlin Potter (:caitp)
Reported
2016-04-03 13:51:01 PDT
[JSC] implement async functions proposal
Attachments
Patch
(185.35 KB, patch)
2016-04-03 14:19 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(185.89 KB, patch)
2016-04-03 14:45 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(186.33 KB, patch)
2016-04-03 15:09 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(190.53 KB, patch)
2016-04-03 18:59 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(187.50 KB, patch)
2016-04-03 19:16 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews100 for mac-yosemite
(1000.60 KB, application/zip)
2016-04-03 20:06 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2
(1013.98 KB, application/zip)
2016-04-03 20:10 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews125 for ios-simulator-wk2
(744.14 KB, application/zip)
2016-04-03 20:13 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews116 for mac-yosemite
(887.74 KB, application/zip)
2016-04-03 20:21 PDT
,
Build Bot
no flags
Details
Patch
(184.77 KB, patch)
2016-04-03 21:27 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(205.63 KB, patch)
2016-04-08 09:49 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews100 for mac-yosemite
(822.06 KB, application/zip)
2016-04-08 10:46 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2
(963.99 KB, application/zip)
2016-04-08 10:49 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews121 for ios-simulator-wk2
(683.22 KB, application/zip)
2016-04-08 10:52 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews114 for mac-yosemite
(883.36 KB, application/zip)
2016-04-08 10:57 PDT
,
Build Bot
no flags
Details
Patch
(205.30 KB, patch)
2016-04-08 11:36 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(204.77 KB, patch)
2016-04-08 11:57 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(238.90 KB, patch)
2016-04-09 12:22 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(238.79 KB, patch)
2016-04-09 15:52 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(238.75 KB, patch)
2016-04-11 08:11 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(241.10 KB, patch)
2016-04-14 12:58 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(243.39 KB, patch)
2016-04-16 15:55 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(243.11 KB, patch)
2016-04-19 14:29 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(243.93 KB, patch)
2016-04-25 21:24 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(243.60 KB, patch)
2016-04-29 08:52 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(244.79 KB, patch)
2016-05-02 11:58 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews112 for mac-yosemite
(324.25 KB, application/zip)
2016-05-02 12:49 PDT
,
Build Bot
no flags
Details
Patch
(247.58 KB, patch)
2016-05-02 15:01 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(245.37 KB, patch)
2016-05-06 15:12 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(245.44 KB, patch)
2016-05-06 17:49 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(249.89 KB, patch)
2016-05-11 20:52 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(250.88 KB, patch)
2016-05-12 09:34 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(250.83 KB, patch)
2016-05-18 06:33 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(249.55 KB, patch)
2016-05-21 19:03 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(249.37 KB, patch)
2016-05-25 15:46 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(256.41 KB, patch)
2016-05-26 16:05 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
(Old version)
(256.97 KB, patch)
2016-05-27 21:43 PDT
,
Caitlin Potter (:caitp)
no flags
Details
Formatted Diff
Diff
Patch
(39.88 KB, patch)
2016-05-30 07:00 PDT
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(48.26 KB, patch)
2016-05-30 11:15 PDT
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews102 for mac-yosemite
(935.70 KB, application/zip)
2016-05-30 12:02 PDT
,
Build Bot
no flags
Details
Patch
(49.38 KB, patch)
2016-05-31 03:28 PDT
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(49.38 KB, patch)
2016-05-31 04:04 PDT
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Show Obsolete
(39)
View All
Add attachment
proposed patch, testcase, etc.
Caitlin Potter (:caitp)
Comment 1
2016-04-03 14:19:04 PDT
Created
attachment 275508
[details]
Patch
Caitlin Potter (:caitp)
Comment 2
2016-04-03 14:29:55 PDT
Hi, I've come a long way with this, passing most of the tests from Mozilla's implementation, and numerous additional ones as well. There are some bits that I could use some pointers on, including: 1) I'm not sure if the intern'd keyword strings are used unless "async" or "await" are defined as keywords --- so my little `isAsyncKeyword()` and `isAwaitKeyword()` helpers might be more expensive than they should be. I guess tokenizing them as keywords always and using the strategy that the "yield" keyword uses is a better approach, but maybe it doesn't matter? 2) currently, home object is always loaded for async arrow functions, and put in the "generator" object --- this wasn't needed a few days ago, but after a recent rebase, it became necessary to pass the `async_arrow_functions_lexical_super_binding.js" test. I'd like to only do this when it's really needed (lexically scoped within a derived method or constructor). Could use some pointers on producing the right scope information to make this work. 3) for a big thing like this, it's a sought after feature that people are excited to use, but it might slow down the usual workflow a bit, and therefore might do with a compile-time flag, to prevent hurting code load performance for typical use. If a runtime flag is feasible, that would be a good option as well. I'm more accustomed to the V8 and Blink style of feature flags, so I'm not quite sure what to do here for that. Anyways, I'll do some self review and continue working on this during the week. Feedback is welcome.
Caitlin Potter (:caitp)
Comment 3
2016-04-03 14:45:42 PDT
Created
attachment 275509
[details]
Patch add AsyncFunctionPrototype.js to xcodeproj
Caitlin Potter (:caitp)
Comment 4
2016-04-03 15:09:18 PDT
Created
attachment 275510
[details]
Patch add AsyncFunctionPrototype.js to CMakeLists.txt
Sam Weinig
Comment 5
2016-04-03 15:10:47 PDT
Really cool!
Yusuke Suzuki
Comment 6
2016-04-03 17:29:48 PDT
great!! i can comment on the part using the generator system ;D
Caitlin Potter (:caitp)
Comment 7
2016-04-03 18:59:42 PDT
Created
attachment 275521
[details]
Patch refactor parser, maybe gets this building on EWS
Caitlin Potter (:caitp)
Comment 8
2016-04-03 19:16:59 PDT
Created
attachment 275522
[details]
Patch rebase, and hope it doesn't require more frontend changes
WebKit Commit Bot
Comment 9
2016-04-03 19:19:02 PDT
Attachment 275522
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/parser/ParserTokens.h:82: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/parser/ParserTokens.h:83: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/parser/Keywords.table:10: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/parser/Keywords.table:11: Line contains tab character. [whitespace/tab] [5] Total errors found: 4 in 45 files If any of these errors are false positives, please file a bug against check-webkit-style.
Build Bot
Comment 10
2016-04-03 20:06:48 PDT
Comment on
attachment 275522
[details]
Patch
Attachment 275522
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.webkit.org/results/1095387
Number of test failures exceeded the failure limit.
Build Bot
Comment 11
2016-04-03 20:06:54 PDT
Created
attachment 275523
[details]
Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Build Bot
Comment 12
2016-04-03 20:10:03 PDT
Comment on
attachment 275522
[details]
Patch
Attachment 275522
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.webkit.org/results/1095390
Number of test failures exceeded the failure limit.
Build Bot
Comment 13
2016-04-03 20:10:08 PDT
Created
attachment 275524
[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 14
2016-04-03 20:13:21 PDT
Comment on
attachment 275522
[details]
Patch
Attachment 275522
[details]
did not pass ios-sim-ews (ios-simulator-wk2): Output:
http://webkit-queues.webkit.org/results/1095391
Number of test failures exceeded the failure limit.
Build Bot
Comment 15
2016-04-03 20:13:25 PDT
Created
attachment 275525
[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.10.5
Ryosuke Niwa
Comment 16
2016-04-03 20:20:54 PDT
Comment on
attachment 275522
[details]
Patch Can we add a runtime flag? e.g.
http://trac.webkit.org/changeset/187356
removes a runtime flag for symbols. Also it looks like a whole bunch of layout tests are failing.
Build Bot
Comment 17
2016-04-03 20:21:30 PDT
Comment on
attachment 275522
[details]
Patch
Attachment 275522
[details]
did not pass mac-debug-ews (mac): Output:
http://webkit-queues.webkit.org/results/1095407
Number of test failures exceeded the failure limit.
Build Bot
Comment 18
2016-04-03 20:21:34 PDT
Created
attachment 275526
[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)
Comment 19
2016-04-03 21:27:04 PDT
Created
attachment 275529
[details]
Patch don't tokenize async/await as keywords, hopefully not costing too much perf
Caitlin Potter (:caitp)
Comment 20
2016-04-03 21:46:27 PDT
(In reply to
comment #16
)
> Comment on
attachment 275522
[details]
> Patch > > Can we add a runtime flag? e.g.
http://trac.webkit.org/changeset/187356
> removes a runtime flag for symbols. > > Also it looks like a whole bunch of layout tests are failing.
I've started doing this, but since it's a language feature and not just an API, the parser needs to know about the flag as well. A compile-time flag probably makes sense here, so that it's easy to turn off if there's a serious problem. I believe I've fixed the layout test failures
Geoffrey Garen
Comment 21
2016-04-04 09:04:40 PDT
Because this is a bleeding-edge language feature, I think we need a runtime flag to enable / disable it. For example, we will probably disable it in the Safari Technology Preview until it's more mature. It's OK if the flag doesn't cause all the code to be compiled out. The goal is just to avoid exposing a language feature to the web before it's mature. (There was some discussion on webkit-dev recently about runtime flags for new features. I think what I"m saying here is consistent with that discussion.)
Caitlin Potter (:caitp)
Comment 22
2016-04-04 13:05:29 PDT
(In reply to
comment #21
)
> Because this is a bleeding-edge language feature, I think we need a runtime > flag to enable / disable it. For example, we will probably disable it in the > Safari Technology Preview until it's more mature. > > It's OK if the flag doesn't cause all the code to be compiled out. The goal > is just to avoid exposing a language feature to the web before it's mature. > > (There was some discussion on webkit-dev recently about runtime flags for > new features. I think what I"m saying here is consistent with that > discussion.)
I agree whole-heartedly that a flag would be beneficial :) What I meant was, currently there's no apparent mechanism for attaching the RuntimeFlags class to the Parser (or the VM object, somewhat surprisingly). So, it looks like it would be a quicker fix to add a compile-time flag. The mechanism for adding runtime flags to the parser is a bit important, for example if multiple tests are run using the same VM asynchronously, some may depend on different sets of flags (a situation which affects V8's C++ tests). It may be possible to just pass in a flags option, but I'm not sure if the Parser is directly exposed to the API, or what. Happy to add the flag, could just use a little guidance on an approach that will work well for JSC/WebKit.
Saam Barati
Comment 23
2016-04-04 13:11:38 PDT
(In reply to
comment #19
)
> Created
attachment 275529
[details]
> Patch > > don't tokenize async/await as keywords, hopefully not costing too much perf
Can you test parser performance on this? Specifically, octane's jquery/closure benchmarks. I'm working on making our parser faster as we've regressed 10% since we've implemented arrow functions and I don't want to introduce anything that slows the parser down.
Caitlin Potter (:caitp)
Comment 24
2016-04-05 13:08:17 PDT
(In reply to
comment #23
)
> (In reply to
comment #19
) > > Created
attachment 275529
[details]
> > Patch > > > > don't tokenize async/await as keywords, hopefully not costing too much perf > > Can you test parser performance on this? Specifically, octane's > jquery/closure benchmarks. > > I'm working on making our parser faster as we've > regressed 10% since we've implemented arrow functions > and I don't want to introduce anything that slows the > parser down.
It was regressing CodeLoad by quite a lot. I've managed to get it down to about a ~5% regression on average with some improvements for arrow function and method parsing, and can probably squeeze a few more out of it in other ways.
Saam Barati
Comment 25
2016-04-05 13:50:55 PDT
(In reply to
comment #24
)
> (In reply to
comment #23
) > > (In reply to
comment #19
) > > > Created
attachment 275529
[details]
> > > Patch > > > > > > don't tokenize async/await as keywords, hopefully not costing too much perf > > > > Can you test parser performance on this? Specifically, octane's > > jquery/closure benchmarks. > > > > I'm working on making our parser faster as we've > > regressed 10% since we've implemented arrow functions > > and I don't want to introduce anything that slows the > > parser down. > > It was regressing CodeLoad by quite a lot. I've managed to get it down to > about a ~5% regression on average with some improvements for arrow function > and method parsing, and can probably squeeze a few more out of it in other > ways.
Based on this information I think it's worth having a compile time flag instead of a runtime flag. That way async functions are disabled we don't slow down the parser. Unless the runtime flag doesn't slow down the parser with async functions disabled. That said, I think it's worth getting the performance regression as down as possible because we will presumably remove the runtime/compile time flag at some point.
Geoffrey Garen
Comment 26
2016-04-05 14:44:58 PDT
> I agree whole-heartedly that a flag would be beneficial :) What I meant was, > currently there's no apparent mechanism for attaching the RuntimeFlags class > to the Parser (or the VM object, somewhat surprisingly). So, it looks like > it would be a quicker fix to add a compile-time flag.
It looks like the current API stores RuntimeFlags on the JSGlobalObject. So, we can pass them through from the global object to the parser.
> The mechanism for adding runtime flags to the parser is a bit important, for > example if multiple tests are run using the same VM asynchronously, some may > depend on different sets of flags (a situation which affects V8's C++ > tests). It may be possible to just pass in a flags option, but I'm not sure > if the Parser is directly exposed to the API, or what.
I don't think we need to worry about multiple tests using the same VM concurrently. We do concurrency for workers by allocating a new VM per thread, and we do concurrency in unit testing at the process level. I believe we create a new Parser for each parsing operation. I would assign flags to the Parser from the global object at Parser creation time.
Caitlin Potter (:caitp)
Comment 27
2016-04-07 09:51:01 PDT
(In reply to
comment #26
)
> > I agree whole-heartedly that a flag would be beneficial :) What I meant was, > > currently there's no apparent mechanism for attaching the RuntimeFlags class > > to the Parser (or the VM object, somewhat surprisingly). So, it looks like > > it would be a quicker fix to add a compile-time flag. > > It looks like the current API stores RuntimeFlags on the JSGlobalObject. So, > we can pass them through from the global object to the parser. > > > The mechanism for adding runtime flags to the parser is a bit important, for > > example if multiple tests are run using the same VM asynchronously, some may > > depend on different sets of flags (a situation which affects V8's C++ > > tests). It may be possible to just pass in a flags option, but I'm not sure > > if the Parser is directly exposed to the API, or what. > > I don't think we need to worry about multiple tests using the same VM > concurrently. We do concurrency for workers by allocating a new VM per > thread, and we do concurrency in unit testing at the process level. > > I believe we create a new Parser for each parsing operation. I would assign > flags to the Parser from the global object at Parser creation time.
I've been looking at doing this --- there are some unfortunate side affects (growing the size of every UnlinkedFunctionExecutable, for one). Is there any good reason not to just use JSC_OPTIONS for this? This would make it easy to enable during tests, without growing a bunch of classes
Caitlin Potter (:caitp)
Comment 28
2016-04-08 09:49:32 PDT
Created
attachment 276009
[details]
Patch
WebKit Commit Bot
Comment 29
2016-04-08 09:50:40 PDT
Attachment 276009
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/parser/ParserTokens.h:117: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/parser/ParserTokens.h:118: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/parser/Keywords.table:10: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/parser/Keywords.table:11: Line contains tab character. [whitespace/tab] [5] Total errors found: 4 in 50 files If any of these errors are false positives, please file a bug against check-webkit-style.
Build Bot
Comment 30
2016-04-08 10:46:32 PDT
Comment on
attachment 276009
[details]
Patch
Attachment 276009
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.webkit.org/results/1121468
New failing tests: js/parser-syntax-check.html js/arrowfunction-syntax.html js/class-syntax-semicolon.html
Build Bot
Comment 31
2016-04-08 10:46:37 PDT
Created
attachment 276012
[details]
Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Build Bot
Comment 32
2016-04-08 10:49:39 PDT
Comment on
attachment 276009
[details]
Patch
Attachment 276009
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.webkit.org/results/1121471
New failing tests: js/parser-syntax-check.html js/arrowfunction-syntax.html js/class-syntax-semicolon.html
Build Bot
Comment 33
2016-04-08 10:49:42 PDT
Created
attachment 276013
[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 34
2016-04-08 10:52:10 PDT
Comment on
attachment 276009
[details]
Patch
Attachment 276009
[details]
did not pass ios-sim-ews (ios-simulator-wk2): Output:
http://webkit-queues.webkit.org/results/1121470
New failing tests: js/parser-syntax-check.html js/arrowfunction-syntax.html js/class-syntax-semicolon.html
Build Bot
Comment 35
2016-04-08 10:52:15 PDT
Created
attachment 276014
[details]
Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5
Build Bot
Comment 36
2016-04-08 10:57:32 PDT
Comment on
attachment 276009
[details]
Patch
Attachment 276009
[details]
did not pass mac-debug-ews (mac): Output:
http://webkit-queues.webkit.org/results/1121477
New failing tests: js/parser-syntax-check.html js/arrowfunction-syntax.html
Build Bot
Comment 37
2016-04-08 10:57:37 PDT
Created
attachment 276015
[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
Caitlin Potter (:caitp)
Comment 38
2016-04-08 11:36:52 PDT
Created
attachment 276020
[details]
Patch
WebKit Commit Bot
Comment 39
2016-04-08 11:38:56 PDT
Attachment 276020
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/parser/ParserTokens.h:117: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/parser/ParserTokens.h:118: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/parser/Keywords.table:10: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/parser/Keywords.table:11: Line contains tab character. [whitespace/tab] [5] Total errors found: 4 in 50 files If any of these errors are false positives, please file a bug against check-webkit-style.
Caitlin Potter (:caitp)
Comment 40
2016-04-08 11:57:09 PDT
Created
attachment 276022
[details]
Patch
WebKit Commit Bot
Comment 41
2016-04-08 11:58:35 PDT
Attachment 276022
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/parser/ParserTokens.h:117: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/parser/ParserTokens.h:118: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/parser/Keywords.table:10: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/parser/Keywords.table:11: Line contains tab character. [whitespace/tab] [5] Total errors found: 4 in 50 files If any of these errors are false positives, please file a bug against check-webkit-style.
Caitlin Potter (:caitp)
Comment 42
2016-04-08 12:39:17 PDT
PTAL --- It's behind a flag now (JSC::Options rather than JSC::RuntimeFlags, to prevent adding members where they're rarely needed). I'll see if it does better on CodeLoad behind the flag.
Geoffrey Garen
Comment 43
2016-04-08 15:30:03 PDT
> > I believe we create a new Parser for each parsing operation. I would assign > > flags to the Parser from the global object at Parser creation time. > > I've been looking at doing this --- there are some unfortunate side affects > (growing the size of every UnlinkedFunctionExecutable, for one). > > Is there any good reason not to just use JSC_OPTIONS for this? This would > make it easy to enable during tests, without growing a bunch of classes
It would be nice to standardize on RuntimeFlags as the single place where we enable or disable features at runtime. Why does propagating a runtime flag from global object to parser require adding data to UnlinkedFunctionExecutable, and why does using a JSC::Option avoid doing so?
Caitlin Potter (:caitp)
Comment 44
2016-04-08 17:23:50 PDT
JSC::Options is initialized during JSC::initializeThreading(), and the flag state is stored statically --- RuntimeFlags is currently closely associated with JSGlobalObject, and the flags can be stored in a port-independent manner, retrieved via a method virtual method. For the Parser to get at these flags, something else holding the flag needs to be accessible --- even when a JSGlobalObject isn't (which is the case when Functions need to be reparsed, for example). Having lots of different copies of these runtime flags seems problematic in case they get out of sync somehow, so it's not necessarily the right way to go about setting these.
Caitlin Potter (:caitp)
Comment 45
2016-04-09 12:22:52 PDT
Created
attachment 276089
[details]
Patch Switch to using RuntimeFlags --- InspectorRuntimeAgent gets this wrong
WebKit Commit Bot
Comment 46
2016-04-09 12:24:27 PDT
Attachment 276089
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/jsc.cpp:2163: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/parser/ParserTokens.h:117: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/parser/ParserTokens.h:118: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/parser/Keywords.table:10: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/parser/Keywords.table:11: Line contains tab character. [whitespace/tab] [5] Total errors found: 5 in 66 files If any of these errors are false positives, please file a bug against check-webkit-style.
Caitlin Potter (:caitp)
Comment 47
2016-04-09 15:52:19 PDT
Created
attachment 276098
[details]
Patch minor fixup in win port that might fix ews
WebKit Commit Bot
Comment 48
2016-04-09 15:55:23 PDT
Attachment 276098
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/jsc.cpp:2163: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/parser/ParserTokens.h:117: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/parser/ParserTokens.h:118: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/parser/Keywords.table:10: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/parser/Keywords.table:11: Line contains tab character. [whitespace/tab] [5] Total errors found: 5 in 66 files If any of these errors are false positives, please file a bug against check-webkit-style.
Caitlin Potter (:caitp)
Comment 49
2016-04-11 08:11:47 PDT
Created
attachment 276150
[details]
Patch actually install AsyncFunction constructor when flag enabled
WebKit Commit Bot
Comment 50
2016-04-11 08:13:52 PDT
Attachment 276150
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/parser/ParserTokens.h:117: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/parser/ParserTokens.h:118: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/parser/Keywords.table:10: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/parser/Keywords.table:11: Line contains tab character. [whitespace/tab] [5] Total errors found: 4 in 66 files If any of these errors are false positives, please file a bug against check-webkit-style.
Caitlin Potter (:caitp)
Comment 51
2016-04-11 08:57:01 PDT
Comment on
attachment 276150
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=276150&action=review
> Source/JavaScriptCore/runtime/RuntimeFlags.h:35 > + macro(AsyncAwaitEnabled, "es7-async-await", false)
This stuff is added to be able to enable the flag individually in jsc.cpp --- might be better to just mark it as "enabled" so that jsc uses the experimental flag always. But it seems like it gives the wrong impression. "createAllEnabled()" should probably be more like "createAllExperimental()" or something, since it's only used by jsc, and only makes sense for jsc. If that's preferred, let me know
Caitlin Potter (:caitp)
Comment 52
2016-04-14 12:58:46 PDT
Created
attachment 276413
[details]
Patch Keep async arrow functions working, fixup some syntax error logic, and add tests for some module reserved word errors
Caitlin Potter (:caitp)
Comment 53
2016-04-14 13:00:55 PDT
Comment on
attachment 276413
[details]
Patch I think this is good to check in --- from recent tests on a 2015 macbook pro, CodeLoad regressions are marginal if present at all --- and the feature flag behaviour isn't great, but it does enable testing the feature in the `jsc` command line app.
WebKit Commit Bot
Comment 54
2016-04-14 13:01:30 PDT
Attachment 276413
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/parser/ParserTokens.h:117: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/parser/ParserTokens.h:118: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/parser/Keywords.table:10: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/parser/Keywords.table:11: Line contains tab character. [whitespace/tab] [5] Total errors found: 4 in 67 files If any of these errors are false positives, please file a bug against check-webkit-style.
Saam Barati
Comment 55
2016-04-14 13:04:04 PDT
Hi Caitlin, I'm traveling today but will make an effort to review tomorrow.
Caitlin Potter (:caitp)
Comment 56
2016-04-16 15:55:18 PDT
Created
attachment 276565
[details]
Patch remove comments from test and re-up
WebKit Commit Bot
Comment 57
2016-04-16 15:57:46 PDT
Attachment 276565
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/parser/ParserTokens.h:117: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/parser/ParserTokens.h:118: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/parser/Keywords.table:10: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/parser/Keywords.table:11: Line contains tab character. [whitespace/tab] [5] Total errors found: 4 in 69 files If any of these errors are false positives, please file a bug against check-webkit-style.
Saam Barati
Comment 58
2016-04-19 12:52:13 PDT
Comment on
attachment 276565
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=276565&action=review
A couple fly by comments. Will look more later.
> Source/JavaScriptCore/builtins/AsyncFunctionPrototype.js:33 > + const Completed = -1; > + const Executing = -2; > + > + const NormalMode = 0; > + const ThrowMode = 2;
Yusuke recently made these intrinsic values in byte code generator for generatorResume. I think we should do the same here for consistency.
> Source/JavaScriptCore/runtime/JSAsyncFunction.h:1 > +/*
Do we really need a separate class for this? It appears that this adds no new fields. I haven't looked in great detail of where this is used.
Caitlin Potter (:caitp)
Comment 59
2016-04-19 14:02:22 PDT
Comment on
attachment 276565
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=276565&action=review
>> Source/JavaScriptCore/builtins/AsyncFunctionPrototype.js:33 >> + const ThrowMode = 2; > > Yusuke recently made these intrinsic values in byte code generator for generatorResume. I think we should do the same here for consistency.
Looks good, will do in the next patchset
>> Source/JavaScriptCore/runtime/JSAsyncFunction.h:1 >> +/* > > Do we really need a separate class for this? It appears that this adds no new fields. > I haven't looked in great detail of where this is used.
Well, at the very least it needs a different structure and prototype from plain JSFunctions --- this just seemed like the simplest way to get that. If there's a better way, I'll modify it
Caitlin Potter (:caitp)
Comment 60
2016-04-19 14:29:15 PDT
Created
attachment 276758
[details]
Patch use BytecodeIntrinsic constants instead in self hosted wrapper code
WebKit Commit Bot
Comment 61
2016-04-19 14:32:22 PDT
Attachment 276758
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/parser/ParserTokens.h:117: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/parser/ParserTokens.h:118: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/JavaScriptCore/parser/Keywords.table:10: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/parser/Keywords.table:11: Line contains tab character. [whitespace/tab] [5] Total errors found: 4 in 69 files If any of these errors are false positives, please file a bug against check-webkit-style.
Caitlin Potter (:caitp)
Comment 62
2016-04-25 21:24:57 PDT
Created
attachment 277330
[details]
Patch Fix stylechecker errors to avoid spam, rebased to keep it fresh
Caitlin Potter (:caitp)
Comment 63
2016-04-29 08:52:39 PDT
Created
attachment 277702
[details]
Patch weekly rebase...
Yusuke Suzuki
Comment 64
2016-04-30 07:58:09 PDT
Comment on
attachment 277702
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=277702&action=review
super good start! I don't still look through. Quick comments. I'll look into it more. And I suggest you to measure Octane code-load. That measures the JS parser's performance.
> Source/JavaScriptCore/parser/Keywords.table:11 > +await AWAIT
Are async and await keywords? I think they are contextual keywords, correct?
> Source/JavaScriptCore/parser/Parser.cpp:357 > + bool isIdent = matchSpecIdentifier();
Nice catch.
> Source/JavaScriptCore/parser/Parser.cpp:598 > + if (!(match(IDENT) || match(ASYNC) || match(AWAIT) || match(LET) || match(YIELD)) && !match(OPENBRACE) && !match(OPENBRACKET))
If ASYNC is contextual keyword, we can use matchContextualKeyword function :)
> Source/JavaScriptCore/runtime/RuntimeFlags.h:35 > + macro(AsyncAwaitEnabled, true)
Nice.
Caitlin Potter (:caitp)
Comment 65
2016-04-30 19:54:18 PDT
Comment on
attachment 277702
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=277702&action=review
>> Source/JavaScriptCore/parser/Keywords.table:11 >> +await AWAIT > > Are async and await keywords? I think they are contextual keywords, correct?
A number of versions ago, they were treated as contextual keywords (and tokenized as IDENTs) --- but they are checked in so many contexts, that it seems worthwhile to internalize them and tokenize them separately from identifiers --- It may be a close call, but I think when the flag is enabled, it makes more sense to do this. Contexts where `async` is potentially a keyword include any PrimaryExpression, any AssignmentExpression, and any StatementListItem (including in module code) --- nearly anywhere a BindingIdentifier is expected. For `await`, it might make a bit more sense to use `matchContextualKeyword`, though, you're right. Thanks for taking a look at this, I appreciate it Yusuke :)
Yusuke Suzuki
Comment 66
2016-05-01 09:04:00 PDT
Comment on
attachment 277702
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=277702&action=review
Second round. I'll look into the parser changes next :)
> Source/JavaScriptCore/builtins/AsyncFunctionPrototype.js:30 > + let value = @undefined;
Let's insert if (typeof state !== 'number') throw new @TypeError("|this| should be an async function"); test here.
>>> Source/JavaScriptCore/parser/Keywords.table:11 >>> +await AWAIT >> >> Are async and await keywords? I think they are contextual keywords, correct? > > A number of versions ago, they were treated as contextual keywords (and tokenized as IDENTs) --- but they are checked in so many contexts, that it seems worthwhile to internalize them and tokenize them separately from identifiers --- It may be a close call, but I think when the flag is enabled, it makes more sense to do this. > > Contexts where `async` is potentially a keyword include any PrimaryExpression, any AssignmentExpression, and any StatementListItem (including in module code) --- nearly anywhere a BindingIdentifier is expected. > > For `await`, it might make a bit more sense to use `matchContextualKeyword`, though, you're right. > > Thanks for taking a look at this, I appreciate it Yusuke :)
I have some concerns about adding "async" and "await" into this keyword category. Currently, Keywords.table only holds keywords. Adding (strictly speaking)non-keyword words into this table seems confusing to readers. For example, "eval", "arguments" are handled like keywords under the strict mode, but it is not included in this table. "async" is so widely accepted word. So "async" is frequently used as a variable, a method name, a parameter etc. If we don't include "async" in IDENT, once we use "IDENT" in the current parser, we easily miss "async" word, and it will break many pages. Seeing
http://tc39.github.io/ecmascript-asyncawait/#identifier-static-semantics-early-errors
, "await" can be introduced into the keywords. But this handling does not include "async". I think it is better that "async" is handled in IDENT at least.
> Source/JavaScriptCore/parser/Parser.cpp:2130 > + AllowAwaitOverride allowAwait(this, !isAsyncFunctionParseMode(mode));
Nice. We can use SetForScope here :)
> Source/JavaScriptCore/runtime/CodeCache.cpp:156 > source, name.string(), SourceCodeKey::FunctionType,
We need to add RuntimeFlats into SourceCodeKey.
> Source/JavaScriptCore/runtime/CodeCache.cpp:200 > + if (isAsyncFunctionParseMode(metadata->parseMode()))
Let's add generator and arrow function cases here.
> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:490 > + putDirectWithoutTransition(vm, vm.propertyNames->AsyncFunction, asyncFunctionConstructor, DontEnum);
AsyncFunction constructor is not exposed as a global variable. (It is the same to the GeneratorFunction constructor). For example, Object constructor,
https://tc39.github.io/ecma262/#sec-object-constructor
"... and the initial value of the Object property of the global object." Array constructor
https://tc39.github.io/ecma262/#sec-array-constructor
"... and the initial value of the Array property of the global object." But, GeneratorFunction constructor and AsyncFunction constructor do not have such statements.
> Source/JavaScriptCore/tests/es6/async_arrow_functions_lexical_this_binding.js:27 > +shouldBeAsync("barley", () => e.y("ley"));
Let's move these tests to tests/stress directory. es6 is used for kangax's compat-table tests.
Yusuke Suzuki
Comment 67
2016-05-01 09:24:24 PDT
Comment on
attachment 277702
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=277702&action=review
>> Source/JavaScriptCore/builtins/AsyncFunctionPrototype.js:30 >> + let value = @undefined; > > Let's insert > > if (typeof state !== 'number') > throw new @TypeError("|this| should be an async function"); > > test here.
Ah, I misunderstood. Here, we can ensure that this `generator` is always a generator.
Caitlin Potter (:caitp)
Comment 68
2016-05-02 11:56:45 PDT
Comment on
attachment 277702
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=277702&action=review
>>>> Source/JavaScriptCore/parser/Keywords.table:11 >>>> +await AWAIT >>> >>> Are async and await keywords? I think they are contextual keywords, correct? >> >> A number of versions ago, they were treated as contextual keywords (and tokenized as IDENTs) --- but they are checked in so many contexts, that it seems worthwhile to internalize them and tokenize them separately from identifiers --- It may be a close call, but I think when the flag is enabled, it makes more sense to do this. >> >> Contexts where `async` is potentially a keyword include any PrimaryExpression, any AssignmentExpression, and any StatementListItem (including in module code) --- nearly anywhere a BindingIdentifier is expected. >> >> For `await`, it might make a bit more sense to use `matchContextualKeyword`, though, you're right. >> >> Thanks for taking a look at this, I appreciate it Yusuke :) > > I have some concerns about adding "async" and "await" into this keyword category. > Currently, Keywords.table only holds keywords. Adding (strictly speaking)non-keyword words into this table seems confusing to readers. > For example, "eval", "arguments" are handled like keywords under the strict mode, but it is not included in this table. > > "async" is so widely accepted word. So "async" is frequently used as a variable, a method name, a parameter etc. > If we don't include "async" in IDENT, once we use "IDENT" in the current parser, we easily miss "async" word, and it will break many pages. > > Seeing
http://tc39.github.io/ecmascript-asyncawait/#identifier-static-semantics-early-errors
, "await" can be introduced into the keywords. > But this handling does not include "async". I think it is better that "async" is handled in IDENT at least.
Okay --- I've changed it back to the contextual keyword style approach
>> Source/JavaScriptCore/parser/Parser.cpp:2130 >> + AllowAwaitOverride allowAwait(this, !isAsyncFunctionParseMode(mode)); > > Nice. We can use SetForScope here :)
Switched to using this, so there are fewer lines added in Parser.h now
>> Source/JavaScriptCore/runtime/CodeCache.cpp:156 >> source, name.string(), SourceCodeKey::FunctionType, > > We need to add RuntimeFlats into SourceCodeKey.
I've added it --- does it need to be used to calculate the hash, or just to determine if a given hash is the right one?
>> Source/JavaScriptCore/runtime/CodeCache.cpp:200 >> + if (isAsyncFunctionParseMode(metadata->parseMode())) > > Let's add generator and arrow function cases here.
I've changed this to a switch statement over the parseMode(), covering each type, and RELEASE_ASSERT_NOT_REACHED() for module/program code (I meant to remove RELEASE_* prefix, but maybe it doesn't matter a lot?).
>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:490 >> + putDirectWithoutTransition(vm, vm.propertyNames->AsyncFunction, asyncFunctionConstructor, DontEnum); > > AsyncFunction constructor is not exposed as a global variable. (It is the same to the GeneratorFunction constructor). > For example, > > Object constructor, >
https://tc39.github.io/ecma262/#sec-object-constructor
> "... and the initial value of the Object property of the global object." > > Array constructor >
https://tc39.github.io/ecma262/#sec-array-constructor
> "... and the initial value of the Array property of the global object." > > But, GeneratorFunction constructor and AsyncFunction constructor do not have such statements.
Updated to no longer install AsyncFunction to the global object
>> Source/JavaScriptCore/tests/es6/async_arrow_functions_lexical_this_binding.js:27 >> +shouldBeAsync("barley", () => e.y("ley")); > > Let's move these tests to tests/stress directory. es6 is used for kangax's compat-table tests.
They've been moved, and doing so found some new bugs, so that's good.
Caitlin Potter (:caitp)
Comment 69
2016-05-02 11:58:37 PDT
Created
attachment 277919
[details]
Patch addressed Yusuke Suzukis comments
Build Bot
Comment 70
2016-05-02 12:49:07 PDT
Comment on
attachment 277919
[details]
Patch
Attachment 277919
[details]
did not pass mac-debug-ews (mac): Output:
http://webkit-queues.webkit.org/results/1257076
Number of test failures exceeded the failure limit.
Build Bot
Comment 71
2016-05-02 12:49:12 PDT
Created
attachment 277925
[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
Caitlin Potter (:caitp)
Comment 72
2016-05-02 15:01:26 PDT
Created
attachment 277938
[details]
Patch Fix bugs from last patchset, add some new tests
Yusuke Suzuki
Comment 73
2016-05-06 10:36:15 PDT
Comment on
attachment 277938
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=277938&action=review
Nice. Seeing the code, it seems that AsyncArrowFunctionMode / AsyncArrowFunctionBodyMode cases are sometimes missing. Can AsyncArrowFunctionBodyMode become an arrow function?
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:245 > + bool containsArrowOrEvalButNotInArrowBlock = ((functionNode->usesArrowFunction() && functionNode->doAnyInnerArrowFunctionsUseAnyFeature()) || functionNode->usesEval()) && (!m_codeBlock->isArrowFunction() && !m_codeBlock->isAsyncArrowFunction());
Can we change this to the new `isArrowFunction()` including ArrowFunction / AsyncArrowFunction? Or is there any obstacles?
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:624 > + emitLabel(didNotThrow.get());
This phase is used when we encounter `async function (param = throwError()) { }`, right? Looks correct.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:630 > + if (SourceParseMode::ArrowFunctionMode == parseMode || SourceParseMode::AsyncArrowFunctionBodyMode == parseMode || SourceParseMode::AsyncArrowFunctionMode == parseMode || SourceParseMode::AsyncArrowFunctionMode == parseMode) {
AsyncArrowFunctionMode is duplicated.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:631 > + if (functionNode->usesThis() || functionNode->usesSuperProperty() || isSuperUsedInInnerArrowFunction())
Nice.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2906 > + ASSERT(func->metadata()->parseMode() == SourceParseMode::ArrowFunctionMode || func->metadata()->parseMode() == SourceParseMode::AsyncArrowFunctionMode);
Is it safe for AsyncArrowFunctionBodyMode with single line expression?
> Source/JavaScriptCore/parser/Parser.cpp:255 > + bool isArrowFunctionBodyExpression = parseMode == SourceParseMode::AsyncArrowFunctionBodyMode && !match(OPENBRACE);
Can we move this `parseMode == SourceParseMode::AsyncArrowFunctionBodyMode` to L263?
> Source/JavaScriptCore/parser/Parser.cpp:552 > + asyncFunctionBodyScope->setSourceParseMode(innerParseMode);
This part is changed in the latest trunk, so rebasing. :)
> Source/JavaScriptCore/parser/Parser.cpp:619 > + // Eagerly parse as AsyncFUnctionDeclaration. This is the uncommon case,
Typo: AsyncFUnctionDeclaration => AsyncFunctionDeclaration
> Source/JavaScriptCore/parser/Parser.cpp:625 > + result = parseAsyncFunctionDeclaration(context);
Let's break here, call restoreSavePoint(savePoint); and fall through to the await / field path.
> Source/JavaScriptCore/parser/Parser.cpp:1796 > + result = parseAsyncFunctionDeclaration(context);
These code is largely the same to the FUNCTION case. So extracting this to some function is recommended.
> Source/JavaScriptCore/parser/Parser.cpp:1797 > + }
Nit: breaking here may clean up the following code :)
> Source/JavaScriptCore/parser/Parser.cpp:2268 > + if (mode != SourceParseMode::AsyncArrowFunctionMode) { > + semanticFailIfTrue(generatorBodyScope->hasDirectSuper(), "super is not valid in this context"); > + if (generatorBodyScope->needsSuperBinding()) > + semanticFailIfTrue(expectedSuperBinding == SuperBinding::NotNeeded, "super can only be used in a method of a derived class"); > + } else { > + if (generatorBodyScope->hasDirectSuper()) > + functionScope->setHasDirectSuper(); > + if (generatorBodyScope->needsSuperBinding()) > + functionScope->setNeedsSuperBinding(); > + }
These checks are now done in parsing location. Let's remove this. When removing this, maybe, we need to recheck that closestParentOrdinaryFunctionNonLexicalScope works correctly with async arrow wrapper / body functions.
> Source/JavaScriptCore/parser/Parser.cpp:3197 > + next();
Is it necessary to create a save point here? Can we just call `semanticFailIfFalse(match(FUNCTION) && !m_lexer->prevTerminator(), "...")` ?
> Source/JavaScriptCore/parser/Parser.cpp:3318 > }
Handling async arrow function here seems natural :)
> Source/JavaScriptCore/parser/Parser.cpp:3602 > + }
Is this safe for `async get value() { }`?
> Source/JavaScriptCore/parser/Parser.cpp:4014 > + }
Which tests need this?
> Source/JavaScriptCore/parser/Parser.cpp:4221 > + failIfFalse(base, "Failed to parse async function expression");
Why is this handled here? Can we handle `async function` case in parsePrimaryExpression? And can we parse async arrow function in parseAssignmentExpression? (At that case, we check `async IDENT =>` in parsePrimaryExpression similar to the current arrow function parsing).
https://tc39.github.io/ecmascript-asyncawait/#prod-UnaryExpression
> Source/JavaScriptCore/runtime/CodeCache.cpp:226 > + ConstructAbility constructAbility; > + switch (metadata->parseMode()) { > + case SourceParseMode::NormalFunctionMode: > + // let allocKind be "constructor". > + constructAbility = ConstructAbility::CanConstruct; > + break; > + > + case SourceParseMode::GeneratorBodyMode: > + case SourceParseMode::GeneratorWrapperFunctionMode: > + case SourceParseMode::GetterMode: > + case SourceParseMode::SetterMode: > + case SourceParseMode::MethodMode: > + case SourceParseMode::ArrowFunctionMode: > + case SourceParseMode::AsyncFunctionBodyMode: > + case SourceParseMode::AsyncArrowFunctionBodyMode: > + case SourceParseMode::AsyncFunctionMode: > + case SourceParseMode::AsyncMethodMode: > + case SourceParseMode::AsyncArrowFunctionMode: > + // let allocKind be "non-constructor". > + constructAbility = ConstructAbility::CannotConstruct; > + break; > + > + case SourceParseMode::ProgramMode: > + case SourceParseMode::ModuleAnalyzeMode: > + case SourceParseMode::ModuleEvaluateMode: > + RELEASE_ASSERT_NOT_REACHED(); > + break;
Could you extract this to static function?
> Source/JavaScriptCore/runtime/CommonIdentifiers.h:36 > + macro(AsyncFunction) \
Let's remove this.
Caitlin Potter (:caitp)
Comment 74
2016-05-06 11:07:36 PDT
Comment on
attachment 277938
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=277938&action=review
I'll have these addressed shortly, thanks for the look
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:630 >> + if (SourceParseMode::ArrowFunctionMode == parseMode || SourceParseMode::AsyncArrowFunctionBodyMode == parseMode || SourceParseMode::AsyncArrowFunctionMode == parseMode || SourceParseMode::AsyncArrowFunctionMode == parseMode) { > > AsyncArrowFunctionMode is duplicated.
Oops, good catch
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2906 >> + ASSERT(func->metadata()->parseMode() == SourceParseMode::ArrowFunctionMode || func->metadata()->parseMode() == SourceParseMode::AsyncArrowFunctionMode); > > Is it safe for AsyncArrowFunctionBodyMode with single line expression?
AsyncArrowFunctionBodyMode never gets here, since it's just a normal function
>> Source/JavaScriptCore/parser/Parser.cpp:255 >> + bool isArrowFunctionBodyExpression = parseMode == SourceParseMode::AsyncArrowFunctionBodyMode && !match(OPENBRACE); > > Can we move this `parseMode == SourceParseMode::AsyncArrowFunctionBodyMode` to L263?
Unfortunately, this matters whether the function is being re-parsed or not --- this breaks if this isn't known here. There might be a better place to move this to, but it can't be in the `isReparsingFunction()` conditional.
>> Source/JavaScriptCore/parser/Parser.cpp:619 >> + // Eagerly parse as AsyncFUnctionDeclaration. This is the uncommon case, > > Typo: AsyncFUnctionDeclaration => AsyncFunctionDeclaration
Right, thanks
>> Source/JavaScriptCore/parser/Parser.cpp:625 >> + result = parseAsyncFunctionDeclaration(context); > > Let's break here, call restoreSavePoint(savePoint); and fall through to the await / field path.
Alright
>> Source/JavaScriptCore/parser/Parser.cpp:1796 >> + result = parseAsyncFunctionDeclaration(context); > > These code is largely the same to the FUNCTION case. So extracting this to some function is recommended.
Sorry, can you clarify what you mean here? I'm not sure if you're referring to this entire case statement, or the inside of `parseAsyncFunctionDeclaration` (which could share some code with parseFunctionDeclaration maybe), or what
>> Source/JavaScriptCore/parser/Parser.cpp:1797 >> + } > > Nit: breaking here may clean up the following code :)
Okay
>> Source/JavaScriptCore/parser/Parser.cpp:2268 >> + } > > These checks are now done in parsing location. Let's remove this. > When removing this, maybe, we need to recheck that closestParentOrdinaryFunctionNonLexicalScope works correctly with async arrow wrapper / body functions.
Yeah, this stuff is gone in the rebase so far --- and seems to do the right thing still
>> Source/JavaScriptCore/parser/Parser.cpp:3197 >> + next(); > > Is it necessary to create a save point here? Can we just call `semanticFailIfFalse(match(FUNCTION) && !m_lexer->prevTerminator(), "...")` ?
Re-reading the spec, it looks like you're right --- I was thinking ASI would matter here, but I suppose not. Good catch!
>> Source/JavaScriptCore/parser/Parser.cpp:4014 >> + } > > Which tests need this?
Ah --- previously, I had rewritten the arrow function parsing stuff to pay attention to the error classifier, which potentially saves it a bit of time. However at this point, this is dead code, I thought I had removed this already. The goal was to help reparse arrow functions less frequently, but at this point it's unnecessary
>> Source/JavaScriptCore/parser/Parser.cpp:4221 >> + failIfFalse(base, "Failed to parse async function expression"); > > Why is this handled here? Can we handle `async function` case in parsePrimaryExpression? > And can we parse async arrow function in parseAssignmentExpression? (At that case, we check `async IDENT =>` in parsePrimaryExpression similar to the current arrow function parsing). > >
https://tc39.github.io/ecmascript-asyncawait/#prod-UnaryExpression
We do parse async arrow functions in `parseAssignmentExpression`. I can probably move the AsyncFunctionExpression parsing to `parsePrimaryExpression`, though.
>> Source/JavaScriptCore/runtime/CommonIdentifiers.h:36 >> + macro(AsyncFunction) \ > > Let's remove this.
Okay
Caitlin Potter (:caitp)
Comment 75
2016-05-06 15:12:22 PDT
Created
attachment 278277
[details]
Patch Address today's comments from Yusuke --- except for the ones where I was unclear on what was being asked for
Caitlin Potter (:caitp)
Comment 76
2016-05-06 17:49:05 PDT
Created
attachment 278300
[details]
Patch make efl-wk2 happy
Yusuke Suzuki
Comment 77
2016-05-09 09:46:20 PDT
Comment on
attachment 277938
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=277938&action=review
>>> Source/JavaScriptCore/parser/Parser.cpp:1796 >>> + result = parseAsyncFunctionDeclaration(context); >> >> These code is largely the same to the FUNCTION case. So extracting this to some function is recommended. > > Sorry, can you clarify what you mean here? > > I'm not sure if you're referring to this entire case statement, or the inside of `parseAsyncFunctionDeclaration` (which could share some code with parseFunctionDeclaration maybe), or what
Ah, sorry. I mean this case statement. (This case clause and FUNCTION cases are function statement special cases, right?)
Caitlin Potter (:caitp)
Comment 78
2016-05-09 09:53:39 PDT
Comment on
attachment 277938
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=277938&action=review
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:245 >> + bool containsArrowOrEvalButNotInArrowBlock = ((functionNode->usesArrowFunction() && functionNode->doAnyInnerArrowFunctionsUseAnyFeature()) || functionNode->usesEval()) && (!m_codeBlock->isArrowFunction() && !m_codeBlock->isAsyncArrowFunction()); > > Can we change this to the new `isArrowFunction()` including ArrowFunction / AsyncArrowFunction? > Or is there any obstacles?
I was considering adding a `isAnyArrowFunction()`, just in case there were things that needed to distinguish between the two --- but I could try making `IsArrowFunction()` cover both cases, and add `IsSyncArrowFunction()` to cover just non-async cases if it's needed
>>>> Source/JavaScriptCore/parser/Parser.cpp:1796 >>>> + result = parseAsyncFunctionDeclaration(context); >>> >>> These code is largely the same to the FUNCTION case. So extracting this to some function is recommended. >> >> Sorry, can you clarify what you mean here? >> >> I'm not sure if you're referring to this entire case statement, or the inside of `parseAsyncFunctionDeclaration` (which could share some code with parseFunctionDeclaration maybe), or what > > Ah, sorry. I mean this case statement. (This case clause and FUNCTION cases are function statement special cases, right?)
So would you envision something like ``` case FUNCTION: const bool isAsync = false; result = parseFunctionDeclarationStatement(context, isAsync); break; case IDENT: if ( /* ... is `async` conditional keyword */ ) { next(); const bool isAsync = true; result = parseFunctionDeclarationStatement(context, isAsync); break; } /* fall through to expression-or-label-statement handling */ ``` --- something like that, so that the duplicated parts are only generated once?
>> Source/JavaScriptCore/parser/Parser.cpp:3602 >> + } > > Is this safe for `async get value() { }`?
Added an edit which makes this safe (as far as I can tell)
>> Source/JavaScriptCore/runtime/CodeCache.cpp:226 >> + break; > > Could you extract this to static function?
Done
Caitlin Potter (:caitp)
Comment 79
2016-05-11 20:52:22 PDT
Created
attachment 278692
[details]
Patch addressed that last comment
Yusuke Suzuki
Comment 80
2016-05-12 08:59:26 PDT
Comment on
attachment 278692
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=278692&action=review
Nice clean up! Sorry for my late review process. I have a time on week end :)
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:245 > + bool containsArrowOrEvalButNotInArrowBlock = ((functionNode->usesArrowFunction() && functionNode->doAnyInnerArrowFunctionsUseAnyFeature()) || functionNode->usesEval()) && (!m_codeBlock->isArrowFunction() && !m_codeBlock->isAsyncArrowFunction());
Now, isArrowFunction() includes isAsyncArrowFunction, let's drop isAsyncArrowFunction. :)
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:249 > + bool needsArguments = (functionNode->usesArguments() || codeBlock->usesEval() || (functionNode->usesArrowFunction() && !codeBlock->isArrowFunction() && !codeBlock->isAsyncArrowFunction() && isArgumentsUsedInInnerArrowFunction()));
Ditto.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:509 > + if (!haveParameterNamedArguments && !m_codeBlock->isArrowFunction() && !m_codeBlock->isAsyncArrowFunction()) {
isAsyncArrowFunction is included in isArrowFunction().
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:630 > + if (SourceParseMode::ArrowFunctionMode == parseMode || SourceParseMode::AsyncArrowFunctionBodyMode == parseMode || SourceParseMode::AsyncArrowFunctionMode == parseMode) {
Is this AsyncArrowFunctionBodyMode correct?
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:638 > + if (needsToUpdateArrowFunctionContext() && !codeBlock->isArrowFunction() && !codeBlock->isAsyncArrowFunction()) {
isAsyncArrowFunction is included in isArrowFunction()
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4313 > + ASSERT(m_codeBlock->isArrowFunction() || parseMode() == SourceParseMode::AsyncArrowFunctionBodyMode || parseMode() == SourceParseMode::AsyncArrowFunctionMode || m_codeBlock->isArrowFunctionContext() || constructorKind() == ConstructorKind::Derived || m_codeType == EvalCode);
Is this `SourceParseMode::AsyncArrowFunctionBodyMode` correct?
> Source/JavaScriptCore/parser/ASTBuilder.h:386 > + usesArrowFunction();
Is this usesArrowFunction() here correct?
> Source/JavaScriptCore/parser/Parser.cpp:3562 > + goto parseProperty;
Is it safe for `async hello: value` property?
> Source/JavaScriptCore/parser/Parser.cpp:3989 > + return parseAsyncFunctionExpression(context);
Looks nice.
> Source/JavaScriptCore/parser/Parser.h:1579 > + ASSERT(false);
Let's use `RELEASE_ASSERT_NOT_REACHED()`
> Source/JavaScriptCore/runtime/CodeCache.cpp:200 > + ConstructAbility constructAbility = constructAbilityForParseMode(metadata->parseMode());
Nice.
Caitlin Potter (:caitp)
Comment 81
2016-05-12 09:34:08 PDT
Created
attachment 278735
[details]
Patch Fixups
Caitlin Potter (:caitp)
Comment 82
2016-05-12 09:34:55 PDT
Comment on
attachment 278692
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=278692&action=review
Thanks --- I've removed the bits that snuck in by mistake.
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:509 >> + if (!haveParameterNamedArguments && !m_codeBlock->isArrowFunction() && !m_codeBlock->isAsyncArrowFunction()) { > > isAsyncArrowFunction is included in isArrowFunction().
Yeah, the last patchset was actually supposed to remove these, but it got undone by mistake. They're gone now
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4313 >> + ASSERT(m_codeBlock->isArrowFunction() || parseMode() == SourceParseMode::AsyncArrowFunctionBodyMode || parseMode() == SourceParseMode::AsyncArrowFunctionMode || m_codeBlock->isArrowFunctionContext() || constructorKind() == ConstructorKind::Derived || m_codeType == EvalCode); > > Is this `SourceParseMode::AsyncArrowFunctionBodyMode` correct?
I'm not sure how else to make this work --- there's probably a better way, but for the purposes of this patch, I think we can do it for the body function too. Ditto for the other one of these
Caitlin Potter (:caitp)
Comment 83
2016-05-18 06:33:16 PDT
Created
attachment 279242
[details]
Patch Keeping it unbitrotten~
Caitlin Potter (:caitp)
Comment 84
2016-05-21 19:03:04 PDT
Created
attachment 279546
[details]
Patch fix failures
Caitlin Potter (:caitp)
Comment 85
2016-05-25 15:46:45 PDT
Created
attachment 279829
[details]
Patch Please take another look when you get some time =)
Yusuke Suzuki
Comment 86
2016-05-25 20:01:53 PDT
(In reply to
comment #85
)
> Created
attachment 279829
[details]
> Patch > > Please take another look when you get some time =)
OK, I'll check your patch tonight!
Yusuke Suzuki
Comment 87
2016-05-26 09:10:44 PDT
Comment on
attachment 279829
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=279829&action=review
Looks good to me. Before landing, please ensure that JSBench (you can run it with run-jsc-benchmark) does not has regression, since JSBench is good for measuring the code loading (parser) performance.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:656 > + if (SourceParseMode::ArrowFunctionMode == parseMode || SourceParseMode::AsyncArrowFunctionBodyMode == parseMode || SourceParseMode::AsyncArrowFunctionMode == parseMode) {
Please add a comment about why AsyncArrowFunctionBodyMode is listed here. (comment about the system that async arrow function body looks up the arrow function's context).
> Source/JavaScriptCore/parser/ASTBuilder.h:386 > + usesArrowFunction();
Is this usesArrowFunction() here correct? These functions are not arrow functions, right? If the body is specially handled, please add a comment about how the body is handled.
> Source/JavaScriptCore/parser/Parser.cpp:3541 > + goto parseProperty;
Is it safe for `async hello: value` property? I think we need to chek !isAsyncMethod in match(COLON) phase to avoid it. And please add the test to ensure that JSC makes `async hello: value` case SyntaxError.
> Source/JavaScriptCore/parser/Parser.h:1579 > + ASSERT(false);
should be RELEASE_ASSERT_NOT_REACHED()
Caitlin Potter (:caitp)
Comment 88
2016-05-26 16:01:20 PDT
Comment on
attachment 279829
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=279829&action=review
I've run benchmarks (report dumped to
https://gist.github.com/caitp/26b24e076ace641ae7c8a9304da3d07c
) --- It doesn't look that good, even with the flag. I'll see if I can improve this a bit tomorrow.
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:656 >> + if (SourceParseMode::ArrowFunctionMode == parseMode || SourceParseMode::AsyncArrowFunctionBodyMode == parseMode || SourceParseMode::AsyncArrowFunctionMode == parseMode) { > > Please add a comment about why AsyncArrowFunctionBodyMode is listed here. > (comment about the system that async arrow function body looks up the arrow function's context).
Done, and added some new tests which go a bit deeper into this (and fixed some bugs related to this)
>> Source/JavaScriptCore/parser/ASTBuilder.h:386 >> + usesArrowFunction(); > > Is this usesArrowFunction() here correct? These functions are not arrow functions, right? > If the body is specially handled, please add a comment about how the body is handled.
It's not needed, removed.
>> Source/JavaScriptCore/parser/Parser.cpp:3541 >> + goto parseProperty; > > Is it safe for `async hello: value` property? > I think we need to chek !isAsyncMethod in match(COLON) phase to avoid it. > And please add the test to ensure that JSC makes `async hello: value` case SyntaxError.
You are right... Added a fix and tests or this
>> Source/JavaScriptCore/parser/Parser.h:1579 >> + ASSERT(false); > > should be RELEASE_ASSERT_NOT_REACHED()
Alright
Caitlin Potter (:caitp)
Comment 89
2016-05-26 16:05:48 PDT
Created
attachment 279921
[details]
Patch more tests, some fixes, and bad news re: benchmarks
Saam Barati
Comment 90
2016-05-26 16:08:49 PDT
(In reply to
comment #87
)
> Comment on
attachment 279829
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=279829&action=review
> > Looks good to me. > Before landing, please ensure that JSBench (you can run it with > run-jsc-benchmark) does not has regression, > since JSBench is good for measuring the code loading (parser) performance.
The Parser isn't actually very hot in JSBench because we have the code cache. Octane/code-load (jQuery + closure) are much better indicators of parser performance. Let's run those.
Saam Barati
Comment 91
2016-05-26 16:10:32 PDT
(In reply to
comment #88
)
> Comment on
attachment 279829
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=279829&action=review
> > I've run benchmarks (report dumped to >
https://gist.github.com/caitp/26b24e076ace641ae7c8a9304da3d07c
) --- It > doesn't look that good, even with the flag. > > I'll see if I can improve this a bit tomorrow.
There is a lot of noise in those results. It's best to run JSBench with a bunch of iterations because of how short running some of the tests are. Probably "--outer 50 --inner 10" or something like that.
Saam Barati
Comment 92
2016-05-26 16:12:37 PDT
(In reply to
comment #90
)
> (In reply to
comment #87
) > > Comment on
attachment 279829
[details]
> > Patch > > > > View in context: > >
https://bugs.webkit.org/attachment.cgi?id=279829&action=review
> > > > Looks good to me. > > Before landing, please ensure that JSBench (you can run it with > > run-jsc-benchmark) does not has regression, > > since JSBench is good for measuring the code loading (parser) performance. > The Parser isn't actually very hot in JSBench because we have > the code cache. Octane/code-load (jQuery + closure) are much better > indicators of parser performance. Let's run those.
I usually run this for code-load `run-jsc-benchmarks --outer 10 --octane --benchmark "closure|jquery" baseline:<pathToVMBaseline> changes:<pathToVMWithChanges>`
Yusuke Suzuki
Comment 93
2016-05-26 17:09:13 PDT
(In reply to
comment #90
)
> (In reply to
comment #87
) > > Comment on
attachment 279829
[details]
> > Patch > > > > View in context: > >
https://bugs.webkit.org/attachment.cgi?id=279829&action=review
> > > > Looks good to me. > > Before landing, please ensure that JSBench (you can run it with > > run-jsc-benchmark) does not has regression, > > since JSBench is good for measuring the code loading (parser) performance. > The Parser isn't actually very hot in JSBench because we have > the code cache. Octane/code-load (jQuery + closure) are much better > indicators of parser performance. Let's run those.
Oops, I was confused between code caching and code loading, thanks :)
Caitlin Potter (:caitp)
Comment 94
2016-05-26 17:42:37 PDT
(In reply to
comment #92
)
> (In reply to
comment #90
) > > (In reply to
comment #87
) > > > Comment on
attachment 279829
[details]
> > > Patch > > > > > > View in context: > > >
https://bugs.webkit.org/attachment.cgi?id=279829&action=review
> > > > > > Looks good to me. > > > Before landing, please ensure that JSBench (you can run it with > > > run-jsc-benchmark) does not has regression, > > > since JSBench is good for measuring the code loading (parser) performance. > > The Parser isn't actually very hot in JSBench because we have > > the code cache. Octane/code-load (jQuery + closure) are much better > > indicators of parser performance. Let's run those. > > I usually run this for code-load > `run-jsc-benchmarks --outer 10 --octane --benchmark "closure|jquery" > baseline:<pathToVMBaseline> changes:<pathToVMWithChanges>`
well, the results of a mix between
comment 91
and
comment 92
are this: Benchmark report for Octane on EchoBeach (MacBookPro11,5). VMs tested: "TipOfTree" at /Users/caitp/git/WebKit2/WebKitBuild/Release/DumpRenderTree "AsyncAwait" at /Users/caitp/git/WebKit/WebKitBuild/Release/DumpRenderTree Collected 500 samples per benchmark/VM, with 50 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. TipOfTree AsyncAwait closure 0.30106+-0.00079 ! 0.30431+-0.00081 ! definitely 1.0108x slower jquery 2.23843+-0.05744 ? 2.26908+-0.05960 ? might be 1.0137x slower <geometric> 0.81418+-0.01038 ? 0.82405+-0.01094 ? might be 1.0121x slower --- Which still isn't great, I guess? It probably gets worse once the runtime flag is removed, too. I'll talk to a colleague about profiling this to see if I can fix it a bit tomorrow/next week.
Caitlin Potter (:caitp)
Comment 95
2016-05-27 14:00:57 PDT
Adding more branch prediction hints to the collections of `m_runtimeFlags.isAsyncAwaitEnabled() && ...` conditions seems to get a big improvement (dramatically fixing regressions in Dromaeo, and significantly mitigating the codeload regressions to less than 1% on average). I think that's pretty good, but the branch prediction hints will not be useful forever, so more work may be needed to keep the parser performing well later. What do you think?
Saam Barati
Comment 96
2016-05-27 14:15:42 PDT
(In reply to
comment #95
)
> Adding more branch prediction hints to the collections of > `m_runtimeFlags.isAsyncAwaitEnabled() && ...` conditions seems to get a big > improvement (dramatically fixing regressions in Dromaeo, and significantly > mitigating the codeload regressions to less than 1% on average). > > I think that's pretty good, but the branch prediction hints will not be > useful forever, so more work may be needed to keep the parser performing > well later. > > What do you think?
Can you post the new numbers? I think this is probably OK for now, but it's worth thinking about and opening up a bug for making this fast once we remove the runtime flag. If we can do anything to help with that now, it's worth it.
Caitlin Potter (:caitp)
Comment 97
2016-05-27 14:48:19 PDT
The dromaeo results for the latest patch are about 3-4% regressions for each test. With the branch prediction adjustment, there's a 5-10% improvement (If there's a command to dump results of run-perf-tests as text, I'll provide all the numbers). With the command `run-jsc-benchmarks --outer 10 --octane --benchmark "closure|jquery" baseline:~/git/WebKit2/WebKitBuild/Release/DumpRenderTree asyncawait:~/git/WebKit/WebKitBuild/Release/DumpRenderTree`, results are: baseline asyncawait closure 0.30989+-0.00551 0.30340+-0.00295 might be 1.0214x faster jquery 3.65550+-0.01427 ! 3.78112+-0.03262 ! definitely 1.0344x slower <geometric> 1.06425+-0.01004 ? 1.07103+-0.00613 ? might be 1.0064x slower --- The jquery results might look a little worse than previously described, but the fact that Dromaeo consistently does so much better with the branch predictions added goes a long way in my opinion. Ensuring that `m_runtimeFlags` is packed efficiently and accessed in cache more often will probably see some improvement, but hard to guess how much
Yusuke Suzuki
Comment 98
2016-05-27 20:19:42 PDT
(In reply to
comment #97
)
> The dromaeo results for the latest patch are about 3-4% regressions for each > test. With the branch prediction adjustment, there's a 5-10% improvement (If > there's a command to dump results of run-perf-tests as text, I'll provide > all the numbers). > > With the command `run-jsc-benchmarks --outer 10 --octane --benchmark > "closure|jquery" baseline:~/git/WebKit2/WebKitBuild/Release/DumpRenderTree > asyncawait:~/git/WebKit/WebKitBuild/Release/DumpRenderTree`, results are: > > baseline asyncawait > > > closure 0.30989+-0.00551 0.30340+-0.00295 > might be 1.0214x faster > jquery 3.65550+-0.01427 ! 3.78112+-0.03262 ! > definitely 1.0344x slower > > <geometric> 1.06425+-0.01004 ? 1.07103+-0.00613 ? > might be 1.0064x slower > --- > > The jquery results might look a little worse than previously described, but > the fact that Dromaeo consistently does so much better with the branch > predictions added goes a long way in my opinion. > > Ensuring that `m_runtimeFlags` is packed efficiently and accessed in cache > more often will probably see some improvement, but hard to guess how much
Personally, I think it is ok if we can track this in the separated bug after this change. (and we need to look into it :))
Caitlin Potter (:caitp)
Comment 99
2016-05-27 21:43:27 PDT
Created
attachment 280020
[details]
(Old version) add patch with those branch prediction adjustments that seemed to mitigate dromaeo regressions
Yusuke Suzuki
Comment 100
2016-05-27 22:20:25 PDT
Comment on
attachment 280020
[details]
(Old version) r=me. Please open a new bug to track the parsing optimization :)
WebKit Commit Bot
Comment 101
2016-05-27 22:42:59 PDT
Comment on
attachment 280020
[details]
(Old version) Clearing flags on attachment: 280020 Committed
r201481
: <
http://trac.webkit.org/changeset/201481
>
WebKit Commit Bot
Comment 102
2016-05-27 22:43:09 PDT
All reviewed patches have been landed. Closing bug.
Saam Barati
Comment 103
2016-05-27 23:28:52 PDT
(In reply to
comment #97
)
> The dromaeo results for the latest patch are about 3-4% regressions for each > test. With the branch prediction adjustment, there's a 5-10% improvement (If > there's a command to dump results of run-perf-tests as text, I'll provide > all the numbers). > > With the command `run-jsc-benchmarks --outer 10 --octane --benchmark > "closure|jquery" baseline:~/git/WebKit2/WebKitBuild/Release/DumpRenderTree > asyncawait:~/git/WebKit/WebKitBuild/Release/DumpRenderTree`, results are: > > baseline asyncawait > > > closure 0.30989+-0.00551 0.30340+-0.00295 > might be 1.0214x faster > jquery 3.65550+-0.01427 ! 3.78112+-0.03262 ! > definitely 1.0344x slower > > <geometric> 1.06425+-0.01004 ? 1.07103+-0.00613 ? > might be 1.0064x slower > --- > > The jquery results might look a little worse than previously described, but > the fact that Dromaeo consistently does so much better with the branch > predictions added goes a long way in my opinion. > > Ensuring that `m_runtimeFlags` is packed efficiently and accessed in cache > more often will probably see some improvement, but hard to guess how much
Are these the performance results of the patch that was just landed? Or is this the performance if we actually have the feature enabled?
Caitlin Potter (:caitp)
Comment 104
2016-05-28 06:14:58 PDT
(In reply to
comment #103
)
> (In reply to
comment #97
) > > The dromaeo results for the latest patch are about 3-4% regressions for each > > test. With the branch prediction adjustment, there's a 5-10% improvement (If > > there's a command to dump results of run-perf-tests as text, I'll provide > > all the numbers). > > > > With the command `run-jsc-benchmarks --outer 10 --octane --benchmark > > "closure|jquery" baseline:~/git/WebKit2/WebKitBuild/Release/DumpRenderTree > > asyncawait:~/git/WebKit/WebKitBuild/Release/DumpRenderTree`, results are: > > > > baseline asyncawait > > > > > > closure 0.30989+-0.00551 0.30340+-0.00295 > > might be 1.0214x faster > > jquery 3.65550+-0.01427 ! 3.78112+-0.03262 ! > > definitely 1.0344x slower > > > > <geometric> 1.06425+-0.01004 ? 1.07103+-0.00613 ? > > might be 1.0064x slower > > --- > > > > The jquery results might look a little worse than previously described, but > > the fact that Dromaeo consistently does so much better with the branch > > predictions added goes a long way in my opinion. > > > > Ensuring that `m_runtimeFlags` is packed efficiently and accessed in cache > > more often will probably see some improvement, but hard to guess how much > Are these the performance results of the patch that was just landed? > Or is this the performance if we actually have the feature enabled?
Well, that's a good question. I assume DumpRenderTree doesn't boot up with all RuntimeFlags enabled (the way the way the jsc shell does). If it does, then that would incorporate the results of the feature if it were shipped
Saam Barati
Comment 105
2016-05-28 09:26:25 PDT
Comment on
attachment 280020
[details]
(Old version) View in context:
https://bugs.webkit.org/attachment.cgi?id=280020&action=review
I don't think we should land patches where we have these known regressions. Can we try to get rid of the regressions?
> Source/JavaScriptCore/parser/Parser.cpp:624 > + if (UNLIKELY(m_runtimeFlags.isAsyncAwaitEnabled() && matchContextualKeyword(m_vm->propertyNames->async))) {
There is a chance that this may be faster if the LHS and RHS were swapped because it might aid the register allocator. Same with the similar compare elsewhere. Is this what's slowing down the parser? Have you tried making ASYNC a key word? That may be faster than checking the IDENT's StringImpl
Saam Barati
Comment 106
2016-05-28 09:28:26 PDT
We can also try making the RuntimeFlags thingy use a byte for every flag instead of a bit so that it doesn't have to do bitmath. That may help that hot branch.
Caitlin Potter (:caitp)
Comment 107
2016-05-28 09:44:52 PDT
something like ``` // private members of class Parser: #define JSC_DECLARE_RUNTIME_FLAG_BOOL(name, isEnabledFlag) bool m_is##name; JSC_RUNTIME_FLAG(JSC_DECLARE_RUNTIME_FLAG_BOOLEAN) // Parser constructor: #define JSC_INITIALIZE_PARSER_RUNTIME_FLAGS(name, isEnabledFlag) \ m_is##name = runtimeFlags.is##name(); JSC_RUNTIME_FLAG_(JSC_INITIALIZE_PARSER_RUNTIME_FLAGS) ``` and replace `m_runtimeFlags` accesses with more direct things? I wouldn't want to make the RuntimeFlags class too fat, since it gets copied around a lot, but maybe only doing the heavy copying in the Parser constructor is okay. I'll try it and see how it goes, I guess
Saam Barati
Comment 108
2016-05-28 10:20:40 PDT
(In reply to
comment #107
)
> something like > > ``` > // private members of class Parser: > #define JSC_DECLARE_RUNTIME_FLAG_BOOL(name, isEnabledFlag) bool m_is##name; > JSC_RUNTIME_FLAG(JSC_DECLARE_RUNTIME_FLAG_BOOLEAN) > > // Parser constructor: > #define JSC_INITIALIZE_PARSER_RUNTIME_FLAGS(name, isEnabledFlag) \ > m_is##name = runtimeFlags.is##name(); > JSC_RUNTIME_FLAG_(JSC_INITIALIZE_PARSER_RUNTIME_FLAGS) > ``` > > and replace `m_runtimeFlags` accesses with more direct things? I wouldn't > want to make the RuntimeFlags class too fat, since it gets copied around a > lot, but maybe only doing the heavy copying in the Parser constructor is > okay. > > I'll try it and see how it goes, I guess
You can also add a field into Parser that copies out the flag at Parser construction time. Have you tried making async a key word?
Yusuke Suzuki
Comment 109
2016-05-28 10:29:47 PDT
Comment on
attachment 280020
[details]
(Old version) View in context:
https://bugs.webkit.org/attachment.cgi?id=280020&action=review
>> Source/JavaScriptCore/parser/Parser.cpp:624 >> + if (UNLIKELY(m_runtimeFlags.isAsyncAwaitEnabled() && matchContextualKeyword(m_vm->propertyNames->async))) { > > There is a chance that this may be faster if the LHS and RHS were swapped because it might aid the register allocator. > Same with the similar compare elsewhere. > Is this what's slowing down the parser? > Have you tried making ASYNC a key word? That may be faster than checking the IDENT's StringImpl
When executing `matchContextualKeyword(m_vm->propertyNames->async)`, it becomes `Identifier == Identifier` check and it should be pointer comparison because Identifier is already interned.
Saam Barati
Comment 110
2016-05-28 10:47:31 PDT
(In reply to
comment #109
)
> Comment on
attachment 280020
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=280020&action=review
> > >> Source/JavaScriptCore/parser/Parser.cpp:624 > >> + if (UNLIKELY(m_runtimeFlags.isAsyncAwaitEnabled() && matchContextualKeyword(m_vm->propertyNames->async))) { > > > > There is a chance that this may be faster if the LHS and RHS were swapped because it might aid the register allocator. > > Same with the similar compare elsewhere. > > Is this what's slowing down the parser? > > Have you tried making ASYNC a key word? That may be faster than checking the IDENT's StringImpl > > When executing `matchContextualKeyword(m_vm->propertyNames->async)`, it > becomes `Identifier == Identifier` check and it should be pointer comparison > because Identifier is already interned.
I was just thinking that we may get almost identical codegen (or we would get slightly worse codegen but the machine's branch/indirect jump prediction would make up for it) for the 'switch' statement if we made ASYNC a keyword, and we wouldn't have this branch at each IDENT. But maybe this would cause slow downs elsewhere. I'm not sure. It's hard to predict without implementing it and measuring perf.
Saam Barati
Comment 111
2016-05-28 10:48:38 PDT
(In reply to
comment #110
)
> (In reply to
comment #109
) > > Comment on
attachment 280020
[details]
> > Patch > > > > View in context: > >
https://bugs.webkit.org/attachment.cgi?id=280020&action=review
> > > > >> Source/JavaScriptCore/parser/Parser.cpp:624 > > >> + if (UNLIKELY(m_runtimeFlags.isAsyncAwaitEnabled() && matchContextualKeyword(m_vm->propertyNames->async))) { > > > > > > There is a chance that this may be faster if the LHS and RHS were swapped because it might aid the register allocator. > > > Same with the similar compare elsewhere. > > > Is this what's slowing down the parser? > > > Have you tried making ASYNC a key word? That may be faster than checking the IDENT's StringImpl > > > > When executing `matchContextualKeyword(m_vm->propertyNames->async)`, it > > becomes `Identifier == Identifier` check and it should be pointer comparison > > because Identifier is already interned. > > I was just thinking that we may get almost identical codegen (or we would > get slightly worse codegen but the machine's branch/indirect jump prediction > would make up for it) for the 'switch' statement if we made ASYNC a keyword, > and we wouldn't have this branch at each IDENT. But maybe this would cause > slow downs elsewhere. I'm not sure. It's hard to predict without > implementing it and measuring perf.
Actually, a good starting point is to see if these branches are actually what the remaining slow down is. Because octane/code-load doesn't use async/await, we can probably get away with removing the branch and seeing if the entire regression goes away. If it doesn't, we know there is slow down elsewhere.
Yusuke Suzuki
Comment 112
2016-05-28 10:59:03 PDT
Comment on
attachment 280020
[details]
(Old version) View in context:
https://bugs.webkit.org/attachment.cgi?id=280020&action=review
>>>>> Source/JavaScriptCore/parser/Parser.cpp:624 >>>>> + if (UNLIKELY(m_runtimeFlags.isAsyncAwaitEnabled() && matchContextualKeyword(m_vm->propertyNames->async))) { >>>> >>>> There is a chance that this may be faster if the LHS and RHS were swapped because it might aid the register allocator. >>>> Same with the similar compare elsewhere. >>>> Is this what's slowing down the parser? >>>> Have you tried making ASYNC a key word? That may be faster than checking the IDENT's StringImpl >>> >>> When executing `matchContextualKeyword(m_vm->propertyNames->async)`, it becomes `Identifier == Identifier` check and it should be pointer comparison because Identifier is already interned. >> >> I was just thinking that we may get almost identical codegen (or we would get slightly worse codegen but the machine's branch/indirect jump prediction would make up for it) for the 'switch' statement if we made ASYNC a keyword, and we wouldn't have this branch at each IDENT. But maybe this would cause slow downs elsewhere. I'm not sure. It's hard to predict without implementing it and measuring perf. > > Actually, a good starting point is to see if these branches are actually what the remaining slow down is. Because octane/code-load doesn't use async/await, we can probably get away with removing the branch and seeing if the entire regression goes away. If it doesn't, we know there is slow down elsewhere.
Sounds reasonable.
Yusuke Suzuki
Comment 113
2016-05-28 11:28:06 PDT
The performance of the Parser.h / Parser.cpp reverted version is the following. It gives us super nice insight that the cause of the code-load regression should reside in Parser.cpp's change. Generating benchmark report at /home/yusukesuzuki/dev/WebKit/baseline_patched_Octane_hanayamata_20160529_0328_report.txt And raw data at /home/yusukesuzuki/dev/WebKit/baseline_patched_Octane_hanayamata_20160529_0328.json Benchmark report for Octane on hanayamata. VMs tested: "baseline" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/old/Release/bin/jsc "patched" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/async/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.62566+-0.00522 0.62292+-0.00696 jquery 8.03686+-0.02728 7.96035+-0.06515 <geometric> 2.24234+-0.00828 2.22676+-0.01878 might be 1.0070x faster
Caitlin Potter (:caitp)
Comment 114
2016-05-28 18:07:19 PDT
I tried a version with 2 changes: `m_runtimeFlags` replaced with a single bool member, and flipping the order of operations (check if identifier === "async" before testing the flag), the results are not promising: ``` Benchmark report for Octane on EchoBeach (MacBookPro11,5). VMs tested: "BeforePatch" at /Users/caitp/git/WebKit/OldBuild/jsc "AfterPatch" at /Users/caitp/git/WebKit/WebKitBuild/Release/jsc Collected 100 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. BeforePatch AfterPatch closure 0.73573+-0.02948 ? 0.73754+-0.02989 ? jquery 6.68742+-0.05906 ? 6.81852+-0.07467 ? might be 1.0196x slower <geometric> 2.20728+-0.04715 ? 2.23115+-0.04883 ? might be 1.0108x slower ``` --- Later in the day, I also tried a version which tokenizes "async" as a keyword, which also does not seem to do any better. ``` Benchmark report for Octane on EchoBeach (MacBookPro11,5). VMs tested: "BeforePatch" at /Users/caitp/git/WebKit/OldBuild/jsc "AfterPatch" at /Users/caitp/git/WebKit/WebKitBuild/Release/jsc Collected 100 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. BeforePatch AfterPatch closure 0.73735+-0.02782 ? 0.75599+-0.03035 ? might be 1.0253x slower jquery 6.95187+-0.05463 ? 7.07144+-0.07175 ? might be 1.0172x slower <geometric> 2.25410+-0.04529 ? 2.30019+-0.04900 ? might be 1.0204x slower ``` And another one with fewer VM runs, ``` Benchmark report for Octane on EchoBeach (MacBookPro11,5). VMs tested: "BeforePatch" at /Users/caitp/git/WebKit/OldBuild/jsc "AfterPatch" at /Users/caitp/git/WebKit/WebKitBuild/Release/jsc Collected 40 samples per benchmark/VM, with 4 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. BeforePatch AfterPatch closure 0.73733+-0.04519 0.73413+-0.04667 jquery 6.86110+-0.07219 ? 6.87968+-0.06205 ? <geometric> 2.23916+-0.07172 2.23714+-0.07551 might be 1.0009x faster ``` which does not really indicate any serious improvements. One thing we could do in the short term is add a compile-time flag to the async/await stuff (since the runtime flag stuff is kind of not really configurable in a the browser anyway yet, there isn't much difference between them anyway). I'll do more real profiling on monday to see what the biggest problems are
Yusuke Suzuki
Comment 115
2016-05-29 08:08:35 PDT
I think I've just found several fixes. Still investigating...
Yusuke Suzuki
Comment 116
2016-05-30 07:00:32 PDT
Reopening to attach new patch.
Yusuke Suzuki
Comment 117
2016-05-30 07:00:36 PDT
Created
attachment 280096
[details]
Patch WIP: still slower than non-landing version. But faster than ToT
Yusuke Suzuki
Comment 118
2016-05-30 07:02:31 PDT
Current situation. WIP patch is improving the performance. Generating benchmark report at /home/yusukesuzuki/dev/WebKit/baseline_patched_Octane_hanayamata_20160530_2302_report.txt And raw data at /home/yusukesuzuki/dev/WebKit/baseline_patched_Octane_hanayamata_20160530_2302.json Benchmark report for Octane on hanayamata. VMs tested: "baseline" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/async-master/Release/bin/jsc "patched" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/async-target/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.65360+-0.00478 ^ 0.62956+-0.00296 ^ definitely 1.0382x faster jquery 8.26063+-0.05769 ^ 8.05663+-0.03835 ^ definitely 1.0253x faster <geometric> 2.32356+-0.01200 ^ 2.25213+-0.00808 ^ definitely 1.0317x faster
Yusuke Suzuki
Comment 119
2016-05-30 07:26:19 PDT
WIP patch against the reverted version. Still slower, but the regression becomes much smaller (1.1%). Generating benchmark report at /home/yusukesuzuki/dev/WebKit/baseline_patched_Octane_hanayamata_20160530_2326_report.txt And raw data at /home/yusukesuzuki/dev/WebKit/baseline_patched_Octane_hanayamata_20160530_2326.json Benchmark report for Octane on hanayamata. VMs tested: "baseline" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/async-reverted/Release/bin/jsc "patched" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/async-target/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.62246+-0.00884 ? 0.63213+-0.01093 ? might be 1.0155x slower jquery 8.00849+-0.02983 ? 8.06323+-0.03352 ? <geometric> 2.23262+-0.01761 ? 2.25746+-0.01750 ? might be 1.0111x slower
WebKit Commit Bot
Comment 120
2016-05-30 07:27:58 PDT
Attachment 280096
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/parser/Parser.cpp:2629: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/parser/ParserModes.h:68: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/parser/ParserModes.h:69: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/parser/ParserModes.h:70: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/parser/ParserModes.h:71: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/parser/ParserModes.h:72: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/parser/ParserModes.h:73: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/parser/ParserModes.h:74: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/parser/ParserModes.h:75: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/parser/ParserModes.h:76: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/parser/ParserModes.h:77: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/parser/ParserModes.h:78: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/parser/ParserModes.h:85: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/parser/ParserModes.h:86: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/parser/ParserModes.h:87: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/parser/ParserModes.h:88: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/parser/ParserModes.h:95: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/parser/ParserModes.h:102: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/parser/ParserModes.h:103: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/parser/ParserModes.h:110: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/parser/ParserModes.h:117: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 21 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Yusuke Suzuki
Comment 121
2016-05-30 11:15:52 PDT
Created
attachment 280104
[details]
Patch
Yusuke Suzuki
Comment 122
2016-05-30 11:17:28 PDT
Comment on
attachment 280104
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=280104&action=review
Added comments.
> Source/JavaScriptCore/parser/Parser.cpp:620 > + // FIXME: These branch causes 1% regression in octane code-load jquery.
Insert
https://bugs.webkit.org/show_bug.cgi?id=158211
link here when landing.
> Source/JavaScriptCore/parser/Parser.cpp:1738 > + // FIXME: These branch causes 1% regression in octane code-load jquery.
Ditto.
> Source/JavaScriptCore/parser/Parser.cpp:2626 > + // FIXME: These branch causes 1% regression in octane code-load jquery.
Ditto.
> Source/JavaScriptCore/parser/Parser.cpp:3254 > + // FIXME: These branch causes 1% regression in octane code-load jquery.
Ditto.
> Source/JavaScriptCore/parser/Parser.cpp:3983 > + // FIXME: These branch causes 1% regression in octane code-load jquery.
Ditto.
> Source/JavaScriptCore/parser/Parser.cpp:4182 > + // FIXME: These branch causes 1% regression in octane code-load jquery.
Ditto.
Build Bot
Comment 123
2016-05-30 12:02:23 PDT
Comment on
attachment 280104
[details]
Patch
Attachment 280104
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.webkit.org/results/1408361
Number of test failures exceeded the failure limit.
Build Bot
Comment 124
2016-05-30 12:02:29 PDT
Created
attachment 280106
[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
Caitlin Potter (:caitp)
Comment 125
2016-05-30 12:54:09 PDT
Comment on
attachment 280104
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=280104&action=review
> Source/JavaScriptCore/parser/Parser.cpp:628 > + result = parseAsyncFunctionDeclaration(context);
A possible speedup here could be adding a lookahead token to the lexer, rather than restoring a save point. That way, you don't re-lex the "async" token. Another option: a more light-weight savepoint for single token lookaheads, which saves the original token state and somehow makes it unnecessary to re-lex. I have an idea how to make this work, if you want to give it a try.
Caitlin Potter (:caitp)
Comment 126
2016-05-30 14:41:06 PDT
(In reply to
comment #125
)
> Comment on
attachment 280104
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=280104&action=review
> > > Source/JavaScriptCore/parser/Parser.cpp:628 > > + result = parseAsyncFunctionDeclaration(context); > > A possible speedup here could be adding a lookahead token to the lexer, > rather than restoring a save point. That way, you don't re-lex the "async" > token. > > Another option: a more light-weight savepoint for single token lookaheads, > which saves the original token state and somehow makes it unnecessary to > re-lex. I have an idea how to make this work, if you want to give it a try.
I gave this second thing a try on top of your patch, ``` Benchmark report for Octane on EchoBeach (MacBookPro11,5). VMs tested: "baseline" at /Users/caitp/git/WebKit/WebKitBuild/Baseline/jsc "current" at /Users/caitp/git/WebKit/WebKitBuild/Release/jsc Collected 100 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 current closure 0.71648+-0.02952 0.71619+-0.02943 jquery 6.44806+-0.04474 6.41269+-0.03904 <geometric> 2.13811+-0.04612 2.13258+-0.04680 might be 1.0026x faster ``` It probably isn't worth, dunno.
Yusuke Suzuki
Comment 127
2016-05-31 03:01:52 PDT
Comment on
attachment 280104
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=280104&action=review
>>> Source/JavaScriptCore/parser/Parser.cpp:628 >>> + result = parseAsyncFunctionDeclaration(context); >> >> A possible speedup here could be adding a lookahead token to the lexer, rather than restoring a save point. That way, you don't re-lex the "async" token. >> >> Another option: a more light-weight savepoint for single token lookaheads, which saves the original token state and somehow makes it unnecessary to re-lex. I have an idea how to make this work, if you want to give it a try. > > I gave this second thing a try on top of your patch, > > ``` > Benchmark report for Octane on EchoBeach (MacBookPro11,5). > > VMs tested: > "baseline" at /Users/caitp/git/WebKit/WebKitBuild/Baseline/jsc > "current" at /Users/caitp/git/WebKit/WebKitBuild/Release/jsc > > Collected 100 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 current > > closure 0.71648+-0.02952 0.71619+-0.02943 > jquery 6.44806+-0.04474 6.41269+-0.03904 > > <geometric> 2.13811+-0.04612 2.13258+-0.04680 might be 1.0026x faster > ``` > > It probably isn't worth, dunno.
Nice. But I think it does not affect on the current code-load performance while this is good for "async function" performance. So if we do that, I think we need to add a new test that measures the performance of parsing async functions (in LayoutTests/js/regress). I think we can handle the further performance improvements in
https://bugs.webkit.org/show_bug.cgi?id=158211
:D
Yusuke Suzuki
Comment 128
2016-05-31 03:28:53 PDT
Created
attachment 280133
[details]
Patch Inserted links to FIXMEs
Yusuke Suzuki
Comment 129
2016-05-31 04:04:01 PDT
Created
attachment 280137
[details]
Patch Fix build error on OS X debug bot
Saam Barati
Comment 130
2016-05-31 09:29:47 PDT
Comment on
attachment 280137
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=280137&action=review
Patch LGTM, but it deserves its own bug. Can you create a bug for it and upload patch there?
> Source/JavaScriptCore/parser/Parser.cpp:259 > + if (oneOfSourceParseModes<SourceParseMode::GeneratorBodyMode>(parseMode) || isAsyncFunctionBodyParseMode(parseMode))
I don't think we need to use this for single SourceParseModes
> Source/JavaScriptCore/parser/Parser.cpp:264 > + if (oneOfSourceParseModes<SourceParseMode::ArrowFunctionMode, SourceParseMode::AsyncArrowFunctionMode>(parseMode) && !hasError()) {
I think it's more readable to have the == checks directly. Since moving to the bit representation, have you looked at the code LLVM generates for this? I suspect it does smart things and does some bit math for you. If it doesn't, then it's worth keeping this function. I don't really like the name though, but I'm not sure what a better name is. I'll think.
> Source/JavaScriptCore/parser/Parser.cpp:618 > + // FIXME: These branch causes 1% regression in octane code-load.
For this FIXME, and all the others with the same message, I would make the comment: "This branch contributes to a 1% octane code-load regression"
> Source/JavaScriptCore/parser/ParserModes.h:75 > +static ALWAYS_INLINE bool oneOfSourceParseModes(SourceParseMode mode)
I think at least let's call this isOneOfSourceParseModes
Caitlin Potter (:caitp)
Comment 131
2016-05-31 09:47:31 PDT
Comment on
attachment 280137
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=280137&action=review
> Source/JavaScriptCore/parser/ParserModes.h:99 > + return oneOfSourceParseModes<
The reason I wrote these as switch statements without a default branch, is to minimize refactoring hazard. If we go with a change like this, it's necessary to remember to update each function (no compiler warning if one is missed). Of course, the perf win takes precedence, but it's worth thinking about.
Yusuke Suzuki
Comment 132
2016-05-31 11:19:36 PDT
Comment on
attachment 280137
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=280137&action=review
Thanks for your comments! I'll open a new bug and attach the revised patch :)
>> Source/JavaScriptCore/parser/Parser.cpp:259 >> + if (oneOfSourceParseModes<SourceParseMode::GeneratorBodyMode>(parseMode) || isAsyncFunctionBodyParseMode(parseMode)) > > I don't think we need to use this for single SourceParseModes
It leads better code generation than the old one. oneOfSourceParseMode will be compiled into test. And isAsyncFunctionBodyParseMode is also compiled into test. Since we can merge test || test onto the same one, these 2 functions are merged into one test in inlined compiled code. But == || test case, we (C++ compiler) cannot merge this. That's why we use oneOfSourceParseModes every time. So not to miss this chance accidentally in the super hot path, in Parser.cpp, I always use oneOfSourceParseModes<>() for merged conditions case (cond || cond). OLD: parseMode == SourceParseMode::GeneratorBodyMode || isAsyncFunctionBodyParseMode(parseMode) code in this patch. (Making bit flags are already applied) 977090: 66 83 f9 02 cmp $0x2,%cx (GeneratorBodyMode = 0x2) 977094: 48 c7 44 24 60 00 00 movq $0x0,0x60(%rsp) 97709b: 00 00 97709d: 48 c7 44 24 68 00 00 movq $0x0,0x68(%rsp) 9770a4: 00 00 9770a6: 48 c7 44 24 70 00 00 movq $0x0,0x70(%rsp) 9770ad: 00 00 9770af: 48 c7 44 24 78 00 00 movq $0x0,0x78(%rsp) 9770b6: 00 00 9770b8: 48 c7 84 24 80 00 00 movq $0x0,0x80(%rsp) 9770bf: 00 00 00 00 00 9770c4: 0f 84 28 06 00 00 je 9776f2 <JSC::Parser<JSC::Lexer<unsigned char> >::parseInner(JSC::Identifier const&, JSC::SourceParseMode)+0xa52> 9770ca: f7 c1 80 01 00 00 test $0x180,%ecx (This 0x180 is isAsyncFunctionBodyParseMode's merged bit flags) 9770d0: 0f 85 1c 06 00 00 jne 9776f2 <JSC::Parser<JSC::Lexer<unsigned char> >::parseInner(JSC::Identifier const&, JSC::SourceParseMode)+0xa52> cmp & jmp + test & jmp NEW: oneOfSourceParseModes<SourceParseMode::GeneratorBodyMode>(parseMode) || isAsyncFunctionBodyParseMode(parseMode) code in this patch. 977080: f7 c1 82 01 00 00 test $0x182,%ecx (GeneratorBodyMode flag is also merged!!!) 977086: 48 c7 44 24 60 00 00 movq $0x0,0x60(%rsp) 97708d: 00 00 97708f: 48 c7 44 24 68 00 00 movq $0x0,0x68(%rsp) 977096: 00 00 977098: 48 c7 44 24 70 00 00 movq $0x0,0x70(%rsp) 97709f: 00 00 9770a1: 48 c7 44 24 78 00 00 movq $0x0,0x78(%rsp) 9770a8: 00 00 9770aa: 48 c7 84 24 80 00 00 movq $0x0,0x80(%rsp) 9770b1: 00 00 00 00 00 9770b6: 0f 85 68 06 00 00 jne 977724 <JSC::Parser<JSC::Lexer<unsigned char> >::parseInner(JSC::Identifier const&, JSC::SourceParseMode)+0xa94> test & jmp!
>> Source/JavaScriptCore/parser/Parser.cpp:264 >> + if (oneOfSourceParseModes<SourceParseMode::ArrowFunctionMode, SourceParseMode::AsyncArrowFunctionMode>(parseMode) && !hasError()) { > > I think it's more readable to have the == checks directly. Since moving to the bit representation, have you looked at the code LLVM generates for this? I suspect it does smart things and does some bit math for you. > > If it doesn't, then it's worth keeping this function. I don't really like the name though, but I'm not sure what a better name is. I'll think.
For GCC, I made sure that this oneOfSourceParseModes is compiled into test & jmp, that is expected one. I'll check it in clang later. For example, let's pick the parseFunctionParameters. In the early stage of the function, we perform, if (UNLIKELY((oneOfSourceParseModes<SourceParseMode::ArrowFunctionMode, SourceParseMode::AsyncArrowFunctionMode>(mode)))) { And here is the code dumped by objdump. In this code, rdi:(Parser's this), rsi:(SyntaxChecker&), rdx:(SourceParseMode) 00000000009598f0 <JSC::SyntaxChecker::FormalParameterList JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionParameters<JSC::SyntaxChecker, JSC::ParserFunctionInfo<JSC::ASTBuilder> >(JSC::SyntaxChecker&, JSC::SourceParseMode, JSC::ParserFunctionInfo<JSC::ASTBuild er>&)>: 9598f0: 41 57 push %r15 9598f2: 41 56 push %r14 9598f4: 41 55 push %r13 9598f6: 41 54 push %r12 9598f8: 55 push %rbp 9598f9: 53 push %rbx 9598fa: 48 83 ec 68 sub $0x68,%rsp 9598fe: 64 48 8b 04 25 28 00 mov %fs:0x28,%rax 959905: 00 00 959907: 48 89 44 24 58 mov %rax,0x58(%rsp) 95990c: 31 c0 xor %eax,%eax 95990e: f6 c6 70 test $0x70,%dh 959911: 0f 85 49 01 00 00 jne 959a60 <JSC::SyntaxChecker::FormalParameterList JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionParameters<JSC::SyntaxChecker, JSC::ParserFunctionInfo<JSC::ASTBuilder> >(JSC::SyntaxChecker&, JSC::SourceParseMode, JSC::ParserFunctionInfo<JSC::ASTBuilder>&)+0x170> 959917: f7 c2 40 08 00 00 test $0x840,%edx 95991d: 8b 9f f4 20 00 00 mov 0x20f4(%rdi),%ebx 959923: 49 89 ff mov %rdi,%r15 959926: 49 89 f4 mov %rsi,%r12 959929: 41 89 d6 mov %edx,%r14d 95992c: 48 89 cd mov %rcx,%rbp 95992f: c7 87 f4 20 00 00 00 movl $0x0,0x20f4(%rdi) 959936: 00 00 00 959939: 8b 87 20 21 00 00 mov 0x2120(%rdi),%eax 95993f: 0f 85 43 06 00 00 jne 959f88 <JSC::SyntaxChecker::FormalParameterList JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionParameters<JSC::SyntaxChecker, JSC::ParserFunctionInfo<JSC::ASTBuilder> >(JSC::SyntaxChecker&, JSC::SourceParseMode, JSC::ParserFunctionInfo<JSC::ASTBuilder>&)+0x698> ... 959a60: e8 fb 14 8f ff callq 24af60 <WTFCrash@plt> And we can find the great test & jmp in // if (UNLIKELY((oneOfSourceParseModes<SourceParseMode::ArrowFunctionMode, SourceParseMode::AsyncArrowFunctionMode>(mode)))) { 959917: f7 c2 40 08 00 00 test $0x840,%edx ... 95993f: 0f 85 43 06 00 00 jne 959f88 <JSC::SyntaxChecker::FormalParameterList JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionParameters<JSC::SyntaxChecker, JSC::ParserFunctionInfo<JSC::ASTBuilder> >(JSC::SyntaxChecker&, JSC::SourceParseMode, JSC::ParserFunctionInfo<JSC::ASTBuilder>&)+0x698> This 0x840 => 0000100001000000 => ArrowFunctionMode | AsyncArrowFunctionMode. Compared to these efficient test & jmp (They are macro-fused in x86 processor (reg imm test & jmp)), the old implementation becomes following. 00000000009583e0 <JSC::SyntaxChecker::FormalParameterList JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionParameters<JSC::SyntaxChecker, JSC::ParserFunctionInfo<JSC::SyntaxChecker> >(JSC::SyntaxChecker&, JSC::SourceParseMode, JSC::ParserFunctionInfo<JSC::SyntaxChecker>&)>: 9583e0: 41 57 push %r15 9583e2: 41 56 push %r14 9583e4: 41 55 push %r13 9583e6: 41 54 push %r12 9583e8: 55 push %rbp 9583e9: 53 push %rbx 9583ea: 48 83 ec 68 sub $0x68,%rsp 9583ee: 64 48 8b 04 25 28 00 mov %fs:0x28,%rax 9583f5: 00 00 9583f7: 48 89 44 24 58 mov %rax,0x58(%rsp) 9583fc: 31 c0 xor %eax,%eax 9583fe: 8d 42 f4 lea -0xc(%rdx),%eax 958401: 3c 02 cmp $0x2,%al 958403: 0f 86 c7 01 00 00 jbe 9585d0 <JSC::SyntaxChecker::FormalParameterList JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionParameters<JSC::SyntaxChecker, JSC::ParserFunctionInfo<JSC::SyntaxChecker> >(JSC::SyntaxChecker&, JSC::SourceParseMode, JSC::ParserFunctionInfo<JSC::SyntaxChecker>&)+0x1f0> 958409: 80 fa 06 cmp $0x6,%dl 95840c: 8b 9f f4 20 00 00 mov 0x20f4(%rdi),%ebx 958412: 49 89 ff mov %rdi,%r15 958415: 49 89 f4 mov %rsi,%r12 958418: 41 89 d6 mov %edx,%r14d 95841b: 48 89 cd mov %rcx,%rbp 95841e: c7 87 f4 20 00 00 00 movl $0x0,0x20f4(%rdi) 958425: 00 00 00 958428: 0f 84 d2 00 00 00 je 958500 <JSC::SyntaxChecker::FormalParameterList JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionParameters<JSC::SyntaxChecker, JSC::ParserFunctionInfo<JSC::SyntaxChecker> >(JSC::SyntaxChecker&, JSC::SourceParseMode, JSC::ParserFunctionInfo<JSC::SyntaxChecker>&)+0x120> 95842e: 80 fa 0b cmp $0xb,%dl 958431: 0f 84 c9 00 00 00 je 958500 <JSC::SyntaxChecker::FormalParameterList JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionParameters<JSC::SyntaxChecker, JSC::ParserFunctionInfo<JSC::SyntaxChecker> >(JSC::SyntaxChecker&, JSC::SourceParseMode, JSC::ParserFunctionInfo<JSC::SyntaxChecker>&)+0x120> The same phase is compiled in, 958409: 80 fa 06 cmp $0x6,%dl ... 958428: 0f 84 d2 00 00 00 je 958500 <JSC::SyntaxChecker::FormalParameterList JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionParameters<JSC::SyntaxChecker, JSC::ParserFunctionInfo<JSC::SyntaxChecker> >(JSC::SyntaxChecker&, JSC::SourceParseMode, JSC::ParserFunctionInfo<JSC::SyntaxChecker>&)+0x120> 95842e: 80 fa 0b cmp $0xb,%dl 958431: 0f 84 c9 00 00 00 je 958500 <JSC::SyntaxChecker::FormalParameterList JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionParameters<JSC::SyntaxChecker, JSC::ParserFunctionInfo<JSC::SyntaxChecker> >(JSC::SyntaxChecker&, JSC::SourceParseMode, JSC::ParserFunctionInfo<JSC::SyntaxChecker>&)+0x120> (cmp & jmp) x2.
>> Source/JavaScriptCore/parser/Parser.cpp:618 >> + // FIXME: These branch causes 1% regression in octane code-load. > > For this FIXME, and all the others with the same message, I would make the comment: > "This branch contributes to a 1% octane code-load regression"
Sounds nice. Fixed.
>> Source/JavaScriptCore/parser/ParserModes.h:75 >> +static ALWAYS_INLINE bool oneOfSourceParseModes(SourceParseMode mode) > > I think at least let's call this > isOneOfSourceParseModes
Sounds nice. Changed.
>> Source/JavaScriptCore/parser/ParserModes.h:99 >> + return oneOfSourceParseModes< > > The reason I wrote these as switch statements without a default branch, is to minimize refactoring hazard. If we go with a change like this, it's necessary to remember to update each function (no compiler warning if one is missed). > > Of course, the perf win takes precedence, but it's worth thinking about.
Yeah, but the performance win is important here. And I think we won't miss this in the future because this "changing switch => isOneOfSourceParseMode" is only done in ParserModes.h, which defines SourceParseMode. I've added the comment at the head of SourceParseMode that describes "When we add a new mode, we should not forget to check predicates in this file".
Yusuke Suzuki
Comment 133
2016-05-31 11:24:44 PDT
Comment on
attachment 280137
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=280137&action=review
>>> Source/JavaScriptCore/parser/Parser.cpp:264 >>> + if (oneOfSourceParseModes<SourceParseMode::ArrowFunctionMode, SourceParseMode::AsyncArrowFunctionMode>(parseMode) && !hasError()) { >> >> I think it's more readable to have the == checks directly. Since moving to the bit representation, have you looked at the code LLVM generates for this? I suspect it does smart things and does some bit math for you. >> >> If it doesn't, then it's worth keeping this function. I don't really like the name though, but I'm not sure what a better name is. I'll think. > > For GCC, I made sure that this oneOfSourceParseModes is compiled into test & jmp, that is expected one. I'll check it in clang later. > For example, let's pick the parseFunctionParameters. In the early stage of the function, we perform, > > if (UNLIKELY((oneOfSourceParseModes<SourceParseMode::ArrowFunctionMode, SourceParseMode::AsyncArrowFunctionMode>(mode)))) { > > And here is the code dumped by objdump. In this code, rdi:(Parser's this), rsi:(SyntaxChecker&), rdx:(SourceParseMode) > > 00000000009598f0 <JSC::SyntaxChecker::FormalParameterList JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionParameters<JSC::SyntaxChecker, JSC::ParserFunctionInfo<JSC::ASTBuilder> >(JSC::SyntaxChecker&, JSC::SourceParseMode, JSC::ParserFunctionInfo<JSC::ASTBuild > er>&)>: > 9598f0: 41 57 push %r15 > 9598f2: 41 56 push %r14 > 9598f4: 41 55 push %r13 > 9598f6: 41 54 push %r12 > 9598f8: 55 push %rbp > 9598f9: 53 push %rbx > 9598fa: 48 83 ec 68 sub $0x68,%rsp > 9598fe: 64 48 8b 04 25 28 00 mov %fs:0x28,%rax > 959905: 00 00 > 959907: 48 89 44 24 58 mov %rax,0x58(%rsp) > 95990c: 31 c0 xor %eax,%eax > 95990e: f6 c6 70 test $0x70,%dh > 959911: 0f 85 49 01 00 00 jne 959a60 <JSC::SyntaxChecker::FormalParameterList JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionParameters<JSC::SyntaxChecker, JSC::ParserFunctionInfo<JSC::ASTBuilder> >(JSC::SyntaxChecker&, JSC::SourceParseMode, JSC::ParserFunctionInfo<JSC::ASTBuilder>&)+0x170> > 959917: f7 c2 40 08 00 00 test $0x840,%edx > 95991d: 8b 9f f4 20 00 00 mov 0x20f4(%rdi),%ebx > 959923: 49 89 ff mov %rdi,%r15 > 959926: 49 89 f4 mov %rsi,%r12 > 959929: 41 89 d6 mov %edx,%r14d > 95992c: 48 89 cd mov %rcx,%rbp > 95992f: c7 87 f4 20 00 00 00 movl $0x0,0x20f4(%rdi) > 959936: 00 00 00 > 959939: 8b 87 20 21 00 00 mov 0x2120(%rdi),%eax > 95993f: 0f 85 43 06 00 00 jne 959f88 <JSC::SyntaxChecker::FormalParameterList JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionParameters<JSC::SyntaxChecker, JSC::ParserFunctionInfo<JSC::ASTBuilder> >(JSC::SyntaxChecker&, JSC::SourceParseMode, JSC::ParserFunctionInfo<JSC::ASTBuilder>&)+0x698> > ... > 959a60: e8 fb 14 8f ff callq 24af60 <WTFCrash@plt> > > And we can find the great test & jmp in > > // if (UNLIKELY((oneOfSourceParseModes<SourceParseMode::ArrowFunctionMode, SourceParseMode::AsyncArrowFunctionMode>(mode)))) { > 959917: f7 c2 40 08 00 00 test $0x840,%edx > ... > 95993f: 0f 85 43 06 00 00 jne 959f88 <JSC::SyntaxChecker::FormalParameterList JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionParameters<JSC::SyntaxChecker, JSC::ParserFunctionInfo<JSC::ASTBuilder> >(JSC::SyntaxChecker&, JSC::SourceParseMode, JSC::ParserFunctionInfo<JSC::ASTBuilder>&)+0x698> > > This 0x840 => 0000100001000000 => ArrowFunctionMode | AsyncArrowFunctionMode. > Compared to these efficient test & jmp (They are macro-fused in x86 processor (reg imm test & jmp)), the old implementation becomes following. > > 00000000009583e0 <JSC::SyntaxChecker::FormalParameterList JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionParameters<JSC::SyntaxChecker, JSC::ParserFunctionInfo<JSC::SyntaxChecker> >(JSC::SyntaxChecker&, JSC::SourceParseMode, JSC::ParserFunctionInfo<JSC::SyntaxChecker>&)>: > 9583e0: 41 57 push %r15 > 9583e2: 41 56 push %r14 > 9583e4: 41 55 push %r13 > 9583e6: 41 54 push %r12 > 9583e8: 55 push %rbp > 9583e9: 53 push %rbx > 9583ea: 48 83 ec 68 sub $0x68,%rsp > 9583ee: 64 48 8b 04 25 28 00 mov %fs:0x28,%rax > 9583f5: 00 00 > 9583f7: 48 89 44 24 58 mov %rax,0x58(%rsp) > 9583fc: 31 c0 xor %eax,%eax > 9583fe: 8d 42 f4 lea -0xc(%rdx),%eax > 958401: 3c 02 cmp $0x2,%al > 958403: 0f 86 c7 01 00 00 jbe 9585d0 <JSC::SyntaxChecker::FormalParameterList JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionParameters<JSC::SyntaxChecker, JSC::ParserFunctionInfo<JSC::SyntaxChecker> >(JSC::SyntaxChecker&, JSC::SourceParseMode, JSC::ParserFunctionInfo<JSC::SyntaxChecker>&)+0x1f0> > 958409: 80 fa 06 cmp $0x6,%dl > 95840c: 8b 9f f4 20 00 00 mov 0x20f4(%rdi),%ebx > 958412: 49 89 ff mov %rdi,%r15 > 958415: 49 89 f4 mov %rsi,%r12 > 958418: 41 89 d6 mov %edx,%r14d > 95841b: 48 89 cd mov %rcx,%rbp > 95841e: c7 87 f4 20 00 00 00 movl $0x0,0x20f4(%rdi) > 958425: 00 00 00 > 958428: 0f 84 d2 00 00 00 je 958500 <JSC::SyntaxChecker::FormalParameterList JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionParameters<JSC::SyntaxChecker, JSC::ParserFunctionInfo<JSC::SyntaxChecker> >(JSC::SyntaxChecker&, JSC::SourceParseMode, JSC::ParserFunctionInfo<JSC::SyntaxChecker>&)+0x120> > 95842e: 80 fa 0b cmp $0xb,%dl > 958431: 0f 84 c9 00 00 00 je 958500 <JSC::SyntaxChecker::FormalParameterList JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionParameters<JSC::SyntaxChecker, JSC::ParserFunctionInfo<JSC::SyntaxChecker> >(JSC::SyntaxChecker&, JSC::SourceParseMode, JSC::ParserFunctionInfo<JSC::SyntaxChecker>&)+0x120> > > The same phase is compiled in, > > 958409: 80 fa 06 cmp $0x6,%dl > ... > 958428: 0f 84 d2 00 00 00 je 958500 <JSC::SyntaxChecker::FormalParameterList JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionParameters<JSC::SyntaxChecker, JSC::ParserFunctionInfo<JSC::SyntaxChecker> >(JSC::SyntaxChecker&, JSC::SourceParseMode, JSC::ParserFunctionInfo<JSC::SyntaxChecker>&)+0x120> > 95842e: 80 fa 0b cmp $0xb,%dl > 958431: 0f 84 c9 00 00 00 je 958500 <JSC::SyntaxChecker::FormalParameterList JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionParameters<JSC::SyntaxChecker, JSC::ParserFunctionInfo<JSC::SyntaxChecker> >(JSC::SyntaxChecker&, JSC::SourceParseMode, JSC::ParserFunctionInfo<JSC::SyntaxChecker>&)+0x120> > > (cmp & jmp) x2.
The first one's test & jmp is not macro-fused since they are not sequenced. (mov operations). But `RELEASE_ASSERT(!(oneOfSourceParseModes<SourceParseMode::ProgramMode, SourceParseMode::ModuleAnalyzeMode, SourceParseMode::ModuleEvaluateMode>(mode)));`'s test & jmp is macro-fused :) Since oneOfSourceParseModes is compiled into test, there are so many macro-fusion chance.
Saam Barati
Comment 134
2016-05-31 11:27:13 PDT
Thanks for looking up what code is generated. I remember looking at what llvm generates for our JSValue::isUndefinedOrNull() and it did some interesting bit manipulations. I thought it might do something similar here.
Ryosuke Niwa
Comment 135
2016-05-31 14:48:16 PDT
This patch regressed JetStream by 1%.
Geoffrey Garen
Comment 136
2016-05-31 14:50:23 PDT
I see some discussion here of a performance regression, and some continuing discussion in
https://bugs.webkit.org/show_bug.cgi?id=158211
about how to fix it. I don't think that's the right process. The right process to not to check in a patch until it is performance-neutral or a performance win. Given the JetStream regression as well, I think we need to roll this out.
Yusuke Suzuki
Comment 137
2016-05-31 21:38:08 PDT
(In reply to
comment #135
)
> This patch regressed JetStream by 1%.
Thanks, while
http://trac.webkit.org/changeset/201523
largely fixes the performance, it seems that there is still 0.3% performance regression in Octane code-load. (In reply to
comment #136
)
> I see some discussion here of a performance regression, and some continuing > discussion in
https://bugs.webkit.org/show_bug.cgi?id=158211
about how to > fix it. > > I don't think that's the right process. The right process to not to check in > a patch until it is performance-neutral or a performance win. > > Given the JetStream regression as well, I think we need to roll this out.
OK, I've just rolled out the two patches
http://trac.webkit.org/changeset/201481
. And spliting the patch into small peices and checking in them incrementally while keeping the performance.
Yusuke Suzuki
Comment 138
2016-05-31 21:39:29 PDT
(In reply to
comment #137
)
> (In reply to
comment #135
) > > This patch regressed JetStream by 1%. > > Thanks, while
http://trac.webkit.org/changeset/201523
largely fixes the > performance, it seems that there is still 0.3% performance regression in > Octane code-load. > > (In reply to
comment #136
) > > I see some discussion here of a performance regression, and some continuing > > discussion in
https://bugs.webkit.org/show_bug.cgi?id=158211
about how to > > fix it. > > > > I don't think that's the right process. The right process to not to check in > > a patch until it is performance-neutral or a performance win. > > > > Given the JetStream regression as well, I think we need to roll this out. > > OK, I've just rolled out the two patches >
http://trac.webkit.org/changeset/201481
. > And spliting the patch into small peices and checking in them incrementally > while keeping the performance.
incorrect link.
http://trac.webkit.org/changeset/201542
Ryosuke Niwa
Comment 139
2016-06-01 13:02:37 PDT
(In reply to
comment #137
)
> (In reply to
comment #135
) > > This patch regressed JetStream by 1%. > > Thanks, while
http://trac.webkit.org/changeset/201523
largely fixes the > performance, it seems that there is still 0.3% performance regression in > Octane code-load. > > (In reply to
comment #136
) > > I see some discussion here of a performance regression, and some continuing > > discussion in
https://bugs.webkit.org/show_bug.cgi?id=158211
about how to > > fix it. > > > > I don't think that's the right process. The right process to not to check in > > a patch until it is performance-neutral or a performance win. > > > > Given the JetStream regression as well, I think we need to roll this out. > > OK, I've just rolled out the two patches >
http://trac.webkit.org/changeset/201481
. > And spliting the patch into small peices and checking in them incrementally > while keeping the performance.
Surprisingly, this rollout didn't entirely fix the regression :(
Yusuke Suzuki
Comment 140
2016-06-01 21:55:59 PDT
(In reply to
comment #139
)
> (In reply to
comment #137
) > > (In reply to
comment #135
) > > > This patch regressed JetStream by 1%. > > > > Thanks, while
http://trac.webkit.org/changeset/201523
largely fixes the > > performance, it seems that there is still 0.3% performance regression in > > Octane code-load. > > > > (In reply to
comment #136
) > > > I see some discussion here of a performance regression, and some continuing > > > discussion in
https://bugs.webkit.org/show_bug.cgi?id=158211
about how to > > > fix it. > > > > > > I don't think that's the right process. The right process to not to check in > > > a patch until it is performance-neutral or a performance win. > > > > > > Given the JetStream regression as well, I think we need to roll this out. > > > > OK, I've just rolled out the two patches > >
http://trac.webkit.org/changeset/201481
. > > And spliting the patch into small peices and checking in them incrementally > > while keeping the performance. > > Surprisingly, this rollout didn't entirely fix the regression :(
:( Anyway, incremental landing is preferable.
Yusuke Suzuki
Comment 141
2016-08-25 18:41:51 PDT
Now, ES6 Generators in JSC is rewritten[1]. So this patch requires some reworks. Maybe, this op_yield stuff simplifies the current patch. [1]:
https://trac.webkit.org/changeset/204994
Caitlin Potter (:caitp)
Comment 142
2016-08-25 19:01:02 PDT
(In reply to
comment #141
)
> Now, ES6 Generators in JSC is rewritten[1]. > So this patch requires some reworks. Maybe, this op_yield stuff simplifies > the current patch. > > [1]:
https://trac.webkit.org/changeset/204994
I'll take a look at this tomorrow --- however, I believe the main cost of this as far as benchmarks are concerned is still 1) startup time (bootstrapping builtins implemented in JS, and 2) the additional workloads to be taken by the parser. It may still be difficult even with the new generators implementation to get this to not regress the common benchmarks.
Yusuke Suzuki
Comment 143
2016-08-25 22:56:58 PDT
(In reply to
comment #142
)
> (In reply to
comment #141
) > > Now, ES6 Generators in JSC is rewritten[1]. > > So this patch requires some reworks. Maybe, this op_yield stuff simplifies > > the current patch. > > > > [1]:
https://trac.webkit.org/changeset/204994
> > I'll take a look at this tomorrow ---
Nice!
> however, I believe the main cost of > this as far as benchmarks are concerned is still 1) startup time > (bootstrapping builtins implemented in JS,
Hmm, right. Currently, the global private functions are paresed at JSGlobalObject initialization phase (calling XXXCodeGenerator). Interesting. I think integrating Filip's Lazy property system may solve this situation. I need to look into it.
> and 2) the additional workloads > to be taken by the parser. It may still be difficult even with the new > generators implementation to get this to not regress the common benchmarks.
Maybe, the parser overhead is almost solved already with my patch I think. How is the regression of the current (your one + my change) patch?
Caitlin Potter (:caitp)
Comment 144
2016-08-30 14:58:31 PDT
This bug is getting a bit packed, so I've uploaded a new patch to
https://bugs.webkit.org/show_bug.cgi?id=161409
--- this only implements the frontend side, so that it's easier to eliminate the performance issues.
Joseph Pecoraro
Comment 145
2016-11-17 14:45:33 PST
Is this original bug still relevant? Maybe it can be closed? =)
Saam Barati
Comment 146
2016-11-17 15:03:03 PST
(In reply to
comment #145
)
> Is this original bug still relevant? Maybe it can be closed? =)
Yeah I think so. I'm closing it for now.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug