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
Summary: Fix InBounds speculation of typed array PutByVal and add extra step to intege...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Michaud
URL:
Keywords: InRadar
Depends on:
Blocks: 149886
  Show dependency treegraph
 
Reported: 2019-08-15 12:51 PDT by Justin Michaud
Modified: 2019-08-20 10:35 PDT (History)
11 users (show)

See Also:


Attachments
Patch (13.97 KB, patch)
2019-08-15 13:06 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (529.26 KB, application/zip)
2019-08-15 14:05 PDT, Build Bot
no flags Details
Patch (14.03 KB, patch)
2019-08-15 14:09 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (3.50 MB, application/zip)
2019-08-15 15:23 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (3.88 MB, application/zip)
2019-08-15 16:20 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews212 for win-future (14.36 MB, application/zip)
2019-08-15 16:53 PDT, Build Bot
no flags Details
Patch (16.95 KB, patch)
2019-08-15 17:41 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (18.94 KB, patch)
2019-08-16 13:31 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (18.95 KB, patch)
2019-08-16 13:48 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (2.11 KB, patch)
2019-08-19 17:51 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (2.84 KB, patch)
2019-08-19 19:15 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (3.94 KB, patch)
2019-08-19 19:21 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Michaud 2019-08-15 12:51:05 PDT
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.
Comment 1 Justin Michaud 2019-08-15 13:06:08 PDT
Created attachment 376413 [details]
Patch
Comment 2 Build Bot 2019-08-15 14:05:41 PDT
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.
Comment 3 Build Bot 2019-08-15 14:05:42 PDT
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
Comment 4 Justin Michaud 2019-08-15 14:09:12 PDT
Created attachment 376421 [details]
Patch
Comment 5 Build Bot 2019-08-15 15:23:08 PDT
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
Comment 6 Build Bot 2019-08-15 15:23:10 PDT
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
Comment 7 Saam Barati 2019-08-15 16:06:56 PDT
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.
Comment 8 Build Bot 2019-08-15 16:20:02 PDT
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
Comment 9 Build Bot 2019-08-15 16:20:05 PDT
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
Comment 10 Build Bot 2019-08-15 16:53:00 PDT
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
Comment 11 Build Bot 2019-08-15 16:53:03 PDT
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
Comment 12 Saam Barati 2019-08-15 16:57:28 PDT
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!
Comment 13 Justin Michaud 2019-08-15 17:41:19 PDT
Created attachment 376453 [details]
Patch
Comment 14 Saam Barati 2019-08-16 11:38:20 PDT
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
Comment 15 Justin Michaud 2019-08-16 13:31:42 PDT
Created attachment 376531 [details]
Patch
Comment 16 Justin Michaud 2019-08-16 13:34:34 PDT
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)
Comment 17 Saam Barati 2019-08-16 13:36:25 PDT
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"
Comment 18 Justin Michaud 2019-08-16 13:48:37 PDT
Created attachment 376533 [details]
Patch
Comment 19 Justin Michaud 2019-08-16 13:50:44 PDT
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.
Comment 20 WebKit Commit Bot 2019-08-16 14:33:35 PDT
Comment on attachment 376533 [details]
Patch

Clearing flags on attachment: 376533

Committed r248798: <https://trac.webkit.org/changeset/248798>
Comment 21 WebKit Commit Bot 2019-08-16 14:33:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Radar WebKit Bug Importer 2019-08-16 14:34:35 PDT
<rdar://problem/54407659>
Comment 23 Ryan Haddad 2019-08-19 16:09:46 PDT
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
Comment 24 Justin Michaud 2019-08-19 16:12:29 PDT
I will skip those tests on debug.
Comment 25 Ryan Haddad 2019-08-19 16:46:18 PDT
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
Comment 26 Justin Michaud 2019-08-19 17:51:39 PDT
Reopening to attach new patch.
Comment 27 Justin Michaud 2019-08-19 17:51:40 PDT
Created attachment 376726 [details]
Patch
Comment 28 Saam Barati 2019-08-19 18:05:03 PDT
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
Comment 29 Justin Michaud 2019-08-19 19:15:39 PDT
Created attachment 376731 [details]
Patch
Comment 30 Saam Barati 2019-08-19 19:17:56 PDT
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
Comment 31 Justin Michaud 2019-08-19 19:21:17 PDT
Created attachment 376732 [details]
Patch
Comment 32 WebKit Commit Bot 2019-08-20 10:35:52 PDT
Comment on attachment 376732 [details]
Patch

Clearing flags on attachment: 376732

Committed r248905: <https://trac.webkit.org/changeset/248905>
Comment 33 WebKit Commit Bot 2019-08-20 10:35:54 PDT
All reviewed patches have been landed.  Closing bug.