Bug 162000 – [DFG] Introduce IsCellWithType node and unify IsJSArray, IsRegExpObject and newly added IsProxyObject

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

After the generator patch & Map patch, isArray becomes hot function in ES6SampleBench's Basic.
Array.isArray is frequently used to distinguish between object and array. That should be handled in DFG.

[Yusuke Suzuki]

And this will pave the way for adding IsMap and IsSet DFG handling.

[Yusuke Suzuki]

Created attachment 289025 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 289030 [details]
Patch

[Yusuke Suzuki]

Created attachment 289032 [details]
Patch

[Saam Barati]

Comment on attachment 289032 [details]
Patch

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

> Source/JavaScriptCore/bytecode/SpeculatedType.h:59
> +static const SpeculatedType SpecMapObject          = 1ull << 16; // It's definitely a Map object (can it be a subclass? FIXME).

Oops. You can remove this FIXME I added.

> Source/JavaScriptCore/bytecode/SpeculatedType.h:60
> +static const SpeculatedType SpecSetObject          = 1ull << 17; // It's definitely a Set object (can it be a subclass? FIXME).

It can be a subclass. But the parenthetical isn't needed.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1792
> +        // FIXME: SpecArray & ArrayUse are potentially useful here.

Not sure what this is for. Can you elaborate and add a Bugzilla?

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1795
> +            if (node->child1()->shouldSpeculateRegExpObject()) {

This looks wrong.

[Saam Barati]

(In reply to comment #5)
> Comment on attachment 289032 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289032&action=review
> 
> > Source/JavaScriptCore/bytecode/SpeculatedType.h:59
> > +static const SpeculatedType SpecMapObject          = 1ull << 16; // It's definitely a Map object (can it be a subclass? FIXME).
> 
> Oops. You can remove this FIXME I added.
> 
> > Source/JavaScriptCore/bytecode/SpeculatedType.h:60
> > +static const SpeculatedType SpecSetObject          = 1ull << 17; // It's definitely a Set object (can it be a subclass? FIXME).
> 
> It can be a subclass. But the parenthetical isn't needed.
> 
> > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1792
> > +        // FIXME: SpecArray & ArrayUse are potentially useful here.
> 
> Not sure what this is for. Can you elaborate and add a Bugzilla?
I don't know why I wrote this. I know what this is for. You should implement
it or create a bug for it. 

> 
> > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1795
> > +            if (node->child1()->shouldSpeculateRegExpObject()) {
> 
> This looks wrong.

[Yusuke Suzuki]

Comment on attachment 289032 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/SpeculatedType.h:60
>> +static const SpeculatedType SpecSetObject          = 1ull << 17; // It's definitely a Set object (can it be a subclass? FIXME).
> 
> It can be a subclass. But the parenthetical isn't needed.

OK, dropped parentheses and FIXME.

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1792
>> +        // FIXME: SpecArray & ArrayUse are potentially useful here.
> 
> Not sure what this is for. Can you elaborate and add a Bugzilla?

Filed it in https://bugs.webkit.org/show_bug.cgi?id=162063.

>>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1795
>>> +            if (node->child1()->shouldSpeculateRegExpObject()) {
>> 
>> This looks wrong.
> 
> 

Oops, right. Fixed.

[Yusuke Suzuki]

Comment on attachment 289032 [details]
Patch

I'll implement ArrayUse first :) Later, I'll update this patch.

[Yusuke Suzuki]

Created attachment 289121 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 289157 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 289157 [details]
Patch

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

added notes.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:-1102
> -            // We don't have a SpeculatedType for Proxies yet so we can't do better at proving false.

NOTE: @isJSArray(proxy) returns false. So we don't need to consider about Proxy here, right?

[Filip Pizlo]

(In reply to comment #11)
> Comment on attachment 289157 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289157&action=review
> 
> added notes.
> 
> > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:-1102
> > -            // We don't have a SpeculatedType for Proxies yet so we can't do better at proving false.
> 
> NOTE: @isJSArray(proxy) returns false. So we don't need to consider about
> Proxy here, right?

Seems reasonable!

[Filip Pizlo]

Comment on attachment 289157 [details]
Patch

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

> Source/JavaScriptCore/builtins/ArrayConstructor.js:114
> +    return @isArraySlow(array);

I like this approach.

> Source/JavaScriptCore/builtins/BuiltinNames.h:129
> +    macro(isArraySlow) \

Man, we need an easier way of defining builtins and private functions.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1420
> +        case IsCellWithType: {

I like this opcode!

[Yusuke Suzuki]

Comment on attachment 289157 [details]
Patch

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

>> Source/JavaScriptCore/builtins/BuiltinNames.h:129
>> +    macro(isArraySlow) \
> 
> Man, we need an easier way of defining builtins and private functions.

Yeah, we should have a good way.
While @globalPrivate is good for defining private builtin JS functions, exposing private host functions is still a bit tricky.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:-437
> -#else

Oops, they are included to build WebKit in my environment. Dropped before landing.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:-467
> -#endif // __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200 && USE(APPLE_INTERNAL_SDK)

Ditto.

[Yusuke Suzuki]

Committed r206065: <http://trac.webkit.org/changeset/206065>

[Saam Barati]

Comment on attachment 289157 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1420
>> +        case IsCellWithType: {
> 
> I like this opcode!

Me too! I wonder if we should also unite all the byte codes that lower to this node to just be a is_cell_with_type opcode that takes a JSType as an argument. What do you think Yusuke?

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1160
>      dispatch(3)

We should probably assert someplace that these all have the same op length. (Maybe I missed if this is done somewhere.)

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1180
> +    traceExecution()

Style: indentation is off here.

[Yusuke Suzuki]

Comment on attachment 289157 [details]
Patch

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

Thanks!!

>>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1420
>>> +        case IsCellWithType: {
>> 
>> I like this opcode!
> 
> Me too! I wonder if we should also unite all the byte codes that lower to this node to just be a is_cell_with_type opcode that takes a JSType as an argument. What do you think Yusuke?

One potential problem of using is_cell_with_type opcode is that the type operand does not become constant value in machine code.
While baseline, DFG, and FTL can embed constant JSType into machine code, LLInt can not when we use is_cell_with_type. This is why I just adds new opcodes here conservatively.
But, maybe, I think it does not matter so much since it's the lowest tier. Trying this on the separate patch and measuring the performance seems nice.
Opened https://bugs.webkit.org/show_bug.cgi?id=162132.

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1160
>>      dispatch(3)
> 
> We should probably assert someplace that these all have the same op length. (Maybe I missed if this is done somewhere.)

Yeah! We should have that! I'll create the follow up patch.

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1180
>> +    traceExecution()
> 
> Style: indentation is off here.

It seems that this line has correct indentation, right?

[Saam Barati]

Comment on attachment 289157 [details]
Patch

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

>>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1180
>>> +    traceExecution()
>> 
>> Style: indentation is off here.
> 
> It seems that this line has correct indentation, right?

Yeah it does. I was reviewing this patch on my phone, so maybe it was just a bug in the patch viewer on my phone.