In ES6 the TypedArray properties length, byteLength, and byteOffset are not own properties any longer. Moving them to the prototype chain caused performance regressions. Thus, we needed to add intrinsic getters to fix this problem. Now when we add a case to an inline cache on a GetById we will observe the getter we are caching is an intrinsic. Later, when we are parsing code for the dfg we observe that we have an access case for an intrinsic getter and convert the GetById operation into one for the intrinsic.
Created attachment 262561 [details] Patch
Attachment 262561 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/GetByIdVariant.cpp:44: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2901: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2902: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2902: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 4 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 262564 [details] Patch
Forgot to fix the CMake build.
Attachment 262564 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/GetByIdVariant.cpp:44: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2901: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2902: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2902: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 4 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 262564 [details] Patch Attachment 262564 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/252265 New failing tests: js/dom/getOwnPropertyDescriptor.html fast/canvas/webgl/arraybuffer-transfer-of-control.html fast/canvas/webgl/array-message-passing.html webgl/1.0.2/conformance/typedarrays/data-view-test.html fast/canvas/webgl/data-view-test.html
Created attachment 262570 [details] Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 262564 [details] Patch Attachment 262564 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/252267 New failing tests: js/dom/getOwnPropertyDescriptor.html fast/canvas/webgl/arraybuffer-transfer-of-control.html fast/canvas/webgl/array-message-passing.html webgl/1.0.2/conformance/typedarrays/data-view-test.html fast/canvas/webgl/data-view-test.html
Created attachment 262571 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 262627 [details] Patch
Attachment 262627 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/GetByIdVariant.cpp:44: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2901: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2902: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2902: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 4 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 262628 [details] Patch
Attachment 262628 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/GetByIdVariant.cpp:44: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2901: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2902: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2902: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 4 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 262645 [details] Patch
Attachment 262645 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/GetByIdVariant.cpp:44: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2901: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2902: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2902: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 4 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.
> ERROR: Source/JavaScriptCore/bytecode/GetByIdVariant.cpp:44: Wrong number > of spaces before statement. (expected: 8) [whitespace/indent] [4] > ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2901: Multi line > control clauses should use braces. [whitespace/braces] [4] > ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2902: Place brace on > its own line for function definitions. [whitespace/braces] [4] This is appears to be the style for lambdas throughout DFGByteCodeParser.cpp. > ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2902: When wrapping > a line, only indent 4 spaces. [whitespace/indent] [3] Other than the third case these are just bugs in the style checker.
Created attachment 262651 [details] Perf Results Perf results for the patch.
> Perf results for the patch. A bunch of the set micro-benchmarks appear to regress in this patch. This is because we run the slow path for the length property access on a typed array inside the set function. This seems minor, it just happens to hit this micro-benchmark hard since set is the main work loop and the arrays are small.
Comment on attachment 262645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262645&action=review LGTM. Some parts I didn't look over very carefully. I just have a few nits. > Source/JavaScriptCore/bytecode/PolymorphicAccess.h:331 > +struct AccessGenerationState { Why'd you move this into the header? > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2901 > + if (handleIntrinsicGetter(destinationOperand, variant, base, Style: I think this would be easier to read if you didn't have it inside the if condition. Maybe just store it to a local. > Source/JavaScriptCore/jit/IntrinsicEmitter.cpp:1 > +/* Is it worth just having the contents of this file inside PolymorphicAccess? If not, I think it's better for it to be in /bytecode along with PolymorphicAccess > Source/JavaScriptCore/jit/IntrinsicEmitter.cpp:92 > + style: no newline > Source/JavaScriptCore/jit/IntrinsicEmitter.cpp:146 > + style: no newline. > Source/JavaScriptCore/jit/Repatch.cpp:240 > + getter = jsDynamicCast<JSFunction*>(slot.getterSetter()->getter()); Does this dynamic cast every fail? > Source/JavaScriptCore/runtime/JSFunction.h:91 > + Style: no newline.
Comment on attachment 262645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262645&action=review >> Source/JavaScriptCore/bytecode/PolymorphicAccess.h:331 >> +struct AccessGenerationState { > > Why'd you move this into the header? The AccessGenerationState is used for some of the emitter code so I had to move it to the header. >> Source/JavaScriptCore/jit/IntrinsicEmitter.cpp:1 >> +/* > > Is it worth just having the contents of this file inside PolymorphicAccess? > If not, I think it's better for it to be in /bytecode along with PolymorphicAccess I put it in a separate file since I expected there to be a growing number of intrinsics and I didn't want it to bloat PolymorphicAccess. I'm not fundamentally opposed to moving into PolymorphicAccess. I agree it should be in bytecode, however.
Comment on attachment 262645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262645&action=review >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2901 >> + if (handleIntrinsicGetter(destinationOperand, variant, base, > > Style: I think this would be easier to read if you didn't have it inside the if condition. Maybe just store it to a local. I doesn't really bother me too much and this is the way it's done throughout the file. If you feel strongly about it, however, I'm not super opposed to changing it. >> Source/JavaScriptCore/jit/IntrinsicEmitter.cpp:92 >> + > > style: no newline fixed. >> Source/JavaScriptCore/jit/IntrinsicEmitter.cpp:146 >> + > > style: no newline. fixed. >> Source/JavaScriptCore/jit/Repatch.cpp:240 >> + getter = jsDynamicCast<JSFunction*>(slot.getterSetter()->getter()); > > Does this dynamic cast every fail? I believe you are right and this can't fail. fixed. >> Source/JavaScriptCore/runtime/JSFunction.h:91 >> + > > Style: no newline. fixed.
Created attachment 262800 [details] Patch
Comment on attachment 262800 [details] Patch Looks like this patch needs rebasing.
Created attachment 263111 [details] Patch
Attachment 263111 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/PolymorphicAccess.h:345: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/PolymorphicAccess.h:346: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/PolymorphicAccess.h:347: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/GetByIdVariant.cpp:44: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2901: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2902: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2902: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 7 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 262645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262645&action=review >>> Source/JavaScriptCore/jit/Repatch.cpp:240 >>> + getter = jsDynamicCast<JSFunction*>(slot.getterSetter()->getter()); >> >> Does this dynamic cast every fail? > > I believe you are right and this can't fail. fixed. Nevermind this can definitely fail. If the getter is a subclass of InternalFunction then this cast will fail. For instance, this will happen if there is no getter function then a special NullGetter function will be set that extends InternalFunction.
This looks significant: Int16Array-to-Int32Array-set 59.0609+-6.9361 ! 123.5749+-2.4797 ! definitely 2.0923x slower Float32Array-to-Float64Array-set 63.8634+-0.9517 ! 128.4263+-0.7322 ! definitely 2.0110x slower Float64Array-to-Int16Array-set 76.3625+-1.1864 ! 145.7946+-3.0086 ! definitely 1.9092x slower Int16Array-to-Int32Array-set 59.0609+-6.9361 ! 123.5749+-2.4797 ! definitely 2.0923x slower Perhaps your explanation that we're paying the cost of a getter inside set() is correct. You can test your theory by changing set() to call thisObject->length() instead of doing a generic [[Get]] of length, since the ES6 spec and the other typed array functions specify use of the [[ArrayLength]] internal slot. But these objects also tax .length heavily in JS, so it's possible that you have a regression in .length access.
Comment on attachment 263111 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263111&action=review Code looks good to me. Please resolve the set() benchmark regression(s) before landing. > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1075 > + // are immutable so just need to watch the property not any value inside it. "we just need" > Source/JavaScriptCore/dfg/DFGClobberize.h:578 > + // We should not get an AnyTypedArray in a GetByVal. This comment doesn't add anything not already said by the code. You should either remove this comment or change it to explain *why* we're sure not to get an AnyTypedArray here. > Source/JavaScriptCore/dfg/DFGClobberize.h:-826 > - case Array::DirectArguments: > - case Array::ScopedArguments: > - read(MiscFields); > - def(HeapLocation(ArrayLengthLoc, MiscFields, node->child1()), LazyNode(node)); > - return; Why did this code disappear?
(In reply to comment #27) > This looks significant: > > Int16Array-to-Int32Array-set 59.0609+-6.9361 ! > 123.5749+-2.4797 ! definitely 2.0923x slower > Float32Array-to-Float64Array-set 63.8634+-0.9517 ! > 128.4263+-0.7322 ! definitely 2.0110x slower > Float64Array-to-Int16Array-set 76.3625+-1.1864 ! > 145.7946+-3.0086 ! definitely 1.9092x slower > Int16Array-to-Int32Array-set 59.0609+-6.9361 ! > 123.5749+-2.4797 ! definitely 2.0923x slower > > Perhaps your explanation that we're paying the cost of a getter inside set() > is correct. You can test your theory by changing set() to call > thisObject->length() instead of doing a generic [[Get]] of length, since the > ES6 spec and the other typed array functions specify use of the > [[ArrayLength]] internal slot. > > But these objects also tax .length heavily in JS, so it's possible that you > have a regression in .length access. I don't believe the regression is an issue with .length in JS as the performance regression disappears when the calls to set are replaced with other calls. However, I looked at the spec again and you are correct; the set function should not do a [[get]] on the length for typed arrays.
Comment on attachment 263111 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263111&action=review >> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1075 >> + // are immutable so just need to watch the property not any value inside it. > > "we just need" Fixed. >> Source/JavaScriptCore/dfg/DFGClobberize.h:-826 >> - return; > > Why did this code disappear? I deleted it because the ASSERT(mode.typedArrayType() != NotTypedArray) below is invalid as the array mode can be AnyTypedArray which does not have a TypedArrayType. However, the assert can be replaced with an ASSERT(mode.isSomeTypedArrayView()). So I will undo this change.
Created attachment 263306 [details] Perf Results updated set I added the set changes in r150216 and the set performance issues went away. Uploading results for posterity.
Committed r191215: <http://trac.webkit.org/changeset/191215>