Bug 200898

Summary: [JSC] Make Promise implementation faster
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, calvaris, erights, ews-watchlist, joepeck, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews213 for win-future
none
Patch
none
Patch
none
Archive of layout-test-results from ews212 for win-future
none
Patch
none
Archive of layout-test-results from ews214 for win-future
none
Patch
none
Archive of layout-test-results from ews215 for win-future
none
Patch
none
Archive of layout-test-results from ews215 for win-future
none
Patch
none
Archive of layout-test-results from ews215 for win-future
none
Patch
none
Patch
none
Archive of layout-test-results from ews215 for win-future
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
saam: review+
Patch for landing none

Description Yusuke Suzuki 2019-08-19 14:10:18 PDT
The sampling-profiler result on async-fs is telling that Promise implementation needs to be optimized.

Starting JetStream2
Running async-fs:
    Startup: 78.125
    Worst Case: 129.310
    Average: 139.485
    Score: 112.112
    Wall time: 0:01.464


First: 78.125
Worst: 129.310
Average: 139.485

Total Score:  112.112



Sampling rate: 1000.000000 microseconds
Top functions as <numSamples  'functionName:sourceID'>
   175    'randomFileContents:3'
   168    ':3'
    53    'Promise:-1'
    39    'createResolvingFunctions:2'
    38    'newPromiseCapability:2'
    13    ':-1'
    12    'initializePromise:2'
     9    'ArrayBuffer:-1'
     7    'newPromiseReaction:2'
     6    'triggerPromiseReactions:2'
     5    'asyncFunctionResume:2'
     5    'mapIteratorNext:2'


Sampling rate: 1000.000000 microseconds
Hottest bytecodes as <numSamples   'functionName#hash:JITType:bytecodeIndex'>
    71    'randomFileContents#CYc0UW:FTL:184'
    53    'Promise#<nil>:None:<nil>'
    37    'randomFileContents#CYc0UW:FTL:172'
    29    'randomFileContents#CYc0UW:FTL:178'
    21    '#EZ2S1s:FTL:296'
    20    'newPromiseCapability#DzQl2X:FTL:18'
    19    '#EZ2S1s:FTL:428'
    14    'createResolvingFunctions#BKmDIq:Baseline:33 <-- initializePromise#Aw6QQA:FTL:64'
    13    'createResolvingFunctions#BKmDIq:Baseline:26 <-- initializePromise#Aw6QQA:FTL:64'
    13    '#<nil>:None:<nil>'
    12    '#EZ2S1s:FTL:0'
    11    'randomFileContents#CYc0UW:FTL:198'
    11    '#EZ2S1s:FTL:230'
    10    'randomFileContents#CYc0UW:FTL:191'
     9    '#EZ2S1s:FTL:395'
     9    'randomFileContents#CYc0UW:FTL:196'
     9    'ArrayBuffer#<nil>:None:<nil>'
     9    'newPromiseCapability#DzQl2X:FTL:55'
     8    '#EZ2S1s:FTL:128'
     7    '#EZ2S1s:FTL:200'
     7    '#EZ2S1s:FTL:423'
     7    'newPromiseReaction#AB1rHy:Baseline:10 <-- then#AzYm2S:FTL:153'
     6    'createResolvingFunctions#BKmDIq:Baseline:10 <-- initializePromise#Aw6QQA:FTL:64'
     6    '#EZ2S1s:FTL:434'
     6    '#EZ2S1s:FTL:322'
     6    '#EZ2S1s:FTL:61'
     6    '#EZ2S1s:FTL:195'
     5    '#EZ2S1s:FTL:256'
     5    '#EZ2S1s:FTL:261'
     5    'newPromiseCapability#DzQl2X:FTL:10'
     5    'initializePromise#Aw6QQA:FTL:28'
     5    'randomFileContents#CYc0UW:FTL:141'
     5    'triggerPromiseReactions#DKTS58:Baseline:13 <-- resolve#DW5aeG:FTL:168'
     4    'initializePromise#Aw6QQA:FTL:86'
     4    '#EZ2S1s:FTL:6'
     3    'then#AzYm2S:FTL:178'
     3    '#EZ2S1s:FTL:50'
     3    '#ELynXq:DFG:72'
     3    'resolve#DW5aeG:FTL:346'
     3    'asyncGeneratorYield#AcNyDY:Baseline:26 <-- doAsyncGeneratorBodyCall#BqviGL:FTL:367'
Comment 1 Yusuke Suzuki 2019-08-20 23:24:59 PDT
Created attachment 376847 [details]
Patch
Comment 2 Yusuke Suzuki 2019-08-20 23:31:31 PDT
The key is avoiding creating user-unobservable promise objects etc. async-await etc. creates bunch of promises which are never shown to users. If we carefully handle it, we can avoid creating them, and we can get faster performance.
Comment 3 Yusuke Suzuki 2019-08-20 23:33:28 PDT
Created attachment 376849 [details]
Patch
Comment 4 EWS Watchlist 2019-08-21 01:49:36 PDT
Comment on attachment 376849 [details]
Patch

Attachment 376849 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12950540

New failing tests:
wasm.yaml/wasm/js-api/test_basic_api.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/memory-alignment.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/table-basic-2.js.wasm-collect-continuously
wasm.yaml/wasm/js-api/element-data.js.wasm-collect-continuously
wasm.yaml/wasm/js-api/extension-MemoryMode.js.wasm-collect-continuously
wasm.yaml/wasm/self-test/test_LowLevelBinary_varuint1.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/br-as-return.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/many-arguments-to-function.js.wasm-collect-continuously
wasm.yaml/wasm/js-api/test_memory.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/brTableManyValues.js.wasm-collect-continuously
wasm.yaml/wasm/references/anyref_modules.js.wasm-collect-continuously
wasm.yaml/wasm/js-api/element.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/function-import-return-value.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/loop-sum.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/rotr.js.wasm-collect-continuously
wasm.yaml/wasm/js-api/web-assembly-instantiate.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/memory-import-and-grow.js.wasm-collect-continuously
wasm.yaml/wasm/self-test/test_LowLevelBinary_uint8.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/load-offset.js.wasm-collect-continuously
wasm.yaml/wasm/js-api/table.js.wasm-collect-continuously
Comment 5 EWS Watchlist 2019-08-21 04:44:04 PDT
Comment on attachment 376849 [details]
Patch

Attachment 376849 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12950983

Number of test failures exceeded the failure limit.
Comment 6 EWS Watchlist 2019-08-21 04:44:15 PDT
Created attachment 376862 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 7 Radar WebKit Bug Importer 2019-08-21 13:34:18 PDT
<rdar://problem/54569373>
Comment 8 Yusuke Suzuki 2019-08-21 13:36:21 PDT
Created attachment 376912 [details]
Patch
Comment 9 Yusuke Suzuki 2019-08-21 13:40:29 PDT
Created attachment 376913 [details]
Patch
Comment 10 EWS Watchlist 2019-08-21 15:38:59 PDT
Comment on attachment 376913 [details]
Patch

Attachment 376913 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12953423

Number of test failures exceeded the failure limit.
Comment 11 EWS Watchlist 2019-08-21 15:39:01 PDT
Created attachment 376936 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 12 EWS Watchlist 2019-08-21 15:58:49 PDT
Comment on attachment 376913 [details]
Patch

Attachment 376913 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12953273

New failing tests:
wasm.yaml/wasm/function-tests/exceptions.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/function-import-return-value.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/double-instance.js.wasm-collect-continuously
wasm.yaml/wasm/js-api/test_memory.js.wasm-collect-continuously
wasm.yaml/wasm/js-api/Module.customSection.js.wasm-collect-continuously
wasm.yaml/wasm/spec-tests/memory.wast.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/trap-store-2.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/memory-section-and-import.js.wasm-collect-continuously
wasm.yaml/wasm/js-api/test_basic_api.js.wasm-collect-continuously
wasm.yaml/wasm/js-api/wasm-to-wasm.js.wasm-collect-continuously
wasm.yaml/wasm/self-test/test_LowLevelBinary_varuint1.js.wasm-collect-continuously
wasm.yaml/wasm/references/anyref_modules.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/br-if-loop-less-than.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/loop-sum.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/trap-load.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/i32-load.js.wasm-collect-continuously
wasm.yaml/wasm/self-test/test_LowLevelBinary_uint24.js.wasm-collect-continuously
wasm.yaml/wasm/js-api/wrapper-function.js.wasm-collect-continuously
wasm.yaml/wasm/stress/osr-entry-many-stacks-f64.js.wasm-collect-continuously
wasm.yaml/wasm/js-api/element.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/brTableAsIf.js.wasm-collect-continuously
wasm.yaml/wasm/spec-tests/const.wast.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/grow-memory-cause-gc.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/ret5.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/invalid-duplicate-export.js.wasm-collect-continuously
wasm.yaml/wasm/js-api/Module.exports.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/table-basic.js.wasm-collect-continuously
wasm.yaml/wasm/js-api/call-indirect.js.wasm-collect-continuously
wasm.yaml/wasm/js-api/web-assembly-instantiate-parallel.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/table-basic-2.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/trap-after-cross-instance-call.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/memory-import-and-grow.js.wasm-collect-continuously
wasm.yaml/wasm/function-tests/stack-trace.js.wasm-collect-continuously
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases
wasm.yaml/wasm/js-api/test_Instance.js.wasm-collect-continuously
Comment 13 Yusuke Suzuki 2019-08-21 17:20:58 PDT
Created attachment 376953 [details]
Patch
Comment 14 EWS Watchlist 2019-08-21 20:00:43 PDT
Comment on attachment 376953 [details]
Patch

Attachment 376953 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12954568

Number of test failures exceeded the failure limit.
Comment 15 EWS Watchlist 2019-08-21 20:00:50 PDT
Created attachment 376969 [details]
Archive of layout-test-results from ews214 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 16 Yusuke Suzuki 2019-08-21 20:19:52 PDT
Created attachment 376971 [details]
Patch
Comment 17 EWS Watchlist 2019-08-21 22:27:00 PDT
Comment on attachment 376971 [details]
Patch

Attachment 376971 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12955159

Number of test failures exceeded the failure limit.
Comment 18 EWS Watchlist 2019-08-21 22:27:02 PDT
Created attachment 376988 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 19 Yusuke Suzuki 2019-08-22 01:03:37 PDT
Created attachment 376994 [details]
Patch
Comment 20 EWS Watchlist 2019-08-22 01:05:37 PDT
Attachment 376994 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1473:  Wrong number of spaces before statement. (expected: 32)  [whitespace/indent] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 75 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 EWS Watchlist 2019-08-22 03:03:51 PDT
Comment on attachment 376994 [details]
Patch

Attachment 376994 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12955854

Number of test failures exceeded the failure limit.
Comment 22 EWS Watchlist 2019-08-22 03:03:53 PDT
Created attachment 377001 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 23 EWS Watchlist 2019-08-22 03:24:31 PDT
Comment on attachment 376994 [details]
Patch

Attachment 376994 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12955841

New failing tests:
stress/async-iteration-for-await-of.js.ftl-eager
stress/async-iteration-async-from-sync.js.bytecode-cache
stress/async-iteration-for-await-of.js.dfg-eager
stress/async-iteration-async-from-sync.js.default
stress/async-iteration-for-await-of.js.no-ftl
stress/async-iteration-for-await-of.js.default
stress/async-iteration-for-await-of.js.ftl-no-cjit-small-pool
stress/async-iteration-for-await-of.js.dfg-eager-no-cjit-validate
stress/async-iteration-async-from-sync.js.no-llint
stress/async-iteration-for-await-of.js.ftl-no-cjit-b3o0
stress/async-iteration-async-from-sync.js.mini-mode
stress/async-iteration-async-from-sync.js.dfg-maximal-flush-validate-no-cjit
stress/async-iteration-async-from-sync.js.ftl-eager-no-cjit-b3o1
stress/async-iteration-async-from-sync.js.ftl-no-cjit-no-inline-validate
stress/async-iteration-for-await-of.js.ftl-no-cjit-validate-sampling-profiler
stress/async-iteration-for-await-of.js.ftl-eager-no-cjit
stress/async-iteration-for-await-of.js.mini-mode
stress/async-iteration-async-from-sync.js.ftl-no-cjit-validate-sampling-profiler
stress/async-iteration-async-from-sync.js.no-cjit-validate-phases
stress/async-iteration-for-await-of.js.no-cjit-collect-continuously
stress/async-iteration-for-await-of.js.bytecode-cache
stress/async-iteration-for-await-of.js.ftl-no-cjit-no-put-stack-validate
stress/async-iteration-for-await-of.js.ftl-no-cjit-no-inline-validate
stress/async-iteration-async-from-sync.js.dfg-eager
stress/async-iteration-async-from-sync.js.ftl-no-cjit-no-put-stack-validate
stress/async-iteration-for-await-of.js.dfg-maximal-flush-validate-no-cjit
stress/async-iteration-async-from-sync.js.ftl-eager
stress/async-iteration-async-from-sync.js.ftl-eager-no-cjit
stress/async-iteration-for-await-of.js.ftl-eager-no-cjit-b3o1
stress/async-iteration-async-from-sync.js.no-cjit-collect-continuously
stress/async-iteration-async-from-sync.js.ftl-no-cjit-b3o0
stress/async-iteration-async-from-sync.js.dfg-eager-no-cjit-validate
stress/async-iteration-for-await-of.js.no-llint
stress/async-iteration-async-from-sync.js.no-ftl
stress/async-iteration-async-from-sync.js.ftl-no-cjit-small-pool
stress/async-iteration-for-await-of.js.no-cjit-validate-phases
Comment 24 Yusuke Suzuki 2019-08-22 17:57:33 PDT
Created attachment 377084 [details]
Patch
Comment 25 EWS Watchlist 2019-08-22 18:05:08 PDT
Attachment 377084 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSPromise.h:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1473:  Wrong number of spaces before statement. (expected: 32)  [whitespace/indent] [4]
Total errors found: 2 in 75 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 EWS Watchlist 2019-08-22 20:17:43 PDT
Comment on attachment 377084 [details]
Patch

Attachment 377084 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12959090

Number of test failures exceeded the failure limit.
Comment 27 EWS Watchlist 2019-08-22 20:17:46 PDT
Created attachment 377094 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 28 Yusuke Suzuki 2019-08-23 21:35:54 PDT
Created attachment 377202 [details]
Patch
Comment 29 EWS Watchlist 2019-08-23 21:37:43 PDT
Attachment 377202 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1469:  Wrong number of spaces before statement. (expected: 32)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:148:  multiple statements on one line (semicolon)  [pep8/E702] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:149:  multiple statements on one line (semicolon)  [pep8/E702] [5]
Total errors found: 3 in 94 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 30 Yusuke Suzuki 2019-08-23 21:44:19 PDT
Created attachment 377203 [details]
Patch
Comment 31 EWS Watchlist 2019-08-23 21:47:26 PDT
Attachment 377203 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1469:  Wrong number of spaces before statement. (expected: 32)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:148:  multiple statements on one line (semicolon)  [pep8/E702] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:149:  multiple statements on one line (semicolon)  [pep8/E702] [5]
Total errors found: 3 in 106 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 32 EWS Watchlist 2019-08-23 23:56:42 PDT
Comment on attachment 377203 [details]
Patch

Attachment 377203 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12964052

Number of test failures exceeded the failure limit.
Comment 33 EWS Watchlist 2019-08-23 23:56:46 PDT
Created attachment 377205 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 34 EWS Watchlist 2019-08-24 00:00:39 PDT
Comment on attachment 377203 [details]
Patch

Attachment 377203 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12964035

New failing tests:
stress/promise-cannot-be-called.js.ftl-no-cjit-no-put-stack-validate
stress/promise-cannot-be-called.js.default
stress/promise-cannot-be-called.js.ftl-no-cjit-validate-sampling-profiler
stress/promise-cannot-be-called.js.no-cjit-validate-phases
jsc-layout-tests.yaml/js/script-tests/Promise-types.js.layout-no-cjit
stress/promise-cannot-be-called.js.ftl-eager-no-cjit
stress/promise-cannot-be-called.js.no-ftl
stress/promise-cannot-be-called.js.ftl-no-cjit-no-inline-validate
stress/promise-cannot-be-called.js.bytecode-cache
stress/promise-cannot-be-called.js.dfg-eager
jsc-layout-tests.yaml/js/script-tests/Promise-types.js.layout-ftl-eager-no-cjit
stress/promise-cannot-be-called.js.mini-mode
jsc-layout-tests.yaml/js/script-tests/Promise-types.js.layout-no-ftl
stress/promise-cannot-be-called.js.dfg-eager-no-cjit-validate
jsc-layout-tests.yaml/js/script-tests/Promise-types.js.layout
stress/promise-cannot-be-called.js.no-cjit-collect-continuously
stress/promise-cannot-be-called.js.no-llint
stress/promise-cannot-be-called.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/Promise-types.js.layout-dfg-eager-no-cjit
stress/promise-cannot-be-called.js.ftl-no-cjit-b3o0
jsc-layout-tests.yaml/js/script-tests/Promise-types.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/Promise-types.js.layout-no-llint
stress/promise-cannot-be-called.js.dfg-maximal-flush-validate-no-cjit
stress/promise-cannot-be-called.js.ftl-eager
stress/promise-cannot-be-called.js.ftl-no-cjit-small-pool
Comment 35 Yusuke Suzuki 2019-08-24 00:12:09 PDT
Created attachment 377206 [details]
Patch
Comment 36 EWS Watchlist 2019-08-24 00:14:02 PDT
Attachment 377206 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1469:  Wrong number of spaces before statement. (expected: 32)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:148:  multiple statements on one line (semicolon)  [pep8/E702] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:149:  multiple statements on one line (semicolon)  [pep8/E702] [5]
Total errors found: 3 in 113 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 37 Yusuke Suzuki 2019-08-24 01:01:30 PDT
Created attachment 377207 [details]
Patch
Comment 38 EWS Watchlist 2019-08-24 01:04:00 PDT
Attachment 377207 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1469:  Wrong number of spaces before statement. (expected: 32)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:148:  multiple statements on one line (semicolon)  [pep8/E702] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:149:  multiple statements on one line (semicolon)  [pep8/E702] [5]
Total errors found: 3 in 116 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 39 Yusuke Suzuki 2019-08-24 02:07:04 PDT
Created attachment 377208 [details]
Patch
Comment 40 Yusuke Suzuki 2019-08-24 02:08:54 PDT
Created attachment 377209 [details]
Patch
Comment 41 EWS Watchlist 2019-08-24 02:12:30 PDT
Attachment 377209 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1469:  Wrong number of spaces before statement. (expected: 32)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:148:  multiple statements on one line (semicolon)  [pep8/E702] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:149:  multiple statements on one line (semicolon)  [pep8/E702] [5]
Total errors found: 3 in 116 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 42 Yusuke Suzuki 2019-08-24 17:17:08 PDT
Created attachment 377215 [details]
Patch
Comment 43 EWS Watchlist 2019-08-24 17:20:40 PDT
Attachment 377215 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1469:  Wrong number of spaces before statement. (expected: 32)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:148:  multiple statements on one line (semicolon)  [pep8/E702] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:149:  multiple statements on one line (semicolon)  [pep8/E702] [5]
Total errors found: 3 in 117 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 44 Yusuke Suzuki 2019-08-24 17:33:35 PDT
Created attachment 377216 [details]
Patch
Comment 45 EWS Watchlist 2019-08-24 17:35:26 PDT
Attachment 377216 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1469:  Wrong number of spaces before statement. (expected: 32)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:148:  multiple statements on one line (semicolon)  [pep8/E702] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:149:  multiple statements on one line (semicolon)  [pep8/E702] [5]
Total errors found: 3 in 117 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 46 Yusuke Suzuki 2019-08-26 00:00:26 PDT
Created attachment 377229 [details]
Patch
Comment 47 EWS Watchlist 2019-08-26 00:05:58 PDT
Attachment 377229 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1469:  Wrong number of spaces before statement. (expected: 32)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:148:  multiple statements on one line (semicolon)  [pep8/E702] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:149:  multiple statements on one line (semicolon)  [pep8/E702] [5]
Total errors found: 3 in 118 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 48 Yusuke Suzuki 2019-08-26 16:47:41 PDT
Created attachment 377289 [details]
Patch
Comment 49 EWS Watchlist 2019-08-26 16:50:38 PDT
Attachment 377289 [details] did not pass style-queue:


ERROR: JSTests/stress/new-promise-should-respect-promise-realm.js:12:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/stress/new-promise-should-respect-promise-realm.js:17:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/stress/new-promise-should-respect-promise-realm.js:18:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/stress/new-promise-should-respect-promise-realm.js:19:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1469:  Wrong number of spaces before statement. (expected: 32)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:148:  multiple statements on one line (semicolon)  [pep8/E702] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:149:  multiple statements on one line (semicolon)  [pep8/E702] [5]
ERROR: JSTests/stress/create-promise-should-respect-promise-realm.js:11:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/stress/create-promise-should-respect-promise-realm.js:12:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/stress/create-promise-should-respect-promise-realm.js:13:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/stress/create-promise-should-respect-promise-realm.js:18:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/stress/create-promise-should-respect-promise-realm.js:19:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/stress/create-promise-should-respect-promise-realm.js:20:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/stress/create-promise-should-respect-promise-realm.js:25:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/stress/create-promise-should-respect-promise-realm.js:30:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/stress/create-promise-should-respect-promise-realm.js:31:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/stress/create-promise-should-respect-promise-realm.js:32:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 17 in 128 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 50 Yusuke Suzuki 2019-08-26 17:07:10 PDT
Created attachment 377292 [details]
Patch
Comment 51 EWS Watchlist 2019-08-26 17:11:28 PDT
Attachment 377292 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1469:  Wrong number of spaces before statement. (expected: 32)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:148:  multiple statements on one line (semicolon)  [pep8/E702] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:149:  multiple statements on one line (semicolon)  [pep8/E702] [5]
Total errors found: 3 in 128 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 52 Yusuke Suzuki 2019-08-26 18:25:19 PDT
Created attachment 377305 [details]
Patch
Comment 53 EWS Watchlist 2019-08-26 18:27:56 PDT
Attachment 377305 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1469:  Wrong number of spaces before statement. (expected: 32)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:148:  multiple statements on one line (semicolon)  [pep8/E702] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:149:  multiple statements on one line (semicolon)  [pep8/E702] [5]
Total errors found: 3 in 129 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 54 Yusuke Suzuki 2019-08-26 20:48:57 PDT
Created attachment 377313 [details]
Patch
Comment 55 EWS Watchlist 2019-08-26 20:51:17 PDT
Attachment 377313 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1469:  Wrong number of spaces before statement. (expected: 32)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:148:  multiple statements on one line (semicolon)  [pep8/E702] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:149:  multiple statements on one line (semicolon)  [pep8/E702] [5]
Total errors found: 3 in 132 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 56 Yusuke Suzuki 2019-08-27 16:52:00 PDT
To slightly make this patch easy, I'll introduce @resolvePromiseWithFirstResolvingFunctionCallCheck and @rejectPromiseWithFirstResolvingFunctionCallCheck. They use special flag in JSPromise to make functions like resolving-function without creating a new environment.
Comment 57 Yusuke Suzuki 2019-08-27 17:21:28 PDT
Created attachment 377406 [details]
Patch
Comment 58 Yusuke Suzuki 2019-08-27 17:37:45 PDT
Created attachment 377409 [details]
Patch
Comment 59 Mark Lam 2019-08-27 21:04:59 PDT
The failed inspector/console/message-stack-trace.html test from the mac-wk2 bot seems legit.
Comment 60 Yusuke Suzuki 2019-08-27 21:20:29 PDT
Created attachment 377424 [details]
Patch
Comment 61 Yusuke Suzuki 2019-08-27 21:21:42 PDT
(In reply to Mark Lam from comment #59)
> The failed inspector/console/message-stack-trace.html test from the mac-wk2
> bot seems legit.

This is because the name of internal promise operations are changed.
This test relies on this internal builtin function's names since it is testing stack trace.
I've rebaselined the expected file in LayoutTest.
Comment 62 Mark Lam 2019-08-27 21:45:34 PDT
Comment on attachment 377424 [details]
Patch

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

I can't do a full review right now, but here are some comments and questions about the ChangeLog.

> Source/JavaScriptCore/ChangeLog:35
> +            do not emit `op_create_this` implicitly and the costructor does not return `this` object implicitly. The naked constructor allows

/costructor/constructor/

> Source/JavaScriptCore/ChangeLog:39
> +            so that we can start introducing object-allocation-sinking for JSPromise too. It is filed in [3].

Did you mean /so that we can/With this, we can/?

> Source/JavaScriptCore/ChangeLog:54
> +            Some of promises are never shown to users, and they are never rejected. One example is `await`'s promise. And some of promise creation can be avoided.

/Some of promises/Some promises/

> Source/JavaScriptCore/ChangeLog:59
> +            check to take a fast path. And promise reaction now handles 4 cases to handle the case not creating a promise.

Can you rephrase this last sentence to be clearer?  I'm having a hard time understanding what it's trying to say, especially this part: "handles 4 cases to handle ... case".

> Source/JavaScriptCore/ChangeLog:63
> +            Resolving functions have `alreadyResolved` flag not to allow calling `resolve` and `reject` multiple times. For the first resolving function creation, this

/flag not to allow/flag to prevent/?

> Source/JavaScriptCore/ChangeLog:65
> +            created again for the same promise. In that case, we just create an usual resolving functions). By doing so, we avoid unnecessary resolving functions

/an usual/a usual/?
Comment 63 Yusuke Suzuki 2019-08-28 14:08:04 PDT
Comment on attachment 377424 [details]
Patch

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

Thanks, I'll quickly upload it with ChangeLog fix (w/o no other changes).

>> Source/JavaScriptCore/ChangeLog:35
>> +            do not emit `op_create_this` implicitly and the costructor does not return `this` object implicitly. The naked constructor allows
> 
> /costructor/constructor/

Fixed.

>> Source/JavaScriptCore/ChangeLog:39
>> +            so that we can start introducing object-allocation-sinking for JSPromise too. It is filed in [3].
> 
> Did you mean /so that we can/With this, we can/?

Yes, fixed.

>> Source/JavaScriptCore/ChangeLog:54
>> +            Some of promises are never shown to users, and they are never rejected. One example is `await`'s promise. And some of promise creation can be avoided.
> 
> /Some of promises/Some promises/

Fixed.

>> Source/JavaScriptCore/ChangeLog:63
>> +            Resolving functions have `alreadyResolved` flag not to allow calling `resolve` and `reject` multiple times. For the first resolving function creation, this
> 
> /flag not to allow/flag to prevent/?

Fixed.

>> Source/JavaScriptCore/ChangeLog:65
>> +            created again for the same promise. In that case, we just create an usual resolving functions). By doing so, we avoid unnecessary resolving functions
> 
> /an usual/a usual/?

Fixed.
Comment 64 Yusuke Suzuki 2019-08-28 14:08:31 PDT
(In reply to Yusuke Suzuki from comment #63)
> Comment on attachment 377424 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377424&action=review
> 
> Thanks, I'll quickly upload it with ChangeLog fix (w/o no other changes).

w/ no other changes.
Comment 65 Yusuke Suzuki 2019-08-28 14:11:32 PDT
Created attachment 377482 [details]
Patch
Comment 66 Keith Miller 2019-09-03 13:34:23 PDT
Comment on attachment 377482 [details]
Patch

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

Some comments more forthcoming

> Source/JavaScriptCore/ChangeLog:57
> +            takes `onFulfilled` and `onRejected` handlers and they do not need an intermediate promise for resolving. This removes internal promise allocations

Nit: They takes => they take

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:37
> +    var syncIterator = @getByIdDirectPrivate(this, "syncIterator");

Is there a reason you made these all vars?

> Source/JavaScriptCore/builtins/PromiseOperations.js:86
> +        return { @promise: promise, @resolve: resolvingFunctions.@resolve, @reject: resolvingFunctions.@reject };

Why not do:

@putDirectPrivate(resolvingFunctions, “promise”, promise);
return resolvingFunctions;
Comment 67 Yusuke Suzuki 2019-09-03 13:43:40 PDT
Comment on attachment 377482 [details]
Patch

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

Thanks, I'll quickly update the patch.

>> Source/JavaScriptCore/ChangeLog:57
>> +            takes `onFulfilled` and `onRejected` handlers and they do not need an intermediate promise for resolving. This removes internal promise allocations
> 
> Nit: They takes => they take

Fixed.

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:37
>> +    var syncIterator = @getByIdDirectPrivate(this, "syncIterator");
> 
> Is there a reason you made these all vars?

const/let emits JSEmpty initialization mov bytecode, and I do not want to emit unnecessary bytecode especially for builtin JS.
Since builtin-js is shared and standard library, I think it is worth doing micro-optimization.

>> Source/JavaScriptCore/builtins/PromiseOperations.js:86
>> +        return { @promise: promise, @resolve: resolvingFunctions.@resolve, @reject: resolvingFunctions.@reject };
> 
> Why not do:
> 
> @putDirectPrivate(resolvingFunctions, “promise”, promise);
> return resolvingFunctions;

No specific reasons. We can just put here. Fixed.
Comment 68 Yusuke Suzuki 2019-09-03 14:32:44 PDT
Created attachment 377920 [details]
Patch
Comment 69 Yusuke Suzuki 2019-09-03 14:42:40 PDT
Created attachment 377922 [details]
Patch
Comment 70 Yusuke Suzuki 2019-09-03 14:48:39 PDT
Created attachment 377923 [details]
Patch
Comment 71 Yusuke Suzuki 2019-09-03 14:50:10 PDT
Fixed the review and rebaselined.
Comment 72 Yusuke Suzuki 2019-09-03 16:31:23 PDT
Created attachment 377932 [details]
Patch
Comment 73 Saam Barati 2019-09-03 22:10:10 PDT
Comment on attachment 377932 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        This is the major change of the Promise implementation and it improves JetStream2/async-fs by 62%.

👍🏼

> Source/JavaScriptCore/ChangeLog:10
> +        1. Make JSPromise C++ friendly

nit: in the future, it'd be nice to split these things in their own patches as they don't seem like they need to be landed all at once.

> Source/JavaScriptCore/ChangeLog:27
> +            The old JSPromise constructor was doing very inefficient thing: JSPromise constructor is InternalFunction in C++, and in it, it

"was doing very inefficient thing" => "was very inefficient"

> Source/JavaScriptCore/ChangeLog:29
> +            If we can implement JSPromise constructor in all JS code, we can recognize `executor` and we have a chance to fully inline them.

"in all JS code" => "fully in JS"

> Source/JavaScriptCore/ChangeLog:31
> +            construct is 100. If we decide inlining it, we should reinvestigate it further.

"If we decide inlining it, we should reinvestigate it further." => "We might want to investigate getting it inlined in the future."

Also, might be worth a bug?

> Source/JavaScriptCore/ChangeLog:33
> +            We can avoid C++ <-> JS dance in such an important operation, allocating JSPromise. This patch introduces @nakedConstructor

this is cool

> Source/JavaScriptCore/ChangeLog:46
> +            When converting CreatePromise to NewPromise, we need to get the correct structure with a specified `callee.prototype`. We mimics the mechanism

mimics => mimic

> Source/JavaScriptCore/ChangeLog:59
> +            check to take a fast path. And promise reaction now handles 4 cases to handle the case not creating a promise.

the final sentence here doesn't make sense to me

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:-30
> -    const promiseCapability = @newPromiseCapability(@Promise);

you should add Apple copyright to this file too

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:42
> +        var nextDone = !!nextResult.done;
> +        var nextValue = nextResult.value;

does this change the order we read the fields from the result? Isn't that observable?

> Source/JavaScriptCore/builtins/PromiseConstructor.js:234
> +    var promise = result;

what's the point of this local variable?

> Source/JavaScriptCore/builtins/PromiseConstructor.js:242
> +    function @resolve(resolution) {
> +        return @resolvePromiseWithFirstResolvingFunctionCallCheck(promise, resolution);
> +    }
> +
> +    function @reject(reason) {
> +        return @rejectPromiseWithFirstResolvingFunctionCallCheck(promise, reason);
> +    }

why give these private names instead of just "resolve" and "reject"?

> Source/JavaScriptCore/builtins/PromiseConstructor.js:270
> +    function @resolve(resolution) {
> +        return @resolvePromiseWithFirstResolvingFunctionCallCheck(promise, resolution);
> +    }
> +
> +    function @reject(reason) {
> +        return @rejectPromiseWithFirstResolvingFunctionCallCheck(promise, reason);
> +    }

ditto

> Source/JavaScriptCore/builtins/PromiseOperations.js:134
> +        return @rejectPromise(promise, @makeTypeError("Resolve a promise with itself"));

maybe "Cannot resolve a promise with itself"?

> Source/JavaScriptCore/builtins/PromiseOperations.js:146
> +    if (@isPromise(resolution) && then === @then) {

maybe we should name "@then" in the global namespace to be less generic. Maybe "@defaultPromiseThen" or something?

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1302
> +            auto& cacheWriteBarrier = metadata.m_cachedCallee;
> +            if (!cacheWriteBarrier || cacheWriteBarrier.unvalidatedGet() == JSCell::seenMultipleCalleeObjects())
> +                break;
> +            JSCell* cachedFunction = cacheWriteBarrier.get();

isn't this a TOCTOU bug? You load the first time and it's ok, and then you load again on line 1302 and it's now "seenMultipleCalleeObjects". Or is this called with the mutator paused?

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2628
> +                    if (rareData->allocationProfileWatchpointSet().isStillValid()) {
> +                        if (Structure* structure = rareData->objectAllocationStructure()) {

why this change? Seems unrelated to your patch

> Source/JavaScriptCore/dfg/DFGUseKind.h:58
> +    PromiseObjectUse,

This seems like it's completely unused. I'd vote for removing it. You said in your changelog that you would use this for making @isPromise faster, but I don't see it.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5923
> +        ValueFromBlock slowResult = m_out.anchor(vmCall(pointerType(), m_out.operation(m_node->isInternalPromise() ? operationNewInternalPromise : operationNewPromise), m_callFrame, weakStructure(m_node->structure())));

why weakStructure here? This feels like a "strong" use case, since all objects which have this structure might go away, but it shouldn't mean we throw this code away, since the code might be reachable and we allocate new structures.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6302
> +        ValueFromBlock promiseStructure = m_out.anchor(weakStructure(m_graph.registerStructure(m_node->isInternalPromise() ? globalObject->internalPromiseStructure() : globalObject->promiseStructure())));

ditto: doesn't feel like it should be weak

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:528
> +    btpz value, .writeBarrierDone

why would we be putting the JSValue zero? Seems wrong.

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2598
> +    getu(size, OpGetPromiseInternalField, m_index, t2)

this zero extends?

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2607
> +    getu(size, OpPutPromiseInternalField, m_index, t1)

this zero extends?

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:289
> +    if (constructorAsObject->type() == JSFunctionType && jsCast<JSFunction*>(constructorAsObject)->canUseAllocationProfile()) {

nit: I think it's nicer to jsDynamicCast<JSFunction*> here instead of checking type manually.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1684
> +    visitor.append(thisObject->m_internalPromiseConstructor);

oops

> Source/JavaScriptCore/runtime/JSPromise.h:72
> +    static ptrdiff_t offsetOfInternalField(unsigned index) { return OBJECT_OFFSETOF(JSPromise, m_internalFields) + index * sizeof(WriteBarrier<Unknown>); }

does std::array give us this guarantee?
Comment 74 Yusuke Suzuki 2019-09-04 03:48:34 PDT
Comment on attachment 377932 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:10
>> +        1. Make JSPromise C++ friendly
> 
> nit: in the future, it'd be nice to split these things in their own patches as they don't seem like they need to be landed all at once.

Yeah, right.

>> Source/JavaScriptCore/ChangeLog:27
>> +            The old JSPromise constructor was doing very inefficient thing: JSPromise constructor is InternalFunction in C++, and in it, it
> 
> "was doing very inefficient thing" => "was very inefficient"

Fixed.

>> Source/JavaScriptCore/ChangeLog:29
>> +            If we can implement JSPromise constructor in all JS code, we can recognize `executor` and we have a chance to fully inline them.
> 
> "in all JS code" => "fully in JS"

Fixed.

>> Source/JavaScriptCore/ChangeLog:31
>> +            construct is 100. If we decide inlining it, we should reinvestigate it further.
> 
> "If we decide inlining it, we should reinvestigate it further." => "We might want to investigate getting it inlined in the future."
> 
> Also, might be worth a bug?

Yes, filed in https://bugs.webkit.org/show_bug.cgi?id=201452.

>> Source/JavaScriptCore/ChangeLog:46
>> +            When converting CreatePromise to NewPromise, we need to get the correct structure with a specified `callee.prototype`. We mimics the mechanism
> 
> mimics => mimic

Fixed.

>> Source/JavaScriptCore/ChangeLog:59
>> +            check to take a fast path. And promise reaction now handles 4 cases to handle the case not creating a promise.
> 
> the final sentence here doesn't make sense to me

We introduced four types of promise reactions to avoid some of object allocations. And microtask reaction is handling these four types.
Fixed the sentence.

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:-30
>> -    const promiseCapability = @newPromiseCapability(@Promise);
> 
> you should add Apple copyright to this file too

Right, added.

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:42
>> +        var nextValue = nextResult.value;
> 
> does this change the order we read the fields from the result? Isn't that observable?

This is observable, and the old order was wrong.

>> Source/JavaScriptCore/builtins/PromiseConstructor.js:242
>> +    }
> 
> why give these private names instead of just "resolve" and "reject"?

The spec defines these functions do not have name. And if we use private symbols here, `function.name` property is not defined. So we were using this intentionally here, and this patch continues using this.

>> Source/JavaScriptCore/builtins/PromiseOperations.js:134
>> +        return @rejectPromise(promise, @makeTypeError("Resolve a promise with itself"));
> 
> maybe "Cannot resolve a promise with itself"?

Sounds nice. Fixed.

>> Source/JavaScriptCore/builtins/PromiseOperations.js:146
>> +    if (@isPromise(resolution) && then === @then) {
> 
> maybe we should name "@then" in the global namespace to be less generic. Maybe "@defaultPromiseThen" or something?

That looks better. Changed.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1302
>> +            JSCell* cachedFunction = cacheWriteBarrier.get();
> 
> isn't this a TOCTOU bug? You load the first time and it's ok, and then you load again on line 1302 and it's now "seenMultipleCalleeObjects". Or is this called with the mutator paused?

finalizeLLIntInlineCaches is called when unconditional-finalizer happens. And unconditional finalizers are executed while stopping the mutator.

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2628
>> +                        if (Structure* structure = rareData->objectAllocationStructure()) {
> 
> why this change? Seems unrelated to your patch

When I wrote the code for CreatePromise in AI, I noticed that adding a watchpoint check is better here to align this code to constant-folding's one.
For correctness issue, this does not matter since we watch the watchpoint and if the watchpoint is invalidated, then we invalidate it.

>> Source/JavaScriptCore/dfg/DFGUseKind.h:58
>> +    PromiseObjectUse,
> 
> This seems like it's completely unused. I'd vote for removing it. You said in your changelog that you would use this for making @isPromise faster, but I don't see it.

Currently our DFG IsCellWithType always requires corresponding usekind so we need to have this one. I think having this one is not profitable right now but it is harmless.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5923
>> +        ValueFromBlock slowResult = m_out.anchor(vmCall(pointerType(), m_out.operation(m_node->isInternalPromise() ? operationNewInternalPromise : operationNewPromise), m_callFrame, weakStructure(m_node->structure())));
> 
> why weakStructure here? This feels like a "strong" use case, since all objects which have this structure might go away, but it shouldn't mean we throw this code away, since the code might be reachable and we allocate new structures.

Right. Fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6302
>> +        ValueFromBlock promiseStructure = m_out.anchor(weakStructure(m_graph.registerStructure(m_node->isInternalPromise() ? globalObject->internalPromiseStructure() : globalObject->promiseStructure())));
> 
> ditto: doesn't feel like it should be weak

This does not matter in practice since these structures are stored in JSGlobalObject.

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:528
>> +    btpz value, .writeBarrierDone
> 
> why would we be putting the JSValue zero? Seems wrong.

The JSEmpty. And I would like to allow JSEmpty thing to later extend this internal field mechanism for generators.

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2598
>> +    getu(size, OpGetPromiseInternalField, m_index, t2)
> 
> this zero extends?

Yes, this zero-extends. getu will emit loadb, loadh, or loadi for narrow, wide16, and wide32. And they performs zero-extension.

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2607
>> +    getu(size, OpPutPromiseInternalField, m_index, t1)
> 
> this zero extends?

Yes.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:289
>> +    if (constructorAsObject->type() == JSFunctionType && jsCast<JSFunction*>(constructorAsObject)->canUseAllocationProfile()) {
> 
> nit: I think it's nicer to jsDynamicCast<JSFunction*> here instead of checking type manually.

Sounds nice. Fixed.

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1684
>> +    visitor.append(thisObject->m_internalPromiseConstructor);
> 
> oops

:)

>> Source/JavaScriptCore/runtime/JSPromise.h:72
>> +    static ptrdiff_t offsetOfInternalField(unsigned index) { return OBJECT_OFFSETOF(JSPromise, m_internalFields) + index * sizeof(WriteBarrier<Unknown>); }
> 
> does std::array give us this guarantee?

Whle std::array's memory layout needs to be contiguous, I think the spec does not guarantee like, `sizeof(std::array<A, N>) == sizeof(A) * N`. maybe, just using C array is simpler here.
Comment 75 Yusuke Suzuki 2019-09-04 03:54:16 PDT
Comment on attachment 377932 [details]
Patch

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

>> Source/JavaScriptCore/builtins/PromiseConstructor.js:234
>> +    var promise = result;
> 
> what's the point of this local variable?

This is a bit of hack to reduce bytecode size to avoid `resolve_scope` and `get_from_scope` in `return result`.
Comment 76 Yusuke Suzuki 2019-09-04 03:54:39 PDT
Created attachment 377970 [details]
Patch for landing
Comment 77 Yusuke Suzuki 2019-09-04 18:23:54 PDT
Committed r249509: <https://trac.webkit.org/changeset/249509>
Comment 78 Yusuke Suzuki 2019-09-04 21:01:22 PDT
Committed r249520: <https://trac.webkit.org/changeset/249520>
Comment 79 Yusuke Suzuki 2019-09-04 21:08:46 PDT
1% progression in JetStream2 on MBP.
Comment 80 Yusuke Suzuki 2019-09-04 21:27:54 PDT
And it also improves RAMification's async-fs memory.
Comment 81 Yusuke Suzuki 2019-09-05 00:06:44 PDT
Followup fix. https://bugs.webkit.org/show_bug.cgi?id=201495
Comment 82 Saam Barati 2019-09-05 09:13:38 PDT
Comment on attachment 377932 [details]
Patch

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

>>> Source/JavaScriptCore/builtins/PromiseConstructor.js:234
>>> +    var promise = result;
>> 
>> what's the point of this local variable?
> 
> This is a bit of hack to reduce bytecode size to avoid `resolve_scope` and `get_from_scope` in `return result`.

Nit: I’d name it that way then, to allude to the purpose. Maybe “capturedPromise”
Comment 83 Yusuke Suzuki 2019-09-05 13:51:43 PDT
(In reply to Saam Barati from comment #82)
> Comment on attachment 377932 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377932&action=review
> 
> >>> Source/JavaScriptCore/builtins/PromiseConstructor.js:234
> >>> +    var promise = result;
> >> 
> >> what's the point of this local variable?
> > 
> > This is a bit of hack to reduce bytecode size to avoid `resolve_scope` and `get_from_scope` in `return result`.
> 
> Nit: I’d name it that way then, to allude to the purpose. Maybe
> “capturedPromise”

Sounds nice. I'll land this rename with the follow-up of https://bugs.webkit.org/show_bug.cgi?id=201495 (which is follow-up of this promise patch).
Comment 84 Yusuke Suzuki 2020-08-13 19:39:24 PDT
*** Bug 195534 has been marked as a duplicate of this bug. ***
Comment 85 Alexey Shvayka 2021-01-04 11:04:34 PST
*** Bug 172419 has been marked as a duplicate of this bug. ***