Bug 161409 – [JSC] Implement parsing of Async Functions

• Component: New Bugs
• Status: RESOLVED FIXED

[Caitlin Potter (:caitp)]

[JSC] Implement parsing of Async Functions

[Caitlin Potter (:caitp)]

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)

[Caitlin Potter (:caitp)]

Created attachment 287437 [details]
Patch

[WebKit Commit Bot]

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.

[Caitlin Potter (:caitp)]

Created attachment 287442 [details]
Patch

Get rid of the xcodeproj changes, since the new files aren't included in this patch.

[Caitlin Potter (:caitp)]

Created attachment 287444 [details]
Async Function Parsing v1

Fix modules test failures

[Caitlin Potter (:caitp)]

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.

[Yusuke Suzuki]

(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?

[Caitlin Potter (:caitp)]

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

[Caitlin Potter (:caitp)]

Created attachment 288192 [details]
Async Function Parsing v2

Patch to try to reduce codeload regression a bit

[Caitlin Potter (:caitp)]

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

[Yusuke Suzuki]

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

[Caitlin Potter (:caitp)]

That still sounds pretty bad, if not worse than it was originally measured to be. I think I will keep looking for improvements.

[Michael Catanzaro]

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?

[Caitlin Potter (:caitp)]

Created attachment 288384 [details]
Patch

[Yusuke Suzuki]

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.

[Caitlin Potter (:caitp)]

Do you have any ideas what might be the result of the poor performance on the linux runs?

[Caitlin Potter (:caitp)]

er, the cause of the poor linux perf.

[Yusuke Suzuki]

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

[Yusuke Suzuki]

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.

[Caitlin Potter (:caitp)]

(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?

[Caitlin Potter (:caitp)]

Created attachment 288477 [details]
Async Function Parsing v3

[Caitlin Potter (:caitp)]

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

[Yusuke Suzuki]

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

[Caitlin Potter (:caitp)]

Created attachment 288528 [details]
Async Function Parsing v4

[Caitlin Potter (:caitp)]

Comment on attachment 288528 [details]
Async Function Parsing v4

Added new version with Options::useAsyncFunctions() to hide the feature

[Caitlin Potter (:caitp)]

with the flag, I am again seeing pretty poor results on the benchmarks.

[Caitlin Potter (:caitp)]

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

[Yusuke Suzuki]

(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!

[Caitlin Potter (:caitp)]

I'll try to do that next week... I guess all I can do for the test is add a //@ skip line?

[Yusuke Suzuki]

(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!

[Caitlin Potter (:caitp)]

Created attachment 289286 [details]
Patch

[Caitlin Potter (:caitp)]

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.

[Caitlin Potter (:caitp)]

Created attachment 289289 [details]
Async Function Parsing v5

[Yusuke Suzuki]

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()`?

[Caitlin Potter (:caitp)]

Created attachment 289446 [details]
Async Function Parsing v6

[Caitlin Potter (:caitp)]

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)

[Caitlin Potter (:caitp)]

Created attachment 289447 [details]
Async Function Parsing v6 + Copy-edit the build-jsc help message changes

[Build Bot]

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

[Build Bot]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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.

[Darin Adler]

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.

[Caitlin Potter (:caitp)]

Created attachment 289488 [details]
Patch

[Caitlin Potter (:caitp)]

Created attachment 289489 [details]
Async Function Parsing v7

[Darin Adler]

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

[Caitlin Potter (:caitp)]

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.

[Yusuke Suzuki]

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!

[Yusuke Suzuki]

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.

[Caitlin Potter (:caitp)]

Created attachment 289499 [details]
Async Function Parsing v8

[Caitlin Potter (:caitp)]

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.

[WebKit Commit Bot]

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.

[Caitlin Potter (:caitp)]

Created attachment 289501 [details]
Async Function Parsing v8 + fix ChangeLog

[Caitlin Potter (:caitp)]

Created attachment 289502 [details]
Async Function Parsing v8 + fix ChangeLog + finish renaming variables/methods

[Build Bot]

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

[Build Bot]

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

[Yusuke Suzuki]

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 :)

[Build Bot]

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

[Build Bot]

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

[Caitlin Potter (:caitp)]

Created attachment 289680 [details]
Async Function Parsing v8 + rebased and given another EWS try

[Caitlin Potter (:caitp)]

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

[Caitlin Potter (:caitp)]

Created attachment 289706 [details]
Async Function Parsing v8

[Caitlin Potter (:caitp)]

Created attachment 289707 [details]
Async Function Parsing v8

[Caitlin Potter (:caitp)]

Created attachment 289708 [details]
Async Function Parsing v8

[Caitlin Potter (:caitp)]

Created attachment 289710 [details]
Async Function Parsing v8

[Yusuke Suzuki]

Comment on attachment 289710 [details]
Async Function Parsing v8

r=me

[WebKit Commit Bot]

Comment on attachment 289710 [details]
Async Function Parsing v8

Clearing flags on attachment: 289710

Committed r206333: <http://trac.webkit.org/changeset/206333>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.