Bug 186462 – [DFG] Fold GetByVal if the indexed value is non configurable and non writable

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

[Yusuke Suzuki]

If the indexed value is non-writable and non-configurable, we can fold it into a constant in DFG AI.
This is useful since tagged templates' values in arrays have these attributes.

[Yusuke Suzuki]

Created attachment 345517 [details]
Patch

[Yusuke Suzuki]

WIP: SparseValueMapEntry is not updated with locking. So the above patch is not completed.

[EWS Watchlist]

Attachment 345517 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:117:  The parameter name "i" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 345518 [details]
Patch

[Yusuke Suzuki]

Created attachment 345519 [details]
Patch

[EWS Watchlist]

Comment on attachment 345519 [details]
Patch

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

New failing tests:
js/mozilla/strict/15.4.4.9.html
js/arguments.html
js/mozilla/strict/10.6.html
js/mozilla/strict/15.4.4.8.html
js/mozilla/strict/15.4.4.12.html
js/mozilla/strict/15.4.4.13.html
js/dom/Object-defineProperty.html
js/array-defineOwnProperty.html
js/dom/reflect-set-onto-dom.html
js/mozilla/strict/8.12.5.html

[EWS Watchlist]

Created attachment 345521 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 345519 [details]
Patch

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

New failing tests:
jsc-layout-tests.yaml/js/script-tests/array-defineOwnProperty.js.layout
jsc-layout-tests.yaml/js/script-tests/array-defineOwnProperty.js.layout-dfg-eager-no-cjit
stress/arguments-define-property.js.ftl-eager
stress/arguments-define-property.js.ftl-no-cjit-small-pool
stress/arguments-define-property.js.dfg-eager
stress/arguments-define-property.js.default
stress/arguments-define-property.js.ftl-eager-no-cjit
ChakraCore.yaml/ChakraCore/test/es5/array_length.js.default
stress/arguments-define-property.js.ftl-no-cjit-no-inline-validate
stress/arguments-define-property.js.dfg-eager-no-cjit-validate
stress/arguments-define-property.js.no-cjit-collect-continuously
stress/arguments-define-property.js.ftl-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/arguments.js.layout-dfg-eager-no-cjit
stress/arguments-define-property.js.ftl-no-cjit-validate-sampling-profiler
jsc-layout-tests.yaml/js/script-tests/arguments.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/arguments.js.layout
jsc-layout-tests.yaml/js/script-tests/arguments.js.layout-no-llint
stress/arguments-define-property.js.ftl-eager-no-cjit-b3o1
stress/arguments-define-property.js.ftl-no-cjit-no-put-stack-validate
ChakraCore.yaml/ChakraCore/test/Miscellaneous/HasOnlyWritableDataPropertiesCache.js.default
jsc-layout-tests.yaml/js/script-tests/array-defineOwnProperty.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/arguments.js.layout-ftl-no-cjit
stress/arguments-define-property.js.no-llint
jsc-layout-tests.yaml/js/script-tests/array-defineOwnProperty.js.layout-ftl-no-cjit
stress/arguments-define-property.js.dfg-maximal-flush-validate-no-cjit
ChakraCore.yaml/ChakraCore/test/es5/defineIndexProperty.js.default
jsc-layout-tests.yaml/js/script-tests/arguments.js.layout-ftl-eager-no-cjit
stress/arguments-define-property.js.no-cjit-validate-phases
jsc-layout-tests.yaml/js/script-tests/array-defineOwnProperty.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/array-defineOwnProperty.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/arguments.js.layout-no-ftl
stress/arguments-define-property.js.no-ftl
jsc-layout-tests.yaml/js/script-tests/array-defineOwnProperty.js.layout-no-ftl
apiTests

[EWS Watchlist]

Comment on attachment 345519 [details]
Patch

Attachment 345519 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8612067

New failing tests:
js/mozilla/strict/15.4.4.9.html
js/arguments.html
js/mozilla/strict/10.6.html
js/mozilla/strict/15.4.4.8.html
js/mozilla/strict/15.4.4.12.html
js/mozilla/strict/15.4.4.13.html
js/dom/Object-defineProperty.html
js/array-defineOwnProperty.html
js/dom/reflect-set-onto-dom.html
js/mozilla/strict/8.12.5.html

[EWS Watchlist]

Created attachment 345524 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 345519 [details]
Patch

Attachment 345519 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8612056

New failing tests:
js/mozilla/strict/15.4.4.9.html
js/arguments.html
js/mozilla/strict/10.6.html
js/mozilla/strict/15.4.4.8.html
js/mozilla/strict/15.4.4.12.html
js/mozilla/strict/15.4.4.13.html
js/dom/Object-defineProperty.html
js/array-defineOwnProperty.html
js/dom/reflect-set-onto-dom.html
js/mozilla/strict/8.12.5.html

[EWS Watchlist]

Created attachment 345525 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 345519 [details]
Patch

Attachment 345519 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8612950

New failing tests:
js/mozilla/strict/15.4.4.9.html
js/arguments.html
js/mozilla/strict/10.6.html
js/mozilla/strict/15.4.4.8.html
js/mozilla/strict/15.4.4.12.html
js/mozilla/strict/15.4.4.13.html
js/dom/Object-defineProperty.html
js/array-defineOwnProperty.html
js/dom/reflect-set-onto-dom.html
js/mozilla/strict/8.12.5.html

[EWS Watchlist]

Created attachment 345533 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 345519 [details]
Patch

Attachment 345519 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8612962

New failing tests:
js/mozilla/strict/15.4.4.9.html
js/arguments.html
js/mozilla/strict/10.6.html
js/mozilla/strict/15.4.4.8.html
js/mozilla/strict/15.4.4.12.html
js/mozilla/strict/15.4.4.13.html
js/dom/Object-defineProperty.html
js/array-defineOwnProperty.html
js/dom/reflect-set-onto-dom.html
js/mozilla/strict/8.12.5.html

[EWS Watchlist]

Created attachment 345534 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 345519 [details]
Patch

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

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 345535 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 345519 [details]
Patch

Attachment 345519 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8613531

New failing tests:
js/mozilla/strict/15.4.4.9.html
js/arguments.html
js/mozilla/strict/10.6.html
js/mozilla/strict/15.4.4.8.html
js/mozilla/strict/15.4.4.12.html
js/mozilla/strict/15.4.4.13.html
js/dom/Object-defineProperty.html
js/array-defineOwnProperty.html
js/dom/reflect-set-onto-dom.html
js/mozilla/strict/8.12.5.html

[EWS Watchlist]

Created attachment 345536 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Yusuke Suzuki]

Created attachment 345537 [details]
Patch

[Saam Barati]

Comment on attachment 345537 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1898
> +                if (node->arrayMode().type() == Array::ArrayStorage || node->arrayMode().type() == Array::SlowPutArrayStorage) {

The name of the lambda this code is inside of needs to be changed since this extends beyond CoW folding

> Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp:190
> +    WTF::loadLoadFence();

This can use Dependency instead of loadLoadFence so it's fast on ARM if we decide to support ARM in the future. On x86, if you refactor this to us Dependency you'll just end up with the same code being emitted.

[Yusuke Suzuki]

Comment on attachment 345537 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1898
>> +                if (node->arrayMode().type() == Array::ArrayStorage || node->arrayMode().type() == Array::SlowPutArrayStorage) {
> 
> The name of the lambda this code is inside of needs to be changed since this extends beyond CoW folding

Oops, right. Fixed.

>> Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp:190
>> +    WTF::loadLoadFence();
> 
> This can use Dependency instead of loadLoadFence so it's fast on ARM if we decide to support ARM in the future. On x86, if you refactor this to us Dependency you'll just end up with the same code being emitted.

Good call! Fixed.

[Yusuke Suzuki]

Committed r234089: <https://trac.webkit.org/changeset/234089>

[Radar WebKit Bug Importer]

<rdar://problem/42480098>