Bug 200898 – [JSC] Make Promise implementation faster

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

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'

[Yusuke Suzuki]

Created attachment 376847 [details]
Patch

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 376849 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Radar WebKit Bug Importer]

<rdar://problem/54569373>

[Yusuke Suzuki]

Created attachment 376912 [details]
Patch

[Yusuke Suzuki]

Created attachment 376913 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Yusuke Suzuki]

Created attachment 376953 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Yusuke Suzuki]

Created attachment 376971 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Yusuke Suzuki]

Created attachment 376994 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Yusuke Suzuki]

Created attachment 377084 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Yusuke Suzuki]

Created attachment 377202 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 377203 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Yusuke Suzuki]

Created attachment 377206 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 377207 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 377208 [details]
Patch

[Yusuke Suzuki]

Created attachment 377209 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 377215 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 377216 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 377229 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 377289 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 377292 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 377305 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 377313 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 377406 [details]
Patch

[Yusuke Suzuki]

Created attachment 377409 [details]
Patch

[Mark Lam]

The failed inspector/console/message-stack-trace.html test from the mac-wk2 bot seems legit.

[Yusuke Suzuki]

Created attachment 377424 [details]
Patch

[Yusuke Suzuki]

(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.

[Mark Lam]

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/?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

Created attachment 377482 [details]
Patch

[Keith Miller]

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;

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 377920 [details]
Patch

[Yusuke Suzuki]

Created attachment 377922 [details]
Patch

[Yusuke Suzuki]

Created attachment 377923 [details]
Patch

[Yusuke Suzuki]

Fixed the review and rebaselined.

[Yusuke Suzuki]

Created attachment 377932 [details]
Patch

[Saam Barati]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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`.

[Yusuke Suzuki]

Created attachment 377970 [details]
Patch for landing

[Yusuke Suzuki]

Committed r249509: <https://trac.webkit.org/changeset/249509>

[Yusuke Suzuki]

Committed r249520: <https://trac.webkit.org/changeset/249520>

[Yusuke Suzuki]

1% progression in JetStream2 on MBP.

[Yusuke Suzuki]

And it also improves RAMification's async-fs memory.

[Yusuke Suzuki]

Followup fix. https://bugs.webkit.org/show_bug.cgi?id=201495

[Saam Barati]

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”

[Yusuke Suzuki]

(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).

[Yusuke Suzuki]

*** Bug 195534 has been marked as a duplicate of this bug. ***

[Alexey Shvayka]

*** Bug 172419 has been marked as a duplicate of this bug. ***