Bug 213317 - Promise built-in functions should be anonymous non-constructors
Summary: Promise built-in functions should be anonymous non-constructors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Trivial
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-17 14:46 PDT by Alexey Shvayka
Modified: 2020-06-18 11:25 PDT (History)
11 users (show)

See Also:


Attachments
Patch (11.58 KB, patch)
2020-06-17 15:15 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (11.88 KB, patch)
2020-06-18 07:56 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Shvayka 2020-06-17 14:46:25 PDT
Promise built-in functions should be anonymous non-constructors
Comment 1 Alexey Shvayka 2020-06-17 15:15:32 PDT
Created attachment 402158 [details]
Patch
Comment 2 Yusuke Suzuki 2020-06-17 15:47:21 PDT
Comment on attachment 402158 [details]
Patch

Can you ensure what bytecode is actually emitted for the rewritten functions and Promise/InternalPromise constructors? This part is super performance critical part. So we need to be extra careful.
Comment 3 Alexey Shvayka 2020-06-17 17:26:55 PDT
Comment on attachment 402158 [details]
Patch

Thank you for review, Darin!

I am denying commit queue to make sure that comma operator doesn't impact emitted bytecode & Promise microbenchmarks are neutral.
Comment 4 Alexey Shvayka 2020-06-18 07:56:23 PDT
Created attachment 402207 [details]
Patch

Add info on microbenchmarks and bytecode to ChangeLog.
Comment 5 Alexey Shvayka 2020-06-18 07:59:01 PDT
(In reply to Yusuke Suzuki from comment #2)
> Comment on attachment 402158 [details]
> Patch
> 
> Can you ensure what bytecode is actually emitted for the rewritten functions
> and Promise/InternalPromise constructors? This part is super performance
> critical part. So we need to be extra careful.

Promise constructors bytecode is unchanged (new_func_exp is emitted), while @createResolvingFunctions (used in slow path for `Promise.resolve(thenable)`) bytecode is reduced by 2 ops:

```
-createResolvingFunctions#BmeHaL:[0x10cfbc480->0x10cfe5600, NoneFunctionCall, 66 (StrictMode)]: 17 instructions (0 16-bit instructions, 0 32-bit instructions, 5 instructions with metadata); 186 bytes (120 metadata bytes); 2 parameter(s); 12 callee register(s); 10 variable(s); scope at loc4
+createResolvingFunctions#DBfCeH:[0x1035bc480->0x1035e5600, NoneFunctionCall, 60 (StrictMode)]: 15 instructions (0 16-bit instructions, 0 32-bit instructions, 5 instructions with metadata); 180 bytes (120 metadata bytes); 2 parameter(s); 12 callee register(s); 10 variable(s); scope at loc4
 [   0] enter              
 [   1] get_scope          loc4
 [   3] mov                loc5, loc4
 [   6] check_traps        
 [   7] mov                loc6, callee
-[  10] create_lexical_environment loc7, loc4, Cell: 0x10cfb80c0 (0x10cff8460:[0x383, SymbolTable, {}, NonArray, Leaf]), StructureID: 899(const0), Undefined(const1)
+[  10] create_lexical_environment loc7, loc4, Cell: 0x1035b80c0 (0x1035f8460:[0x450a, SymbolTable, {}, NonArray, Leaf]), StructureID: 17674(const0), Undefined(const1)
 [  15] mov                loc4, loc7
 [  18] put_to_scope       loc7, 0, arg1, 1073743876<ThrowIfNotFound|LocalClosureVar|NotInitialization|StrictMode>, 0, 0
-[  26] new_func           loc10, loc4, 0
-[  30] mov                loc8, loc10
-[  33] new_func           loc10, loc4, 1
-[  37] mov                loc9, loc10
-[  40] put_to_scope       loc7, 1, False(const2), 1073741828<ThrowIfNotFound|LocalClosureVar|Initialization|StrictMode>, 0, 1
-[  48] new_object         loc10, 2
-[  52] put_by_id          loc10, 2, loc8, IsDirect|Strict
-[  58] put_by_id          loc10, 3, loc9, IsDirect|Strict
-[  64] ret                loc10
+[  26] put_to_scope       loc7, 1, False(const2), 1073741828<ThrowIfNotFound|LocalClosureVar|Initialization|StrictMode>, 0, 1
+[  34] new_func_exp       loc9, loc4, 0
+[  38] new_func_exp       loc8, loc4, 1
+[  42] new_object         loc10, 2
+[  46] put_by_id          loc10, 2, loc9, IsDirect|Strict
+[  52] put_by_id          loc10, 3, loc8, IsDirect|Strict
+[  58] ret                loc10
 
 Identifiers:
   id0 = promise
   id1 = alreadyResolved
   id2 = PrivateSymbol.resolve
   id3 = PrivateSymbol.reject
 
 Constants:
-   k0 = Cell: 0x10cfb80c0 (0x10cff8460:[0x383, SymbolTable, {}, NonArray, Leaf]), StructureID: 899
+   k0 = Cell: 0x1035b80c0 (0x1035f8460:[0x450a, SymbolTable, {}, NonArray, Leaf]), StructureID: 17674
    k1 = Undefined
    k2 = False
```

Promise microbenchmarks are neutral.
Comment 6 Yusuke Suzuki 2020-06-18 11:09:42 PDT
Comment on attachment 402207 [details]
Patch

Looks good!
Comment 7 EWS 2020-06-18 11:22:19 PDT
Committed r263222: <https://trac.webkit.org/changeset/263222>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402207 [details].
Comment 8 Radar WebKit Bug Importer 2020-06-18 11:23:15 PDT
<rdar://problem/64497346>