Bug 193263 - builtins should be able to use async/await
Summary: builtins should be able to use async/await
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-08 15:52 PST by Keith Miller
Modified: 2019-01-09 09:42 PST (History)
7 users (show)

See Also:


Attachments
Patch (19.15 KB, patch)
2019-01-08 15:58 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (19.09 KB, patch)
2019-01-08 23:40 PST, Keith Miller
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-cq-02 for mac-sierra (1.05 MB, application/zip)
2019-01-09 00:21 PST, WebKit Commit Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2019-01-08 15:52:52 PST
builtins should be able to use async/await
Comment 1 Keith Miller 2019-01-08 15:58:05 PST
Created attachment 358645 [details]
Patch
Comment 2 Saam Barati 2019-01-08 18:48:22 PST
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.
Comment 3 Keith Miller 2019-01-08 23:39:10 PST
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.
Comment 4 Keith Miller 2019-01-08 23:40:22 PST
Created attachment 358676 [details]
Patch for landing
Comment 5 WebKit Commit Bot 2019-01-09 00:21:41 PST
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
Comment 6 WebKit Commit Bot 2019-01-09 00:21:42 PST
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
Comment 7 Keith Miller 2019-01-09 09:40:10 PST
Committed r239774: <https://trac.webkit.org/changeset/239774>
Comment 8 Radar WebKit Bug Importer 2019-01-09 09:42:04 PST
<rdar://problem/47148780>