...
Created attachment 308882 [details] WIP I need to teach the JITs that put_by_val_direct on a typed array is not optimizable in the same way as put_by_val. We currently do the same optimization in baseline/DFG regardless of direct or not.
<rdar://problem/31735695>
Created attachment 308938 [details] WIP Might be done. Want to run on EWS.
Comment on attachment 308938 [details] WIP Attachment 308938 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/3666318 New failing tests: jsc-layout-tests.yaml/js/script-tests/class-syntax-string-and-numeric-names.js.layout-no-llint stress/array-species-config-array-constructor.js.ftl-eager-no-cjit-b3o1 stress/array-species-config-array-constructor.js.ftl-no-cjit-validate-sampling-profiler stress/array-species-config-array-constructor.js.no-cjit-collect-continuously jsc-layout-tests.yaml/js/script-tests/class-syntax-string-and-numeric-names.js.layout-ftl-eager-no-cjit stress/array-species-config-array-constructor.js.ftl-no-cjit-no-put-stack-validate jsc-layout-tests.yaml/js/script-tests/class-syntax-string-and-numeric-names.js.layout-dfg-eager-no-cjit stress/array-species-config-array-constructor.js.ftl-eager jsc-layout-tests.yaml/js/script-tests/class-syntax-string-and-numeric-names.js.layout-ftl-no-cjit stress/array-species-config-array-constructor.js.no-cjit-validate-phases jsc-layout-tests.yaml/js/script-tests/class-syntax-string-and-numeric-names.js.layout-no-ftl jsc-layout-tests.yaml/js/script-tests/class-syntax-string-and-numeric-names.js.layout stress/array-species-config-array-constructor.js.default stress/array-species-config-array-constructor.js.no-llint stress/array-species-config-array-constructor.js.ftl-eager-no-cjit stress/array-species-config-array-constructor.js.ftl-no-cjit-no-inline-validate stress/array-species-config-array-constructor.js.ftl-no-cjit-b3o1 stress/array-species-config-array-constructor.js.ftl-no-cjit-small-pool jsc-layout-tests.yaml/js/script-tests/class-syntax-string-and-numeric-names.js.layout-no-cjit stress/array-species-config-array-constructor.js.dfg-eager stress/array-species-config-array-constructor.js.dfg-maximal-flush-validate-no-cjit stress/array-species-config-array-constructor.js.no-ftl stress/array-species-config-array-constructor.js.dfg-eager-no-cjit-validate
Comment on attachment 308938 [details] WIP Attachment 308938 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3666401 New failing tests: js/class-syntax-string-and-numeric-names.html
Created attachment 308945 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 308938 [details] WIP Attachment 308938 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3666409 New failing tests: js/class-syntax-string-and-numeric-names.html
Created attachment 308946 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 308938 [details] WIP Attachment 308938 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3666415 New failing tests: js/class-syntax-string-and-numeric-names.html
Created attachment 308948 [details] Archive of layout-test-results from ews116 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 308938 [details] WIP Attachment 308938 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3666440 New failing tests: js/class-syntax-string-and-numeric-names.html
Created attachment 308951 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
*** Bug 159645 has been marked as a duplicate of this bug. ***
Created attachment 308984 [details] patch
Comment on attachment 308984 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=308984&action=review > Source/JavaScriptCore/ChangeLog:14 > + defineProperty({configurable:true, enumerable:true, writable:true}), however, Run on sentence here. I suggest putting a period before "however", and starting "However" as a new sentence. > Source/JavaScriptCore/ChangeLog:26 > + This patch fixes this issue by adding a whitelist of cell types that can > + go down putDirectIndex's fast path. Anything not in that whitelist will > + simply call into defineOwnProperty. Did you benchmark this to make sure there's no perf impact due to checking the whitelist? > Source/JavaScriptCore/dfg/DFGArrayMode.cpp:251 > + return typedArrayResult(withSpeculation(Array::InBounds)); why not just fall thru to the case at line 253? They do the same thing. > Source/JavaScriptCore/jit/JITOperations.cpp:-590 > - if (baseObject->canSetIndexQuicklyForPutDirect(index)) { > - baseObject->setIndexQuickly(callFrame->vm(), index, value); > - return; > - } Why remove this? > Source/JavaScriptCore/jit/JITOperations.cpp:589 > // FIXME: This will make us think that in-bounds typed array accesses are actually > // out-of-bounds. Is this still relevant? > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:-1042 > - result->putByIndexInline(exec, k, v, true); if !canSetIndexQuickly(), putByIndexInline() will go thru the generic methodTable(exec->vm())->putByIndex(...) route i.e. can be overridden by Proxy, no? However, putDirectIndex() below will not route thru the methodTable's putByIndex. I see that you've changed putDirectIndexSlowOrBeyondVectorLength to route thru methodTable(vm)->defineOwnProperty() as needed. But will this result in an observable difference in behavior? If so, is it still spec compliant? > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:399 > + return typeError(exec, scope, shouldThrow, makeString("Attempting to store accessor property on a typed array at index: ", String::number(*index))); Would this end up making the error string even if we don't need to throw? If so, maybe you should change typeError() to a be a vararg template and just forward the construction cost to the throw case only.
Comment on attachment 308984 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=308984&action=review Thanks for taking a look. Comments below. >> Source/JavaScriptCore/ChangeLog:14 >> + defineProperty({configurable:true, enumerable:true, writable:true}), however, > > Run on sentence here. I suggest putting a period before "however", and starting "However" as a new sentence. sounds good >> Source/JavaScriptCore/ChangeLog:26 >> + simply call into defineOwnProperty. > > Did you benchmark this to make sure there's no perf impact due to checking the whitelist? No. Will do before landing. >> Source/JavaScriptCore/dfg/DFGArrayMode.cpp:251 >> + return typedArrayResult(withSpeculation(Array::InBounds)); > > why not just fall thru to the case at line 253? They do the same thing. sounds good >> Source/JavaScriptCore/jit/JITOperations.cpp:-590 >> - } > > Why remove this? It's wrong. See that I removed this function for this exact reason. It should be internal to putDirectIndex. >> Source/JavaScriptCore/jit/JITOperations.cpp:589 >> // out-of-bounds. > > Is this still relevant? Yeah it is. I think the repro situation is something like this: index is below vector length, but above public length. >> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:-1042 >> - result->putByIndexInline(exec, k, v, true); > > if !canSetIndexQuickly(), putByIndexInline() will go thru the generic methodTable(exec->vm())->putByIndex(...) route i.e. can be overridden by Proxy, no? However, putDirectIndex() below will not route thru the methodTable's putByIndex. I see that you've changed putDirectIndexSlowOrBeyondVectorLength to route thru methodTable(vm)->defineOwnProperty() as needed. But will this result in an observable difference in behavior? If so, is it still spec compliant? putByIndex does Set(). putDirectIndex does defineOwnProperty. It's not spec compliant to have this to do Set(). It should've always been doing defineOwnProperty. One of my tests catches this exact case. putDirectIndex will go through the method table's defineOwnProperty (that is what the heart of this patch does). >> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:399 >> + return typeError(exec, scope, shouldThrow, makeString("Attempting to store accessor property on a typed array at index: ", String::number(*index))); > > Would this end up making the error string even if we don't need to throw? If so, maybe you should change typeError() to a be a vararg template and just forward the construction cost to the throw case only. Yeah I thought about doing something like this. It probably doesn't matter in practice, but something like your suggestion could work.
Comment on attachment 308984 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=308984&action=review >> Source/JavaScriptCore/jit/JITOperations.cpp:-590 >> - } > > Why remove this? But I think there is a different bug here. Doesn't this mean that we will think anything code that does a putDirect with an uint32 accesses out of bounds? I think before we would have only thought it went out of bounds if it went past the vector length. >> Source/JavaScriptCore/jit/JITOperations.cpp:589 >> // out-of-bounds. > > Is this still relevant? Yeah, this still happens.
(In reply to Keith Miller from comment #17) > Comment on attachment 308984 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=308984&action=review > > >> Source/JavaScriptCore/jit/JITOperations.cpp:-590 > >> - } > > > > Why remove this? > > But I think there is a different bug here. Doesn't this mean that we will > think anything code that does a putDirect with an uint32 accesses out of > bounds? > > I think before we would have only thought it went out of bounds if it went > past the vector length. Yeah I missed this. Perf tests should probably pick this up. I'll properly only set out of bounds when we would have used to. Maybe I'll rename the old function. > > >> Source/JavaScriptCore/jit/JITOperations.cpp:589 > >> // out-of-bounds. > > > > Is this still relevant? > > Yeah, this still happens.
Comment on attachment 308984 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=308984&action=review >>>>> Source/JavaScriptCore/jit/JITOperations.cpp:589 >>>>> // out-of-bounds. >>>> >>>> Is this still relevant? >>> >>> Yeah it is. I think the repro situation is something like this: index is below vector length, but above public length. >> >> Yeah, this still happens. > > Sorry, you're correct, this comment is no longer relevant. I'm just going to switch the code to manually switch on indexing type.
Created attachment 309017 [details] patch Going to run benchmarks now.
Created attachment 309020 [details] benchmark results looks neutral
Comment on attachment 309017 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=309017&action=review > Source/JavaScriptCore/jit/JITOperations.cpp:-594 > - // FIXME: This will make us think that in-bounds typed array accesses are actually > - // out-of-bounds. > - // https://bugs.webkit.org/show_bug.cgi?id=149886 I don't understand how this is not relevant anymore. Don't all typed arrays have an Undecided indexing type? If so, wouldn't they always set out of bounds?
(In reply to Keith Miller from comment #22) > Comment on attachment 309017 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=309017&action=review > > > Source/JavaScriptCore/jit/JITOperations.cpp:-594 > > - // FIXME: This will make us think that in-bounds typed array accesses are actually > > - // out-of-bounds. > > - // https://bugs.webkit.org/show_bug.cgi?id=149886 > > I don't understand how this is not relevant anymore. Don't all typed arrays > have an Undecided indexing type? If so, wouldn't they always set out of > bounds? You're correct that we will mark byValInfo for PutDirect on a typed array as OOB. However, this won't matter in terms of perf impact. We would never optimize this to anything smart in the JIT since we don't model PutDirect to typed arrays in the JIT. (I should actually add an assertion for this in the DFG backend and FTLLower.)
Comment on attachment 309017 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=309017&action=review r=me > Source/JavaScriptCore/bytecode/ByValInfo.h:180 > + default: > + return false; This default includes JITDirectArguments and JITScopedArguments, but your comment only covers typed arrays. Was it intentional to include JITDirectArguments and JITScopedArguments in forbidding PutDirect? If so, I would mention that, and why.
Created attachment 309184 [details] patch for landing
Comment on attachment 309184 [details] patch for landing Clearing flags on attachment: 309184 Committed r216279: <http://trac.webkit.org/changeset/216279>
All reviewed patches have been landed. Closing bug.
Comment on attachment 309184 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=309184&action=review > JSTests/stress/put-direct-index-broken-2.js:247 > +function assert(b) { > + if (!b) > + throw new Error("Bad assertion"); > +} > + > +function test(f) { > + for (let i = 0; i < 1000; ++i) { > + f(); > + } > +} > + > +let __oldDesc = null; > +let __localLength; > +function makeLengthWritable() { > + assert(__oldDesc === null); > + __oldDesc = Object.getOwnPropertyDescriptor(Uint8ClampedArray.prototype.__proto__, "length"); > + assert(typeof __oldDesc.get === "function"); > + Reflect.defineProperty(Uint8ClampedArray.prototype.__proto__, "length", {configurable:true, get() { return __localLength; }, set(x) { __localLength = x; }}); > +} > + > +function restoreOldDesc() { > + assert(__oldDesc !== null); > + Reflect.defineProperty(Uint8ClampedArray.prototype.__proto__, "length", __oldDesc); > + __oldDesc = null; > +} > + > +test(function() { > + "use strict"; > + let a = []; > + a.push(300); > + a.length = 4277; > + > + let x = new Uint8ClampedArray; > + a.__proto__ = x; > + let err = null; > + try { > + let y = Array.prototype.map.call(a, x => x); > + } catch(e) { > + err = e; > + } > + assert(!!err); > + assert(err.toString() === "TypeError: Attempting to configure non-configurable property on a typed array at index: 0"); > +}); > + > +test(function() { > + let a = []; > + for (let i = 0; i < 100; i++) { > + a.push(i); > + } > + a.length = 4277; > + let x = new Uint8ClampedArray; > + a.__proto__ = x; > + let err = null; > + try { > + let y = Array.prototype.filter.call(a, x => true); > + } catch(e) { > + err = e; > + } > + assert(err.toString() === "TypeError: Attempting to configure non-configurable property on a typed array at index: 0"); > +}); > + > +test(function() { > + let a = []; > + for (let i = 0; i < 100; i++) { > + a.push(i); > + } > + a.length = 4277; > + let x = new Uint8ClampedArray; > + a.__proto__ = x; > + let err = null; > + let y = Array.prototype.filter.call(a, x => false); > + assert(y instanceof Uint8ClampedArray); > +}); > + > +test(function() { > + let a = []; > + for (let i = 0; i < 100; i++) { > + a.push(i); > + } > + a.length = 4277; > + let x = new Uint8ClampedArray; > + a.__proto__ = x; > + > + let err = null; > + try { > + let y = Array.prototype.slice.call(a, 0); > + } catch(e) { > + err = e; > + } > + assert(err.toString() === "TypeError: Attempting to configure non-configurable property on a typed array at index: 0"); > +}); > + > +test(function() { > + let a = []; > + for (let i = 0; i < 100; i++) { > + a.push(i); > + } > + a.length = 4277; > + let x = new Uint8ClampedArray; > + a.__proto__ = x; > + > + makeLengthWritable(); > + let y = Array.prototype.slice.call(a, 100); > + assert(y.length === 4277 - 100); > + assert(y.length === __localLength); > + assert(y instanceof Uint8ClampedArray); > + restoreOldDesc(); > +}); > + > +test(function() { > + let a = []; > + for (let i = 0; i < 100; i++) { > + a.push(i); > + } > + a.length = 4277; > + let x = new Uint8ClampedArray; > + a.__proto__ = x; > + > + makeLengthWritable(); > + let y = Array.prototype.splice.call(a); > + assert(y.length === __localLength); > + assert(y.length === 0); > + restoreOldDesc(); > +}); > + > +test(function() { > + let a = []; > + for (let i = 0; i < 100; i++) { > + a.push(i); > + } > + a.length = 4277; > + let x = new Uint8ClampedArray; > + a.__proto__ = x; > + > + let err = null; > + try { > + let y = Array.prototype.splice.call(a, 0); > + } catch(e) { > + err = e; > + } > + assert(err.toString() === "TypeError: Attempting to configure non-configurable property on a typed array at index: 0"); > +}); > + > +test(function() { > + let a = []; > + for (let i = 0; i < 100; i++) { > + a.push(i); > + } > + a.length = 4277; > + let x = new Uint8ClampedArray; > + a.__proto__ = x; > + > + makeLengthWritable(); > + let y = Array.prototype.slice.call(a, 100); > + assert(y.length === 4277 - 100); > + assert(y.length === __localLength); > + assert(y instanceof Uint8ClampedArray); > + restoreOldDesc(); > +}); > + > +test(function() { > + let a = []; > + for (let i = 0; i < 100; i++) { > + a.push(i); > + } > + a.length = 4277; > + let calls = 0; > + let target = {}; > + a.__proto__ = { > + constructor: { > + [Symbol.species]: function(length) { > + assert(length === 4277) > + return new Proxy(target, { > + defineProperty(...args) { > + ++calls; > + return Reflect.defineProperty(...args); > + } > + }); > + } > + } > + }; > + let y = Array.prototype.map.call(a, x => x); > + assert(calls === 100); > + for (let i = 0; i < 100; ++i) > + assert(target[i] === i); > +}); > + > +test(function() { > + let a = []; > + for (let i = 0; i < 100; i++) { > + a.push(i); > + } > + a.length = 4277; > + let calls = 0; > + let target = {}; > + a.__proto__ = { > + constructor: { > + [Symbol.species]: function(length) { > + assert(length === 0) > + return new Proxy(target, { > + defineProperty(...args) { > + ++calls; > + return Reflect.defineProperty(...args); > + } > + }); > + } > + } > + }; > + let y = Array.prototype.filter.call(a, x => true); > + assert(calls === 100); > + for (let i = 0; i < 100; ++i) > + assert(target[i] === i); > +}); > + > +test(function() { > + let a = []; > + for (let i = 0; i < 100; i++) { > + a.push(i); > + } > + a.length = 4277; > + let calls = 0; > + let target = {}; > + let keys = []; > + a.__proto__ = { > + constructor: { > + [Symbol.species]: function(length) { > + assert(length === 4277) > + return new Proxy(target, { > + defineProperty(...args) { > + keys.push(args[1]) > + ++calls; > + return Reflect.defineProperty(...args); > + } > + }); > + } > + } > + }; > + let y = Array.prototype.slice.call(a, 0); > + assert(calls === 101); // length gets defined too. > + assert(keys.length === 101); > + for (let i = 0; i < 100; ++i) { > + assert(parseInt(keys[i]) === i); > + assert(target[i] === i); > + } > + assert(keys[keys.length - 1] === "length"); > + assert(target.length === 4277); > +}); This test is super slow. Is there a way to write such tests so that they aren't super slow? For example: maybe instead of having each test simply run 1000 times, have just the part of the test that needs to warm up run 1000 times.