Bug 161409

Summary: [JSC] Implement parsing of Async Functions
Product: WebKit Reporter: Caitlin Potter (:caitp) <caitp>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, darin, fpizlo, ggaren, keith_miller, mark.lam, msaboff, rniwa, saam, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 156147    
Attachments:
Description Flags
Patch
none
Patch
none
Async Function Parsing v1
none
Async Function Parsing v2
none
Patch
none
Async Function Parsing v3
none
Async Function Parsing v4
none
Patch
none
Async Function Parsing v5
none
Async Function Parsing v6
none
Async Function Parsing v6 + Copy-edit the build-jsc help message changes
none
Archive of layout-test-results from ews117 for mac-yosemite
none
Patch
none
Async Function Parsing v7
none
Async Function Parsing v8
none
Async Function Parsing v8 + fix ChangeLog
none
Async Function Parsing v8 + fix ChangeLog + finish renaming variables/methods
none
Archive of layout-test-results from ews107 for mac-yosemite-wk2
none
Archive of layout-test-results from ews101 for mac-yosemite
none
Async Function Parsing v8 + rebased and given another EWS try
none
Async Function Parsing v8
none
Async Function Parsing v8
none
Async Function Parsing v8
none
Async Function Parsing v8 none

Description Caitlin Potter (:caitp) 2016-08-30 14:56:53 PDT
[JSC] Implement parsing of Async Functions
Comment 1 Caitlin Potter (:caitp) 2016-08-30 15:01:08 PDT
TODO:

- Put async function parsing behind compile-time flag (the runtime flag approach in 156147 was probably not worth it)
- Fix bug 161408 so that test FIXME can be uncommented
- Fix module parsing errors re: AWAIT keyword (Scope objects no longer know if they're parsing a module or not, have not yet introduced a fix)
Comment 2 Caitlin Potter (:caitp) 2016-08-30 15:04:18 PDT
Created attachment 287437 [details]
Patch
Comment 3 WebKit Commit Bot 2016-08-30 15:06:19 PDT
Attachment 287437 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Keywords.table:10:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Caitlin Potter (:caitp) 2016-08-30 15:38:00 PDT
Created attachment 287442 [details]
Patch

Get rid of the xcodeproj changes, since the new files aren't included in this patch.
Comment 5 Caitlin Potter (:caitp) 2016-08-30 16:02:54 PDT
Created attachment 287444 [details]
Async Function Parsing v1

Fix modules test failures
Comment 6 Caitlin Potter (:caitp) 2016-09-07 12:21:54 PDT
So, experimenting with this a bit, I'm finding that those "expensive branches" identified by Yusuke Suzuki can be (mostly) mitigated in the benchmarks by moving their contents to never-inlined functions. I guess this makes the on-stack allocation for the more commonly executed code smaller, and means the only extra work the "commonly executed" code has to do is the interned string comparison (testing if a particular string is "async" or not), which should be pretty cheap.

The biggest one of these in the jQuery benchmark is, according to Instruments.app, the one in parseStatement(). Uninlining these rare branches might be a good way to go, but I'd appreciate a second set of eyes measuring that solution using different tools and settings as well.

Some benchmark runs (just with jquery) with 4 VM runs and 10 inner runs:

```
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.

1)
                         Baseline                  Patched                                      

jquery               6.76557+-0.13436    ?     6.89083+-0.08124       ? might be 1.0185x slower

<geometric>          6.76557+-0.13436    ?     6.89083+-0.08124       ? might be 1.0185x slower

2)
                         Baseline                  Patched                                      

jquery               7.06592+-0.09167          7.03025+-0.07808       

<geometric>          7.06592+-0.09167          7.03025+-0.07808         might be 1.0051x faster
```

So, I dunno, that seems like an improvement over the existing regression to me.
Comment 7 Yusuke Suzuki 2016-09-07 13:31:00 PDT
(In reply to comment #6)
> So, experimenting with this a bit, I'm finding that those "expensive
> branches" identified by Yusuke Suzuki can be (mostly) mitigated in the
> benchmarks by moving their contents to never-inlined functions. I guess this
> makes the on-stack allocation for the more commonly executed code smaller,
> and means the only extra work the "commonly executed" code has to do is the
> interned string comparison (testing if a particular string is "async" or
> not), which should be pretty cheap.
> 
> The biggest one of these in the jQuery benchmark is, according to
> Instruments.app, the one in parseStatement(). Uninlining these rare branches
> might be a good way to go, but I'd appreciate a second set of eyes measuring
> that solution using different tools and settings as well.
> 
> Some benchmark runs (just with jquery) with 4 VM runs and 10 inner runs:
> 
> ```
> 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.
> 
> 1)
>                          Baseline                  Patched                  
> 
> 
> jquery               6.76557+-0.13436    ?     6.89083+-0.08124       ?
> might be 1.0185x slower
> 
> <geometric>          6.76557+-0.13436    ?     6.89083+-0.08124       ?
> might be 1.0185x slower
> 
> 2)
>                          Baseline                  Patched                  
> 
> 
> jquery               7.06592+-0.09167          7.03025+-0.07808       
> 
> <geometric>          7.06592+-0.09167          7.03025+-0.07808        
> might be 1.0051x faster
> ```
> 
> So, I dunno, that seems like an improvement over the existing regression to
> me.

Great! Did you already upload the updated patch?
Comment 8 Caitlin Potter (:caitp) 2016-09-07 15:11:44 PDT
no, but the change is pretty minor, so im not sure if it has the benefit i imagine it does. I'll put it up in a sec
Comment 9 Caitlin Potter (:caitp) 2016-09-07 15:15:28 PDT
Created attachment 288192 [details]
Async Function Parsing v2

Patch to try to reduce codeload regression a bit
Comment 10 Caitlin Potter (:caitp) 2016-09-08 14:16:47 PDT
Ping: have you tried this and seen similar results re: the perf regression on jQuery tending to be significantly lower than it was originally? I'd like to establish that the perf is acceptable before adding the runtime portions
Comment 11 Yusuke Suzuki 2016-09-08 14:18:44 PDT
(In reply to comment #10)
> Ping: have you tried this and seen similar results re: the perf regression
> on jQuery tending to be significantly lower than it was originally? I'd like
> to establish that the perf is acceptable before adding the runtime portions

Last night, I tried the patch in my Linux box (64bit), and I've got 1~1.5% regression in octane jquery and closure.
Today, I'll re-check it in my OS X laptop.
Comment 12 Caitlin Potter (:caitp) 2016-09-08 14:23:11 PDT
That still sounds pretty bad, if not worse than it was originally measured to be. I think I will keep looking for improvements.
Comment 13 Michael Catanzaro 2016-09-08 20:20:52 PDT
Comment on attachment 288192 [details]
Async Function Parsing v2

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

> JSTests/stress/async-await-syntax.js:1
> +// Copyright (C) 2016 the V8 project authors. All rights reserved.

Careful, lack of license is not acceptable for WebKit. I hope it's unintentional? Can you get it fixed to BSD, like other Chromium project stuff?
Comment 14 Caitlin Potter (:caitp) 2016-09-08 20:36:33 PDT
Created attachment 288384 [details]
Patch
Comment 15 Yusuke Suzuki 2016-09-09 11:49:22 PDT
I took the benchmark on my OS X laptop last night.

shell> Tools/Scripts/run-jsc-benchmarks baseline:WebKitBuild/async-master/Release/jsc patched:WebKitBuild/async/Release/jsc --outer=30 --octane --benchmark='closure|jquery'

Generating benchmark report at /Users/yusukesuzuki/dev/WebKit/baseline_patched_Octane_dandelion_20160909_1144_report.txt
And raw data at /Users/yusukesuzuki/dev/WebKit/baseline_patched_Octane_dandelion_20160909_1144.json

Benchmark report for Octane on dandelion (MacBookPro10,1).

VMs tested:
"baseline" at /Users/yusukesuzuki/dev/WebKit/WebKitBuild/async-master/Release/jsc
"patched" at /Users/yusukesuzuki/dev/WebKit/WebKitBuild/async/Release/jsc

Collected 30 samples per benchmark/VM, with 30 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.59315+-0.00124          0.59265+-0.00138       
jquery               7.91025+-0.02773    ?     7.91560+-0.03665       ?

<geometric>          2.16606+-0.00448          2.16585+-0.00537         might be 1.0001x faster

It seems that the performance is neutral.
Comment 16 Caitlin Potter (:caitp) 2016-09-09 12:00:20 PDT
Do you have any ideas what might be the result of the poor performance on the linux runs?
Comment 17 Caitlin Potter (:caitp) 2016-09-09 12:01:41 PDT
er, the cause of the poor linux perf.
Comment 18 Yusuke Suzuki 2016-09-09 14:09:25 PDT
(In reply to comment #17)
> er, the cause of the poor linux perf.

OK, I've retaken the performance on my Linux box with rebooting.

Generating benchmark report at /home/yusukesuzuki/dev/WebKit/baseline_patch_Octane_hanayamata_20160910_0605_report.txt
And raw data at /home/yusukesuzuki/dev/WebKit/baseline_patch_Octane_hanayamata_20160910_0605.json

Benchmark report for Octane on hanayamata.

VMs tested:
"baseline" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/async-master/Release/bin/jsc
"patch" 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                   patch                                       

closure              0.62470+-0.01647          0.62386+-0.00330       
jquery               8.14041+-0.09801    ?     8.17138+-0.04920       ?

<geometric>          2.25454+-0.02681    ?     2.25780+-0.00850       ? might be 1.0014x slower

Seems good.
Comment 19 Yusuke Suzuki 2016-09-09 14:40:23 PDT
Comment on attachment 288384 [details]
Patch

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

Could you hide async / await parsing feature behind compile time / run time flag?

> Source/JavaScriptCore/ChangeLog:7
> +

Could you note ChangeLog?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2980
> +    ASSERT(func->metadata()->parseMode() == SourceParseMode::ArrowFunctionMode || func->metadata()->parseMode() == SourceParseMode::AsyncArrowFunctionMode);

Let's use SourceParseModeSet here.

> Source/JavaScriptCore/parser/Parser.cpp:264
> +        if (parseMode == SourceParseMode::GeneratorBodyMode || isAsyncFunctionBodyParseMode(parseMode))

Let's use SourceParseModeSet even if there is only one candidate. See the reason why https://bugs.webkit.org/show_bug.cgi?id=156147#c132.

> Source/JavaScriptCore/parser/Parser.cpp:269
> +        if (oneOfSourceParseModes<SourceParseMode::ArrowFunctionMode, SourceParseMode::AsyncArrowFunctionMode>(parseMode) && !hasError()) {

Could you change this oneOfSourceParseModes to one done in https://trac.webkit.org/changeset/201523 (SourceParseModeSet) ?

> Source/JavaScriptCore/parser/Parser.cpp:315
> +    if (oneOfSourceParseModes<SourceParseMode::GeneratorWrapperFunctionMode>(parseMode) || isAsyncFunctionWrapperParseMode(parseMode)) {

Ditto

> Source/JavaScriptCore/parser/Parser.cpp:559
> +    info.parameterCount = 4; // generator, state, value, resume mode

We no longer need this code since createGeneratorParameters automatically set info.parameterCount.
And keep in mind that the updated generator takes 5th argument, @generatorFrame.

> Source/JavaScriptCore/parser/Parser.cpp:629
> +        // FIXME: These branch causes 1% regression in octane code-load jquery.

Since we already solved this, let's drop it.

> Source/JavaScriptCore/parser/Parser.cpp:1748
> +        // FIXME: These branch causes 1% regression in octane code-load jquery.

Ditto.

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

Ditto

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

Ditto.

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

Ditto.

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

Ditto.

> Source/JavaScriptCore/parser/Parser.cpp:2179
> +        if (!(functionDefinitionType == FunctionDefinitionType::Expression && oneOfSourceParseModes<SourceParseMode::NormalFunctionMode, SourceParseMode::AsyncFunctionMode>(mode)))

Ditto.

> Source/JavaScriptCore/parser/Parser.cpp:2294
> +        RELEASE_ASSERT((oneOfSourceParseModes<SourceParseMode::NormalFunctionMode, SourceParseMode::MethodMode, SourceParseMode::ArrowFunctionMode, SourceParseMode::GeneratorBodyMode, SourceParseMode::GeneratorWrapperFunctionMode>(mode)) || isAsyncFunctionWrapperParseMode(mode));

Ditto.

> Source/JavaScriptCore/parser/Parser.cpp:2722
> +        // FIXME: These branch causes 1% regression in octane code-load jquery.

Since it is already solved, we can drop this FIXME.

> Source/JavaScriptCore/parser/Parser.cpp:3353
> +    // FIXME: These branch causes 1% regression in octane code-load jquery.

Ditto.

> Source/JavaScriptCore/parser/Parser.cpp:3512
> +    return context.createYield(location, argument, delegate, divotStart, argumentStart, lastTokenEndPosition());

How about using AwaitNode here and emit the similar code in code generation phase?

> Source/JavaScriptCore/parser/Parser.cpp:4117
> +        // FIXME: These branch causes 1% regression in octane code-load jquery.

Ditto.

> Source/JavaScriptCore/parser/ParserModes.h:76
> +static ALWAYS_INLINE bool oneOfSourceParseModes(SourceParseMode mode)

Change to SourceParseModeSet version.

> Source/JavaScriptCore/parser/ParserModes.h:83
> +    return oneOfSourceParseModes<

Ditto.

> Source/JavaScriptCore/parser/ParserModes.h:100
> +    return oneOfSourceParseModes<

Ditto.

> Source/JavaScriptCore/parser/ParserModes.h:110
> +    return oneOfSourceParseModes<

Ditto.

> Source/JavaScriptCore/parser/ParserModes.h:117
> +    return oneOfSourceParseModes<

Ditto.

> Source/JavaScriptCore/parser/ParserModes.h:125
> +    return oneOfSourceParseModes<

Ditto.

> Source/JavaScriptCore/parser/ParserModes.h:132
> +    return oneOfSourceParseModes<

Ditto.

> Source/JavaScriptCore/parser/ParserModes.h:142
> +    return oneOfSourceParseModes<

Ditto.

> Source/JavaScriptCore/parser/ParserModes.h:149
> +    return oneOfSourceParseModes<SourceParseMode::ProgramMode>(parseMode);

Ditto.
Comment 20 Caitlin Potter (:caitp) 2016-09-09 16:53:01 PDT
(In reply to comment #19)
> Comment on attachment 288384 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288384&action=review
> 
> Could you hide async / await parsing feature behind compile time / run time
> flag?

I'll get around to addressing the review comments on Monday. But, before I leave this for the weekend, question re: your overall comments.

1) If using the compile-time flag option, is there any way I can keep tests added in this patch running and passing? I haven't really found a way to do this.

2) If a runtime flag approach is used, I don't think the runtime flag approach from the other bug is a good way to go, since it means adding flags all over the place when it's not realistically needed. A single global boolean in runtime/Options.cpp would probably make more sense, and would avoid changes all over the place for storing parser flags in various places where the parser might need to be entered again. Does that seem reasonable, or is the RuntimeFlags class the absolute preferred way to go?
Comment 21 Caitlin Potter (:caitp) 2016-09-09 19:29:26 PDT
Created attachment 288477 [details]
Async Function Parsing v3
Comment 22 Caitlin Potter (:caitp) 2016-09-09 19:30:26 PDT
Comment on attachment 288477 [details]
Async Function Parsing v3

Most of the review addressed, just waiting on an answer to the questions before adding the flag
Comment 23 Yusuke Suzuki 2016-09-10 16:53:37 PDT
(In reply to comment #22)
> Comment on attachment 288477 [details]
> Async Function Parsing v3
> 
> Most of the review addressed, just waiting on an answer to the questions
> before adding the flag

Runtime flag based on Options.{h, cpp} is ok to me.
Comment 24 Caitlin Potter (:caitp) 2016-09-11 08:15:44 PDT
Created attachment 288528 [details]
Async Function Parsing v4
Comment 25 Caitlin Potter (:caitp) 2016-09-11 08:17:04 PDT
Comment on attachment 288528 [details]
Async Function Parsing v4

Added new version with Options::useAsyncFunctions() to hide the feature
Comment 26 Caitlin Potter (:caitp) 2016-09-11 08:46:43 PDT
with the flag, I am again seeing pretty poor results on the benchmarks.
Comment 27 Caitlin Potter (:caitp) 2016-09-12 09:11:34 PDT
(In reply to comment #26)
> with the flag, I am again seeing pretty poor results on the benchmarks.

I've tried giving the Parser a local copy of the Options value, and rearranged branches which rely on it a bit, seems to help a bit locally, but maybe it's not really needed.
Comment 28 Yusuke Suzuki 2016-09-14 19:43:13 PDT
(In reply to comment #27)
> (In reply to comment #26)
> > with the flag, I am again seeing pretty poor results on the benchmarks.
> 
> I've tried giving the Parser a local copy of the Options value, and
> rearranged branches which rely on it a bit, seems to help a bit locally, but
> maybe it's not really needed.

Oh... So compile time flag is OK. Thank you for trying!
Comment 29 Caitlin Potter (:caitp) 2016-09-15 21:01:17 PDT
I'll try to do that next week... I guess all I can do for the test is add a //@ skip line?
Comment 30 Yusuke Suzuki 2016-09-15 21:40:13 PDT
(In reply to comment #29)
> I'll try to do that next week... I guess all I can do for the test is add a
> //@ skip line?

Yup!
Comment 31 Caitlin Potter (:caitp) 2016-09-19 17:02:18 PDT
Created attachment 289286 [details]
Patch
Comment 32 Caitlin Potter (:caitp) 2016-09-19 17:04:25 PDT
I've changed this to use a feature flag, and am again seeing neutral, or effectively neutral parser performance. This seems like a good thing.
Comment 33 Caitlin Potter (:caitp) 2016-09-19 17:14:25 PDT
Created attachment 289289 [details]
Async Function Parsing v5
Comment 34 Yusuke Suzuki 2016-09-19 20:02:44 PDT
Comment on attachment 289289 [details]
Async Function Parsing v5

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

r=me with comments.

> Source/JavaScriptCore/parser/Parser.cpp:3174
> +            ExpressionErrorClassifier classifier(this);

Is this necessary? parseAssignmentExpression(context) has ExpressionErrorClassifier inside it.

> Source/JavaScriptCore/parser/Parser.cpp:3522
> +    // AwaitExpression desugared to YieldExpression

This comment is not necessary.

> Source/JavaScriptCore/parser/Parser.h:320
> +    }

What is the purpose of this `isModule()`?
Comment 35 Caitlin Potter (:caitp) 2016-09-21 05:42:31 PDT
Created attachment 289446 [details]
Async Function Parsing v6
Comment 36 Caitlin Potter (:caitp) 2016-09-21 05:44:26 PDT
Comment on attachment 289289 [details]
Async Function Parsing v5

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

I've uploaded the patch instead of manually committing, I'd like someone to sign off on the changes to build-jsc, just in case it's not quite right (and I haven't tested the cmake build for it at all)

>> Source/JavaScriptCore/parser/Parser.cpp:3174
>> +            ExpressionErrorClassifier classifier(this);
> 
> Is this necessary? parseAssignmentExpression(context) has ExpressionErrorClassifier inside it.

Acknowledged

>> Source/JavaScriptCore/parser/Parser.cpp:3522
>> +    // AwaitExpression desugared to YieldExpression
> 
> This comment is not necessary.

Acknowledged

>> Source/JavaScriptCore/parser/Parser.h:320
>> +    }
> 
> What is the purpose of this `isModule()`?

m_moduleScopeData used to be a member of Scope, so this made sense back then. I've gotten rid of this method, and just check if the one in the parser is null or not instead.

This seems to fix a bug where modules allow "await" as an identifier in nested functions (which was incorrect per https://tc39.github.io/ecma262/#prod-FutureReservedWord)
Comment 37 Caitlin Potter (:caitp) 2016-09-21 05:54:21 PDT
Created attachment 289447 [details]
Async Function Parsing v6 + Copy-edit the build-jsc help message changes
Comment 38 Build Bot 2016-09-21 07:13:31 PDT
Comment on attachment 289447 [details]
Async Function Parsing v6 + Copy-edit the build-jsc help message changes

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

New failing tests:
imported/w3c/web-platform-tests/media-source/mediasource-duration.html
Comment 39 Build Bot 2016-09-21 07:13:36 PDT
Created attachment 289449 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 40 Yusuke Suzuki 2016-09-21 15:34:39 PDT
Comment on attachment 289289 [details]
Async Function Parsing v5

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

>>> Source/JavaScriptCore/parser/Parser.h:320
>>> +    }
>> 
>> What is the purpose of this `isModule()`?
> 
> m_moduleScopeData used to be a member of Scope, so this made sense back then. I've gotten rid of this method, and just check if the one in the parser is null or not instead.
> 
> This seems to fix a bug where modules allow "await" as an identifier in nested functions (which was incorrect per https://tc39.github.io/ecma262/#prod-FutureReservedWord)

Nice. Make sense.
Comment 41 Yusuke Suzuki 2016-09-21 16:16:18 PDT
Comment on attachment 289447 [details]
Async Function Parsing v6 + Copy-edit the build-jsc help message changes

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

r=me, with nits.

> Source/JavaScriptCore/ChangeLog:87
> +        * runtime/CommonIdentifiers.h:

Could you update this ChangeLog by using Tools/Scripts/webkit-patch upload --update-changelogs?

> Source/JavaScriptCore/parser/Parser.h:1560
> +        // FIXME: `m_moduleScopeData.get() != nullptr` is not a valid method of determining if parser is parsing a Module.

Could you open a bug for this and note the URL here?
And I think this can be easily fixed since we already have this information: JSParserCommentMode {Classic, Module}!
This flag is named like this since currently it is only used for controlling comment mode. When you use this information here, you need to rename this mode to something different one (maybe, JSParserScriptMode?).

> Source/JavaScriptCore/parser/Parser.h:1568
> +        // FIXME: `m_moduleScopeData.get() != nullptr` is not a valid method of determining if parser is parsing a Module.

Ditto.

> Tools/Scripts/build-jsc:211
> +            if ($featureEnabled != $_->{default}) {

For string, we need to use ne instead of !=. This causes warnings.
Comment 42 Darin Adler 2016-09-21 16:30:32 PDT
Comment on attachment 289447 [details]
Async Function Parsing v6 + Copy-edit the build-jsc help message changes

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

>> Tools/Scripts/build-jsc:211
>> +            if ($featureEnabled != $_->{default}) {
> 
> For string, we need to use ne instead of !=. This causes warnings.

Not just warnings, incorrect behavior! Using != causes it to do a numeric comparison, not a string one. For example, ("ON" != "OFF") is false because both strings have a numeric value of 0.
Comment 43 Caitlin Potter (:caitp) 2016-09-21 16:32:35 PDT
Created attachment 289488 [details]
Patch
Comment 44 Caitlin Potter (:caitp) 2016-09-21 16:35:24 PDT
Created attachment 289489 [details]
Async Function Parsing v7
Comment 45 Darin Adler 2016-09-21 16:36:42 PDT
Comment on attachment 289489 [details]
Async Function Parsing v7

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

> Tools/Scripts/build-jsc:206
> +            if ($featureEnabled != $_->{default}) {

ne
Comment 46 Caitlin Potter (:caitp) 2016-09-21 16:37:46 PDT
Comment on attachment 289447 [details]
Async Function Parsing v6 + Copy-edit the build-jsc help message changes

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

>> Source/JavaScriptCore/ChangeLog:87
>> +        * runtime/CommonIdentifiers.h:
> 
> Could you update this ChangeLog by using Tools/Scripts/webkit-patch upload --update-changelogs?

Done

>> Source/JavaScriptCore/parser/Parser.h:1560
>> +        // FIXME: `m_moduleScopeData.get() != nullptr` is not a valid method of determining if parser is parsing a Module.
> 
> Could you open a bug for this and note the URL here?
> And I think this can be easily fixed since we already have this information: JSParserCommentMode {Classic, Module}!
> This flag is named like this since currently it is only used for controlling comment mode. When you use this information here, you need to rename this mode to something different one (maybe, JSParserScriptMode?).

For the uses in this patch, I think this actually doesn't matter here, because nested functions will never be re-parsed if `isDisallowedIdentifierAwait()` triggers an error (which it always should here).

Maybe the FIXMEs are better to just be removed.

>>> Tools/Scripts/build-jsc:211
>>> +            if ($featureEnabled != $_->{default}) {
>> 
>> For string, we need to use ne instead of !=. This causes warnings.
> 
> Not just warnings, incorrect behavior! Using != causes it to do a numeric comparison, not a string one. For example, ("ON" != "OFF") is false because both strings have a numeric value of 0.

Done --- Right, this was sloppily written entirely and has been fixed up. Note, the behaviour is different from build-webkit, but I don't think it should really matter. Thanks for the look and testing on cmake.
Comment 47 Yusuke Suzuki 2016-09-21 16:48:36 PDT
Comment on attachment 289447 [details]
Async Function Parsing v6 + Copy-edit the build-jsc help message changes

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

>>>> Tools/Scripts/build-jsc:211
>>>> +            if ($featureEnabled != $_->{default}) {
>>> 
>>> For string, we need to use ne instead of !=. This causes warnings.
>> 
>> Not just warnings, incorrect behavior! Using != causes it to do a numeric comparison, not a string one. For example, ("ON" != "OFF") is false because both strings have a numeric value of 0.
> 
> Done --- Right, this was sloppily written entirely and has been fixed up. Note, the behaviour is different from build-webkit, but I don't think it should really matter. Thanks for the look and testing on cmake.

Oops, thanks!
Comment 48 Yusuke Suzuki 2016-09-21 17:02:34 PDT
Comment on attachment 289447 [details]
Async Function Parsing v6 + Copy-edit the build-jsc help message changes

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

>>> Source/JavaScriptCore/parser/Parser.h:1560
>>> +        // FIXME: `m_moduleScopeData.get() != nullptr` is not a valid method of determining if parser is parsing a Module.
>> 
>> Could you open a bug for this and note the URL here?
>> And I think this can be easily fixed since we already have this information: JSParserCommentMode {Classic, Module}!
>> This flag is named like this since currently it is only used for controlling comment mode. When you use this information here, you need to rename this mode to something different one (maybe, JSParserScriptMode?).
> 
> For the uses in this patch, I think this actually doesn't matter here, because nested functions will never be re-parsed if `isDisallowedIdentifierAwait()` triggers an error (which it always should here).
> 
> Maybe the FIXMEs are better to just be removed.

Yeah, in this patch, it works and doesn't matter.
But I think relying on this fact looks a bit dangerous since it is not this function that throws an actual syntax error.
The name of this function is generic. So we may use this function for the other purpose.
Consider the following case,

if (!isDisallowedIdentifierAwait(xxx))
    fail();
// pass

In the above case, the first syntax checking succeeds, but the second actual parsing will fail.
I think using JSParserScriptMode is less tricky.
Comment 49 Caitlin Potter (:caitp) 2016-09-21 18:18:58 PDT
Created attachment 289499 [details]
Async Function Parsing v8
Comment 50 Caitlin Potter (:caitp) 2016-09-21 18:20:43 PDT
Comment on attachment 289499 [details]
Async Function Parsing v8

Updated with JSParserCommentMode renamed to JSParserScriptMode, m_commentMode renamed to m_scriptMode, and commentMode methods/variables renamed to scriptMode.
Comment 51 WebKit Commit Bot 2016-09-21 18:22:04 PDT
Attachment 289499 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 48 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 52 Caitlin Potter (:caitp) 2016-09-21 18:29:27 PDT
Created attachment 289501 [details]
Async Function Parsing v8 + fix ChangeLog
Comment 53 Caitlin Potter (:caitp) 2016-09-21 18:37:42 PDT
Created attachment 289502 [details]
Async Function Parsing v8 + fix ChangeLog + finish renaming variables/methods
Comment 54 Build Bot 2016-09-21 20:00:04 PDT
Comment on attachment 289502 [details]
Async Function Parsing v8 + fix ChangeLog + finish renaming variables/methods

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

New failing tests:
fast/images/object-data-url-case-insensitivity.html
Comment 55 Build Bot 2016-09-21 20:00:08 PDT
Created attachment 289508 [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 56 Yusuke Suzuki 2016-09-21 21:55:02 PDT
Comment on attachment 289502 [details]
Async Function Parsing v8 + fix ChangeLog + finish renaming variables/methods

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

> Source/JavaScriptCore/ChangeLog:87
> +        * runtime/CommonIdentifiers.h:

ChangeLog seems duplicated. Please drop the above one :)
Comment 57 Build Bot 2016-09-21 22:05:40 PDT
Comment on attachment 289502 [details]
Async Function Parsing v8 + fix ChangeLog + finish renaming variables/methods

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

New failing tests:
fast/images/move-image-to-new-document.html
Comment 58 Build Bot 2016-09-21 22:05:44 PDT
Created attachment 289519 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 59 Caitlin Potter (:caitp) 2016-09-23 06:29:08 PDT
Created attachment 289680 [details]
Async Function Parsing v8 + rebased and given another EWS try
Comment 60 Caitlin Potter (:caitp) 2016-09-23 13:57:53 PDT
As requested, a followup run benchmark:

```
                         Baseline                 WithPatch                                     

closure              0.51188+-0.02331          0.50381+-0.01643         might be 1.0160x faster
jquery               6.63200+-0.04502    ?     6.64141+-0.04629       ?

<geometric>          1.82306+-0.02974          1.81769+-0.02420         might be 1.0030x faster
```

The WithPatch VM is built with --asyncfunction-syntax, indicating that even with these changes, it's still essentially a non-issue for the parser.

New patch with fixed Source/JavaScriptCore/ChangeLog uploaded momentarily
Comment 61 Caitlin Potter (:caitp) 2016-09-23 13:59:20 PDT
Created attachment 289706 [details]
Async Function Parsing v8
Comment 62 Caitlin Potter (:caitp) 2016-09-23 14:11:11 PDT
Created attachment 289707 [details]
Async Function Parsing v8
Comment 63 Caitlin Potter (:caitp) 2016-09-23 14:34:02 PDT
Created attachment 289708 [details]
Async Function Parsing v8
Comment 64 Caitlin Potter (:caitp) 2016-09-23 14:51:36 PDT
Created attachment 289710 [details]
Async Function Parsing v8
Comment 65 Yusuke Suzuki 2016-09-23 14:54:21 PDT
Comment on attachment 289710 [details]
Async Function Parsing v8

r=me
Comment 66 WebKit Commit Bot 2016-09-23 15:26:45 PDT
Comment on attachment 289710 [details]
Async Function Parsing v8

Clearing flags on attachment: 289710

Committed r206333: <http://trac.webkit.org/changeset/206333>
Comment 67 WebKit Commit Bot 2016-09-23 15:26:53 PDT
All reviewed patches have been landed.  Closing bug.