Summary: | Fix InBounds speculation of typed array PutByVal and add extra step to integer range optimization to search for equality relationships on the RHS value | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Justin Michaud <justin_michaud> | ||||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Justin Michaud <justin_michaud> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, rniwa, ryanhaddad, saam, tzagallo, webkit-bot-watchers-bugzilla, webkit-bug-importer | ||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||
Bug Blocks: | 149886 | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Justin Michaud
2019-08-15 12:51:05 PDT
Created attachment 376413 [details]
Patch
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. 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
Created attachment 376421 [details]
Patch
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 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 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 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 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 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 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 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! Created attachment 376453 [details]
Patch
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 Created attachment 376531 [details]
Patch
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 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" Created attachment 376533 [details]
Patch
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 on attachment 376533 [details] Patch Clearing flags on attachment: 376533 Committed r248798: <https://trac.webkit.org/changeset/248798> All reviewed patches have been landed. Closing bug. 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 I will skip those tests on debug. 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 Reopening to attach new patch. Created attachment 376726 [details]
Patch
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 Created attachment 376731 [details]
Patch
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 Created attachment 376732 [details]
Patch
Comment on attachment 376732 [details] Patch Clearing flags on attachment: 376732 Committed r248905: <https://trac.webkit.org/changeset/248905> All reviewed patches have been landed. Closing bug. |