Summary: | [JSC] Make Promise implementation faster | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | 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
Yusuke Suzuki
2019-08-19 14:10:18 PDT
Created attachment 376847 [details]
Patch
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. Created attachment 376849 [details]
Patch
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 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. 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
Created attachment 376912 [details]
Patch
Created attachment 376913 [details]
Patch
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. 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 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 Created attachment 376953 [details]
Patch
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. 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
Created attachment 376971 [details]
Patch
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. 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
Created attachment 376994 [details]
Patch
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 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. 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 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 Created attachment 377084 [details]
Patch
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 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. 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
Created attachment 377202 [details]
Patch
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.
Created attachment 377203 [details]
Patch
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 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. 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 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 Created attachment 377206 [details]
Patch
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.
Created attachment 377207 [details]
Patch
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.
Created attachment 377208 [details]
Patch
Created attachment 377209 [details]
Patch
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.
Created attachment 377215 [details]
Patch
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.
Created attachment 377216 [details]
Patch
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.
Created attachment 377229 [details]
Patch
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.
Created attachment 377289 [details]
Patch
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.
Created attachment 377292 [details]
Patch
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.
Created attachment 377305 [details]
Patch
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.
Created attachment 377313 [details]
Patch
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.
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. Created attachment 377406 [details]
Patch
Created attachment 377409 [details]
Patch
The failed inspector/console/message-stack-trace.html test from the mac-wk2 bot seems legit. Created attachment 377424 [details]
Patch
(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 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 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. (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. Created attachment 377482 [details]
Patch
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 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. Created attachment 377920 [details]
Patch
Created attachment 377922 [details]
Patch
Created attachment 377923 [details]
Patch
Fixed the review and rebaselined. Created attachment 377932 [details]
Patch
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 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 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`. Created attachment 377970 [details]
Patch for landing
Committed r249509: <https://trac.webkit.org/changeset/249509> Committed r249520: <https://trac.webkit.org/changeset/249520> 1% progression in JetStream2 on MBP. And it also improves RAMification's async-fs memory. Followup fix. https://bugs.webkit.org/show_bug.cgi?id=201495 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” (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). *** Bug 195534 has been marked as a duplicate of this bug. *** *** Bug 172419 has been marked as a duplicate of this bug. *** |