Bug 149687 – Add Intrinsic Getters and use them to fix performance on the getters of TypedArray properties.

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

[Keith Miller]

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.

[Keith Miller]

Created attachment 262561 [details]
Patch

[WebKit Commit Bot]

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.

[Keith Miller]

Created attachment 262564 [details]
Patch

[Keith Miller]

Forgot to fix the CMake build.

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Keith Miller]

Created attachment 262627 [details]
Patch

[WebKit Commit Bot]

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.

[Keith Miller]

Created attachment 262628 [details]
Patch

[WebKit Commit Bot]

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.

[Keith Miller]

Created attachment 262645 [details]
Patch

[WebKit Commit Bot]

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.

[Keith Miller]

> 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.

[Keith Miller]

Created attachment 262651 [details]
Perf Results

Perf results for the patch.

[Keith Miller]

> 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.

[Saam Barati]

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.

[Keith Miller]

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.

[Keith Miller]

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.

[Keith Miller]

Created attachment 262800 [details]
Patch

[Geoffrey Garen]

Comment on attachment 262800 [details]
Patch

Looks like this patch needs rebasing.

[Keith Miller]

Created attachment 263111 [details]
Patch

[WebKit Commit Bot]

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.

[Keith Miller]

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.

[Geoffrey Garen]

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.

[Geoffrey Garen]

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?

[Keith Miller]

(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.

[Keith Miller]

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.

[Keith Miller]

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.

[Keith Miller]

Committed r191215: <http://trac.webkit.org/changeset/191215>