Bug 176345 – [DFG] PutByVal with Array::Generic is too generic

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

[Yusuke Suzuki]

Worth looking into it.
https://arewefastyet.com/#machine=29&view=single&suite=six-speed&subtest=object-assign-es5

[Yusuke Suzuki]

We should have some fixup to remove runtime branches.
For example,

PutByVal(CellUse, StringUse, UntypedUse).

[Yusuke Suzuki]

Created attachment 320257 [details]
Patch

[Saam Barati]

Comment on attachment 320257 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGOperations.cpp:151
> +        RELEASE_ASSERT(base->isObject());

what guarantees this? Inside Fixup, you speculate Cell, not object? If it's guaranteed, why don't you speculate object?

[Yusuke Suzuki]

Comment on attachment 320257 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:151
>> +        RELEASE_ASSERT(base->isObject());
> 
> what guarantees this? Inside Fixup, you speculate Cell, not object? If it's guaranteed, why don't you speculate object?

This is originally guaranteed in operationPutByVal generic functions too. This is because we emit `op_put_by_val` with direct flag only for objects.

[Filip Pizlo]

Comment on attachment 320257 [details]
Patch

Cool!

I think that we can make the Generic case even better by implementing it as an inline cache.  Then we can have baseline, DFG, and FTL share that inline cache.  And we could use it for better array access profiling.

[Yusuke Suzuki]

(In reply to Filip Pizlo from comment #5)
> Comment on attachment 320257 [details]
> Patch
> 
> Cool!
> 
> I think that we can make the Generic case even better by implementing it as
> an inline cache.  Then we can have baseline, DFG, and FTL share that inline
> cache.  And we could use it for better array access profiling.

It sounds nice. We already have IC for GetByVal if it sees only one String/Symbol Identifier. But adding this type of IC (String/Symbol specialization) could further improve performance in baseline, DFG, and FTL.

[Yusuke Suzuki]

Committed r221793: <http://trac.webkit.org/changeset/221793>

[Saam Barati]

Comment on attachment 320257 [details]
Patch

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

>>> Source/JavaScriptCore/dfg/DFGOperations.cpp:151
>>> +        RELEASE_ASSERT(base->isObject());
>> 
>> what guarantees this? Inside Fixup, you speculate Cell, not object? If it's guaranteed, why don't you speculate object?
> 
> This is originally guaranteed in operationPutByVal generic functions too. This is because we emit `op_put_by_val` with direct flag only for objects.

Right, but your patch looks like it’s calling this from non-direct case. Am I missing something?

[Yusuke Suzuki]

Comment on attachment 320257 [details]
Patch

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

>>>> Source/JavaScriptCore/dfg/DFGOperations.cpp:151
>>>> +        RELEASE_ASSERT(base->isObject());
>>> 
>>> what guarantees this? Inside Fixup, you speculate Cell, not object? If it's guaranteed, why don't you speculate object?
>> 
>> This is originally guaranteed in operationPutByVal generic functions too. This is because we emit `op_put_by_val` with direct flag only for objects.
> 
> Right, but your patch looks like it’s calling this from non-direct case. Am I missing something?

This "direct" template param is true only when it's used for PutByValDirect.

[Saam Barati]

Comment on attachment 320257 [details]
Patch

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

>>>>> Source/JavaScriptCore/dfg/DFGOperations.cpp:151
>>>>> +        RELEASE_ASSERT(base->isObject());
>>>> 
>>>> what guarantees this? Inside Fixup, you speculate Cell, not object? If it's guaranteed, why don't you speculate object?
>>> 
>>> This is originally guaranteed in operationPutByVal generic functions too. This is because we emit `op_put_by_val` with direct flag only for objects.
>> 
>> Right, but your patch looks like it’s calling this from non-direct case. Am I missing something?
> 
> This "direct" template param is true only when it's used for PutByValDirect.

Sorry I was completely misreading the code. This LGTM

[Radar WebKit Bug Importer]

<rdar://problem/34693626>