[webkit-reviews] review granted: [Bug 193263] builtins should be able to use async/await : [Attachment 358645] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 193263: builtins should be able to use async/await
https://bugs.webkit.org/show_bug.cgi?id=193263

Attachment 358645: Patch

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




--- Comment #2 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 358645
  --> https://bugs.webkit.org/attachment.cgi?id=358645
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.