Bug 156147 - [JSC] implement async functions proposal
: [JSC] implement async functions proposal
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore
: WebKit Nightly Build
: Unspecified Unspecified
: P2 Normal
Assigned To: Yusuke Suzuki
:
Depends on: 158211 161409
Blocks:
  Show dependency treegraph
 
Reported: 2016-04-03 13:51 PDT by Caitlin Potter (:caitp)
Modified: 2016-11-17 15:03 PST (History)
17 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Caitlin Potter (:caitp) 2016-04-03 13:51:01 PDT
[JSC] implement async functions proposal
Comment 1 Caitlin Potter (:caitp) 2016-04-03 14:19:04 PDT
Created attachment 275508 [details]
Patch
Comment 2 Caitlin Potter (:caitp) 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.
Comment 3 Caitlin Potter (:caitp) 2016-04-03 14:45:42 PDT
Created attachment 275509 [details]
Patch

add AsyncFunctionPrototype.js to xcodeproj
Comment 4 Caitlin Potter (:caitp) 2016-04-03 15:09:18 PDT
Created attachment 275510 [details]
Patch

add AsyncFunctionPrototype.js to CMakeLists.txt
Comment 5 Sam Weinig 2016-04-03 15:10:47 PDT
Really cool!
Comment 6 Yusuke Suzuki 2016-04-03 17:29:48 PDT
great!! i can comment on the part using the generator system ;D
Comment 7 Caitlin Potter (:caitp) 2016-04-03 18:59:42 PDT
Created attachment 275521 [details]
Patch

refactor parser, maybe gets this building on EWS
Comment 8 Caitlin Potter (:caitp) 2016-04-03 19:16:59 PDT
Created attachment 275522 [details]
Patch

rebase, and hope it doesn't require more frontend changes
Comment 9 WebKit Commit Bot 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.
Comment 10 Build Bot 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.
Comment 11 Build Bot 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
Comment 12 Build Bot 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.
Comment 13 Build Bot 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
Comment 14 Build Bot 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.
Comment 15 Build Bot 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
Comment 16 Ryosuke Niwa 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.
Comment 17 Build Bot 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.
Comment 18 Build Bot 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
Comment 19 Caitlin Potter (:caitp) 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
Comment 20 Caitlin Potter (:caitp) 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
Comment 21 Geoffrey Garen 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.)
Comment 22 Caitlin Potter (:caitp) 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.
Comment 23 Saam Barati 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.
Comment 24 Caitlin Potter (:caitp) 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.
Comment 25 Saam Barati 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.
Comment 26 Geoffrey Garen 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.
Comment 27 Caitlin Potter (:caitp) 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
Comment 28 Caitlin Potter (:caitp) 2016-04-08 09:49:32 PDT
Created attachment 276009 [details]
Patch
Comment 29 WebKit Commit Bot 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.
Comment 30 Build Bot 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
Comment 31 Build Bot 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
Comment 32 Build Bot 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
Comment 33 Build Bot 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
Comment 34 Build Bot 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
Comment 35 Build Bot 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
Comment 36 Build Bot 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
Comment 37 Build Bot 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
Comment 38 Caitlin Potter (:caitp) 2016-04-08 11:36:52 PDT
Created attachment 276020 [details]
Patch
Comment 39 WebKit Commit Bot 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.
Comment 40 Caitlin Potter (:caitp) 2016-04-08 11:57:09 PDT
Created attachment 276022 [details]
Patch
Comment 41 WebKit Commit Bot 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.
Comment 42 Caitlin Potter (:caitp) 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.
Comment 43 Geoffrey Garen 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?
Comment 44 Caitlin Potter (:caitp) 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.
Comment 45 Caitlin Potter (:caitp) 2016-04-09 12:22:52 PDT
Created attachment 276089 [details]
Patch

Switch to using RuntimeFlags --- InspectorRuntimeAgent gets this wrong
Comment 46 WebKit Commit Bot 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.
Comment 47 Caitlin Potter (:caitp) 2016-04-09 15:52:19 PDT
Created attachment 276098 [details]
Patch

minor fixup in win port that might fix ews
Comment 48 WebKit Commit Bot 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.
Comment 49 Caitlin Potter (:caitp) 2016-04-11 08:11:47 PDT
Created attachment 276150 [details]
Patch

actually install AsyncFunction constructor when flag enabled
Comment 50 WebKit Commit Bot 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.
Comment 51 Caitlin Potter (:caitp) 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
Comment 52 Caitlin Potter (:caitp) 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
Comment 53 Caitlin Potter (:caitp) 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.
Comment 54 WebKit Commit Bot 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.
Comment 55 Saam Barati 2016-04-14 13:04:04 PDT
Hi Caitlin,
I'm traveling today but will make an effort to review tomorrow.
Comment 56 Caitlin Potter (:caitp) 2016-04-16 15:55:18 PDT
Created attachment 276565 [details]
Patch

remove comments from test and re-up
Comment 57 WebKit Commit Bot 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.
Comment 58 Saam Barati 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.
Comment 59 Caitlin Potter (:caitp) 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
Comment 60 Caitlin Potter (:caitp) 2016-04-19 14:29:15 PDT
Created attachment 276758 [details]
Patch

use BytecodeIntrinsic constants instead in self hosted wrapper code
Comment 61 WebKit Commit Bot 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.
Comment 62 Caitlin Potter (:caitp) 2016-04-25 21:24:57 PDT
Created attachment 277330 [details]
Patch

Fix stylechecker errors to avoid spam, rebased to keep it fresh
Comment 63 Caitlin Potter (:caitp) 2016-04-29 08:52:39 PDT
Created attachment 277702 [details]
Patch

weekly rebase...
Comment 64 Yusuke Suzuki 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.
Comment 65 Caitlin Potter (:caitp) 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 :)
Comment 66 Yusuke Suzuki 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.
Comment 67 Yusuke Suzuki 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.
Comment 68 Caitlin Potter (:caitp) 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.
Comment 69 Caitlin Potter (:caitp) 2016-05-02 11:58:37 PDT
Created attachment 277919 [details]
Patch

addressed Yusuke Suzukis comments
Comment 70 Build Bot 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.
Comment 71 Build Bot 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
Comment 72 Caitlin Potter (:caitp) 2016-05-02 15:01:26 PDT
Created attachment 277938 [details]
Patch

Fix bugs from last patchset, add some new tests
Comment 73 Yusuke Suzuki 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.
Comment 74 Caitlin Potter (:caitp) 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
Comment 75 Caitlin Potter (:caitp) 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
Comment 76 Caitlin Potter (:caitp) 2016-05-06 17:49:05 PDT
Created attachment 278300 [details]
Patch

make efl-wk2 happy
Comment 77 Yusuke Suzuki 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?)
Comment 78 Caitlin Potter (:caitp) 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
Comment 79 Caitlin Potter (:caitp) 2016-05-11 20:52:22 PDT
Created attachment 278692 [details]
Patch

addressed that last comment
Comment 80 Yusuke Suzuki 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.
Comment 81 Caitlin Potter (:caitp) 2016-05-12 09:34:08 PDT
Created attachment 278735 [details]
Patch

Fixups
Comment 82 Caitlin Potter (:caitp) 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
Comment 83 Caitlin Potter (:caitp) 2016-05-18 06:33:16 PDT
Created attachment 279242 [details]
Patch

Keeping it unbitrotten~
Comment 84 Caitlin Potter (:caitp) 2016-05-21 19:03:04 PDT
Created attachment 279546 [details]
Patch

fix failures
Comment 85 Caitlin Potter (:caitp) 2016-05-25 15:46:45 PDT
Created attachment 279829 [details]
Patch

Please take another look when you get some time =)
Comment 86 Yusuke Suzuki 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!
Comment 87 Yusuke Suzuki 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()
Comment 88 Caitlin Potter (:caitp) 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
Comment 89 Caitlin Potter (:caitp) 2016-05-26 16:05:48 PDT
Created attachment 279921 [details]
Patch

more tests, some fixes, and bad news re: benchmarks
Comment 90 Saam Barati 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.
Comment 91 Saam Barati 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.
Comment 92 Saam Barati 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>`
Comment 93 Yusuke Suzuki 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 :)
Comment 94 Caitlin Potter (:caitp) 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.
Comment 95 Caitlin Potter (:caitp) 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?
Comment 96 Saam Barati 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.
Comment 97 Caitlin Potter (:caitp) 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
Comment 98 Yusuke Suzuki 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 :))
Comment 99 Caitlin Potter (:caitp) 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
Comment 100 Yusuke Suzuki 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 :)
Comment 101 WebKit Commit Bot 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>
Comment 102 WebKit Commit Bot 2016-05-27 22:43:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 103 Saam Barati 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?
Comment 104 Caitlin Potter (:caitp) 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
Comment 105 Saam Barati 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
Comment 106 Saam Barati 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.
Comment 107 Caitlin Potter (:caitp) 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
Comment 108 Saam Barati 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?
Comment 109 Yusuke Suzuki 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.
Comment 110 Saam Barati 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.
Comment 111 Saam Barati 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.
Comment 112 Yusuke Suzuki 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.
Comment 113 Yusuke Suzuki 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
Comment 114 Caitlin Potter (:caitp) 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
Comment 115 Yusuke Suzuki 2016-05-29 08:08:35 PDT
I think I've just found several fixes. Still investigating...
Comment 116 Yusuke Suzuki 2016-05-30 07:00:32 PDT
Reopening to attach new patch.
Comment 117 Yusuke Suzuki 2016-05-30 07:00:36 PDT
Created attachment 280096 [details]
Patch

WIP: still slower than non-landing version. But faster than ToT
Comment 118 Yusuke Suzuki 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
Comment 119 Yusuke Suzuki 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
Comment 120 WebKit Commit Bot 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.
Comment 121 Yusuke Suzuki 2016-05-30 11:15:52 PDT
Created attachment 280104 [details]
Patch
Comment 122 Yusuke Suzuki 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.
Comment 123 Build Bot 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.
Comment 124 Build Bot 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
Comment 125 Caitlin Potter (:caitp) 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.
Comment 126 Caitlin Potter (:caitp) 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.
Comment 127 Yusuke Suzuki 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
Comment 128 Yusuke Suzuki 2016-05-31 03:28:53 PDT
Created attachment 280133 [details]
Patch

Inserted links to FIXMEs
Comment 129 Yusuke Suzuki 2016-05-31 04:04:01 PDT
Created attachment 280137 [details]
Patch

Fix build error on OS X debug bot
Comment 130 Saam Barati 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
Comment 131 Caitlin Potter (:caitp) 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.
Comment 132 Yusuke Suzuki 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".
Comment 133 Yusuke Suzuki 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.
Comment 134 Saam Barati 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.
Comment 135 Ryosuke Niwa 2016-05-31 14:48:16 PDT
This patch regressed JetStream by 1%.
Comment 136 Geoffrey Garen 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.
Comment 137 Yusuke Suzuki 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.
Comment 138 Yusuke Suzuki 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
Comment 139 Ryosuke Niwa 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 :(
Comment 140 Yusuke Suzuki 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.
Comment 141 Yusuke Suzuki 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
Comment 142 Caitlin Potter (:caitp) 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.
Comment 143 Yusuke Suzuki 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?
Comment 144 Caitlin Potter (:caitp) 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.
Comment 145 Joseph Pecoraro 2016-11-17 14:45:33 PST
Is this original bug still relevant? Maybe it can be closed? =)
Comment 146 Saam Barati 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.