Bug 200782 – Fix InBounds speculation of typed array PutByVal and add extra step to integer range optimization to search for equality relationships on the RHS value

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

[Justin Michaud]

Speculate that putByVals on typed arrays are in bounds initially, and add an extra rule to integer range optimization to remove their checks when we are looping over two arrays.

[Justin Michaud]

Created attachment 376413 [details]
Patch

[EWS Watchlist]

Comment on attachment 376413 [details]
Patch

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

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 376420 [details]
Archive of layout-test-results from ews100 for mac-highsierra

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

[Justin Michaud]

Created attachment 376421 [details]
Patch

[EWS Watchlist]

Comment on attachment 376421 [details]
Patch

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

New failing tests:
fast/canvas/canvas-ImageData-behaviour.html
imported/w3c/canvas/2d.imageData.object.string.html
js/builtins/shielding-typedarray.html
canvas/philip/tests/2d.imageData.object.string.html
webgl/2.0.0/conformance2/misc/views-with-offsets.html

[EWS Watchlist]

Created attachment 376432 [details]
Archive of layout-test-results from ews100 for mac-highsierra

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

[Saam Barati]

Comment on attachment 376421 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        Omit CheckInBounds for memcpy loops on typed arrays

this name seems weird, since it's for way more than memcpy

> Source/JavaScriptCore/ChangeLog:9
> +        Speculate that putByVals on typed arrays are in bounds initially, and add an extra rule to integer range optimization to 
> +        remove their checks when we are looping over two arrays.

This doesn't feel like an apt description. Can you:
- Say what change you're making so we don't mark PutByVal as OOB on types arrays
- say what change you actually made to the integer range optimization phase. This is not at all limited to loops.

[EWS Watchlist]

Comment on attachment 376421 [details]
Patch

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

New failing tests:
fast/canvas/canvas-ImageData-behaviour.html
canvas/philip/tests/2d.imageData.object.nan.html
webgl/2.0.0/conformance2/misc/views-with-offsets.html
imported/w3c/canvas/2d.imageData.object.string.html
imported/w3c/canvas/2d.imageData.object.nan.html
js/builtins/shielding-typedarray.html
canvas/philip/tests/2d.imageData.object.string.html
imported/w3c/canvas/2d.imageData.object.undefined.html
canvas/philip/tests/2d.imageData.object.undefined.html

[EWS Watchlist]

Created attachment 376438 [details]
Archive of layout-test-results from ews114 for mac-highsierra

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

[EWS Watchlist]

Comment on attachment 376421 [details]
Patch

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

New failing tests:
fast/canvas/canvas-ImageData-behaviour.html
imported/w3c/canvas/2d.imageData.object.string.html
js/builtins/shielding-typedarray.html
canvas/philip/tests/2d.imageData.object.string.html

[EWS Watchlist]

Created attachment 376446 [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

[Saam Barati]

Comment on attachment 376421 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSObjectInlines.h:406
> +inline bool JSObject::canSetIndexQuicklyForTypedArray(unsigned i) const

You need to see what value you're storing here!

[Justin Michaud]

Created attachment 376453 [details]
Patch

[Saam Barati]

Comment on attachment 376453 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGIntegerRangeOptimizationPhase.cpp:1694
> +                // We have:
> +                //     @a op @b
> +                //     @b == @c
> +                //
> +                // This implies:
> +                //     @a op @c
> +                //
> +                // Where: @a == relationship.left(), @b == relationship.right()

please write this out with the constants.

> Source/JavaScriptCore/jit/JITOperations.cpp:-2082
> -        // https://bugs.webkit.org/show_bug.cgi?id=149886

should we mark this bug as a dupe?

> JSTests/ChangeLog:8
> +        * stress/int8-repeat-in-then-out-of-bounds.js: Added.

Can we also add microbenchmarks for:
1. Repeatedly OSR exitting because we do OOB on typed array
2. Eliminating the CheckInBounds in the loop based on your changes in the integer range optimization phase

[Justin Michaud]

Created attachment 376531 [details]
Patch

[Justin Michaud]

Comment on attachment 376453 [details]
Patch

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

>> Source/JavaScriptCore/jit/JITOperations.cpp:-2082
>> -        // https://bugs.webkit.org/show_bug.cgi?id=149886
> 
> should we mark this bug as a dupe?

No, there is one more fixme left in hasIndexedProperty (I think)

[Saam Barati]

Comment on attachment 376531 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:13
> +        Microbenchmarks give a 40% improvement on the memcpy loop test, and neutral on the out-of-bounds typed array test.

why not faster on the OOB test? Aren't we skipping repeatedly exitting?

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:132
> +    bool canSetIndexQuickly(unsigned i, JSValue v) const

nit: we typically call this "value", not "v"

> Source/JavaScriptCore/runtime/JSObject.h:370
> +    bool canSetIndexQuickly(unsigned i, JSValue v)

style nit: we typically call this "value", not "v"

[Justin Michaud]

Created attachment 376533 [details]
Patch

[Justin Michaud]

Comment on attachment 376531 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:13
>> +        Microbenchmarks give a 40% improvement on the memcpy loop test, and neutral on the out-of-bounds typed array test.
> 
> why not faster on the OOB test? Aren't we skipping repeatedly exitting?

No, since the existing bug prevented us from speculating in bounds in the first place. The in-bounds loop should be slightly faster (and it is), it is just not enough to be "definitely" faster.

[WebKit Commit Bot]

Comment on attachment 376533 [details]
Patch

Clearing flags on attachment: 376533

Committed r248798: <https://trac.webkit.org/changeset/248798>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54407659>

[Ryan Haddad]

Some variants of one of the new tests are timing out on the debug bot:

** The following JSC stress test failures have been introduced:
	microbenchmarks/memcpy-typed-loop.js.dfg-eager-no-cjit-validate
	microbenchmarks/memcpy-typed-loop.js.dfg-maximal-flush-validate-no-cjit
	microbenchmarks/memcpy-typed-loop.js.no-cjit-validate-phases

https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20(Tests)/builds/3480/steps/jscore-test/logs/stdio

[Justin Michaud]

I will skip those tests on debug.

[Ryan Haddad]

One of the other tests looks like it may be a flaky failure on the release bot:

Running stress/int8-repeat-out-of-bounds.js.default
stress/int8-repeat-in-then-out-of-bounds.js.no-llint: Exception: Error: unexpected retry count: 0
stress/int8-repeat-in-then-out-of-bounds.js.no-llint: ERROR: Unexpected exit code: 3

https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20JSC%20%28Tests%29/builds/10678/steps/jscore-test/logs/stdio

[Justin Michaud]

Reopening to attach new patch.

[Justin Michaud]

Created attachment 376726 [details]
Patch

[Saam Barati]

Comment on attachment 376726 [details]
Patch

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

> JSTests/stress/int8-repeat-in-then-out-of-bounds.js:16
>  
> +for (var i = 0; i < 100000; ++i)
> +    foo(array, true);

You don't need this, but you should just make this test run with CJIT off

[Justin Michaud]

Created attachment 376731 [details]
Patch

[Saam Barati]

Comment on attachment 376731 [details]
Patch

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

> Tools/Scripts/run-jsc-stress-tests:888
>  def defaultNoEagerRun

just make an option

[Justin Michaud]

Created attachment 376732 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 376732 [details]
Patch

Clearing flags on attachment: 376732

Committed r248905: <https://trac.webkit.org/changeset/248905>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.