Summary: | Promise built-in functions should be anonymous non-constructors | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Shvayka <ashvayka> | ||||||
Component: | JavaScriptCore | Assignee: | Alexey Shvayka <ashvayka> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Trivial | CC: | darin, ews-watchlist, joepeck, keith_miller, mark.lam, msaboff, ross.kirsling, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=204214 | ||||||||
Attachments: |
|
Description
Alexey Shvayka
2020-06-17 14:46:25 PDT
Created attachment 402158 [details]
Patch
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 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.
Created attachment 402207 [details]
Patch
Add info on microbenchmarks and bytecode to ChangeLog.
(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 on attachment 402207 [details]
Patch
Looks good!
Committed r263222: <https://trac.webkit.org/changeset/263222> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402207 [details]. |