Bug 159048 – We should have a DFG intrinsic that checks if a value is a TypedArrayView

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

We should have a DFG intrinsic that checks if a value is a TypedArrayView

[Keith Miller]

Created attachment 281887 [details]
Patch

[Keith Miller]

Created attachment 281888 [details]
Patch

[Saam Barati]

Comment on attachment 281888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281888&action=review

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1276
> +            if (!(child.m_type & ~SpecTypedArrayView)) {

Shouldn't this also check !!child.m_type?

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1281
> +            if (!(child.m_type & SpecTypedArrayView)) {

ditto

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6121
> +        m_out.branch(isCell(value, provenType(m_node->child1())), unsure(isCellCase), unsure(continuation));

I would guess the isCellCase is likely.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:707
> +        GlobalPropertyInfo(vm.propertyNames->builtinNames().isTypedArrayViewPrivateName(), privateFuncIsTypedArrayView, DontEnum | DontDelete | ReadOnly),

Where is this used from besides the test?

> Source/JavaScriptCore/runtime/JSTypedArrayViewPrototype.cpp:70
> +    return JSValue::encode(jsBoolean(value.isObject() && isTypedView(value.getObject()->classInfo()->typedArrayStorageType)));

I think this would be faster as:
jsBoolean(value.isCell() && isTypedView(value.asCell()->classInfo()->typedArrayStorageType))
because most cells you will see are going to be objects.

> Source/JavaScriptCore/tests/stress/istypedarrayview-intrinsic.js:4
> +    let foo = createBuiltin(`(function (a) { "use strict"; return @isTypedArrayView(a); })`);

Neat.

[Saam Barati]

Comment on attachment 281888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281888&action=review

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1281
>> +            if (!(child.m_type & SpecTypedArrayView)) {
> 
> ditto

ignore this ditto

[Keith Miller]

Comment on attachment 281888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281888&action=review

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1276
>> +            if (!(child.m_type & ~SpecTypedArrayView)) {
> 
> Shouldn't this also check !!child.m_type?

I'm pretty sure that we force OSR if the child has no speculated type.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6121
>> +        m_out.branch(isCell(value, provenType(m_node->child1())), unsure(isCellCase), unsure(continuation));
> 
> I would guess the isCellCase is likely.

While I agree with you in general, my thought process is that this node will be eliminated in most of the cases where the value is a TypedArray. In the cases where we can't eliminate the node I think its harder to claim the value is a usually a cell.

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:707
>> +        GlobalPropertyInfo(vm.propertyNames->builtinNames().isTypedArrayViewPrivateName(), privateFuncIsTypedArrayView, DontEnum | DontDelete | ReadOnly),
> 
> Where is this used from besides the test?

I'm going to use this when I move subarray to a builtin. Currently it is not used elsewhere.

>> Source/JavaScriptCore/runtime/JSTypedArrayViewPrototype.cpp:70
>> +    return JSValue::encode(jsBoolean(value.isObject() && isTypedView(value.getObject()->classInfo()->typedArrayStorageType)));
> 
> I think this would be faster as:
> jsBoolean(value.isCell() && isTypedView(value.asCell()->classInfo()->typedArrayStorageType))
> because most cells you will see are going to be objects.

Fixed.

[Keith Miller]

Created attachment 281892 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 281892 [details]
Patch for landing

Clearing flags on attachment: 281892

Committed r202363: <http://trac.webkit.org/changeset/202363>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.