Bug 193263 – builtins should be able to use async/await

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

builtins should be able to use async/await

[Keith Miller]

Created attachment 358645 [details]
Patch

[Saam Barati]

Comment on attachment 358645 [details]
Patch

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

r=me but your changelog makes me feel like we should measure the perf impact.

> Source/JavaScriptCore/ChangeLog:10
> +        It's not clear if this substantially regresses the performance of our module loader
> +        code though.

Can we determine if it does? Is it worth landing regardless?

> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:88
> +    // FIXME: Can we just make MetaData computation be constexpr and have the compiler do this for us?

This would be super nice. You should file a bug.

> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:-105
> -    RELEASE_ASSERT(view.length() >= 15); // strlen("(function (){})") == 15
> -    RELEASE_ASSERT(characters[0] ==  '(');
> -    RELEASE_ASSERT(characters[1] ==  'f');
> -    RELEASE_ASSERT(characters[2] ==  'u');
> -    RELEASE_ASSERT(characters[3] ==  'n');
> -    RELEASE_ASSERT(characters[4] ==  'c');
> -    RELEASE_ASSERT(characters[5] ==  't');
> -    RELEASE_ASSERT(characters[6] ==  'i');
> -    RELEASE_ASSERT(characters[7] ==  'o');
> -    RELEASE_ASSERT(characters[8] ==  'n');
> -    RELEASE_ASSERT(characters[9] ==  ' ');
> -    RELEASE_ASSERT(characters[10] == '(');

I'm Sorry :(

> Source/JavaScriptCore/builtins/ModuleLoader.js:341
> +    let key = await this.resolve(moduleName, @undefined, fetcher);
> +    let entry = await this.requestSatisfy(this.ensureRegistered(key), parameters, fetcher, new @Set);
> +    return entry.key;

very nice

> Source/JavaScriptCore/parser/Parser.cpp:282
> +            // FIXME: We allow async to leak because it appearing as a closed variable is a side effect of trying to parse async arrow functions.

I would also file a bug here or just remove the FIXME. This seems unlikely something we'll ever implement.

[Keith Miller]

Comment on attachment 358645 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:10
>> +        code though.
> 
> Can we determine if it does? Is it worth landing regardless?

My very scientific experiments running our module loading tests before and after this change doesn't appear to significantly impact the end to end time.

>> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:88
>> +    // FIXME: Can we just make MetaData computation be constexpr and have the compiler do this for us?
> 
> This would be super nice. You should file a bug.

Done.

>> Source/JavaScriptCore/parser/Parser.cpp:282
>> +            // FIXME: We allow async to leak because it appearing as a closed variable is a side effect of trying to parse async arrow functions.
> 
> I would also file a bug here or just remove the FIXME. This seems unlikely something we'll ever implement.

I'll just remove the FIXME part but leave the comment.

[Keith Miller]

Created attachment 358676 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 358676 [details]
Patch for landing

Rejecting attachment 358676 [details] from commit-queue.

Number of test failures exceeded the failure limit.
Full output: https://webkit-queues.webkit.org/results/10681459

[WebKit Commit Bot]

Created attachment 358680 [details]
Archive of layout-test-results from webkit-cq-02 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-02  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Keith Miller]

Committed r239774: <https://trac.webkit.org/changeset/239774>

[Radar WebKit Bug Importer]

<rdar://problem/47148780>