Bug 210583 – [JSC] Use ensureStillAliveHere in FTL when content of storage should be kept alive

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Use ensureStillAliveHere in FTL when content of storage should be kept alive

[Yusuke Suzuki]

Created attachment 396598 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/61831515>

[Yusuke Suzuki]

Created attachment 396599 [details]
Patch

[Yusuke Suzuki]

Created attachment 396600 [details]
Patch

[Mark Lam]

Comment on attachment 396600 [details]
Patch

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4668
>                  break;

Let's change this break to a return so that it is clear that we're terminating here.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5770
>              break;

Can we change this to a return?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5778
>              break;

Ditto, change to return?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5784
>                  break;

Change to return to be consistent?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5791
>                  break;

Change to return?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5835
> +                // We have to keep base alive to keep content in storage alive.
> +                if (m_node->arrayMode().type() == Array::Contiguous)
> +                    ensureStillAliveHere(base);

Shouldn't this be after the load and store below since they use pointer, which points into storage?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5884
> +            // We have to keep base alive to keep content in storage alive.
> +            ensureStillAliveHere(base);
>              LValue result = m_out.load64(pointer);
>              m_out.branch(m_out.notZero64(result), usually(fastCase), rarely(slowCase));

We're branching to fastCase or slowCase after this.  fastCase stores and loads from storage.  Shouldn't we defer this ensureStillAliveHere() till after the last use of storage in the fastCase?  The slowCase is already using base in a call: so, I think that's got it covered.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8007
> +        // We have to keep base alive since that keeps content of storage alive.
> +        ensureStillAliveHere(lowCell(m_node->child2()));
>          setJSValue(loadProperty(
>              lowStorage(m_node->child1()), data.identifierNumber, data.offset));

Shouldn't the ensureStillAliveHere() come between the loadProperty() and the setJSValue()?

[Yusuke Suzuki]

Comment on attachment 396600 [details]
Patch

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

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4668
>>                  break;
> 
> Let's change this break to a return so that it is clear that we're terminating here.

Fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5770
>>              break;
> 
> Can we change this to a return?

Fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5778
>>              break;
> 
> Ditto, change to return?

Fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5784
>>                  break;
> 
> Change to return to be consistent?

Fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5791
>>                  break;
> 
> Change to return?

Fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5835
>> +                    ensureStillAliveHere(base);
> 
> Shouldn't this be after the load and store below since they use pointer, which points into storage?

It is OK to move this after loading for consistency, but practically, this does not matter.
The problem happens only when we get the following sequence.

ensure-still-alive-here-cell
GC
load-from-storage

But it is not possible to insert GC between ensure-still-alive-here-cell & load-from-storage since both are reading something the Heap & GC is invoked from CCall (which clobbers the Heap).
So, if you have

ensure-still-alive-here-cell
load-from-storage

sequence, any code motion cannot insert GC between them.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5884
>>              m_out.branch(m_out.notZero64(result), usually(fastCase), rarely(slowCase));
> 
> We're branching to fastCase or slowCase after this.  fastCase stores and loads from storage.  Shouldn't we defer this ensureStillAliveHere() till after the last use of storage in the fastCase?  The slowCase is already using base in a call: so, I think that's got it covered.

fastCase is loading m_heaps.ArrayStorage_numValuesInVector. And m_heaps.ArrayStorage_numValuesInVector is Int32. So this is not JSCell.
So, even if GC happens and GC does not mark the content of this ArrayStorage, loading m_heaps.ArrayStorage_numValuesInVector is OK.
And since ArrayStorage is AuxiliaryBuffer memory, ArrayStorage itself is kept alive if it is in stack/register (while the content of ArrayStorage is not marked by the owner cell if the owner cell is dead).

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8007
>>              lowStorage(m_node->child1()), data.identifierNumber, data.offset));
> 
> Shouldn't the ensureStillAliveHere() come between the loadProperty() and the setJSValue()?

It does not matter practically. But for consistency, I've just moved.

[Yusuke Suzuki]

Created attachment 396604 [details]
Patch

[Mark Lam]

Comment on attachment 396600 [details]
Patch

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

>>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5835
>>> +                    ensureStillAliveHere(base);
>> 
>> Shouldn't this be after the load and store below since they use pointer, which points into storage?
> 
> It is OK to move this after loading for consistency, but practically, this does not matter.
> The problem happens only when we get the following sequence.
> 
> ensure-still-alive-here-cell
> GC
> load-from-storage
> 
> But it is not possible to insert GC between ensure-still-alive-here-cell & load-from-storage since both are reading something the Heap & GC is invoked from CCall (which clobbers the Heap).
> So, if you have
> 
> ensure-still-alive-here-cell
> load-from-storage
> 
> sequence, any code motion cannot insert GC between them.

Good point.  I forgot about that.

[Mark Lam]

Comment on attachment 396600 [details]
Patch

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

>>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5884
>>>              m_out.branch(m_out.notZero64(result), usually(fastCase), rarely(slowCase));
>> 
>> We're branching to fastCase or slowCase after this.  fastCase stores and loads from storage.  Shouldn't we defer this ensureStillAliveHere() till after the last use of storage in the fastCase?  The slowCase is already using base in a call: so, I think that's got it covered.
> 
> fastCase is loading m_heaps.ArrayStorage_numValuesInVector. And m_heaps.ArrayStorage_numValuesInVector is Int32. So this is not JSCell.
> So, even if GC happens and GC does not mark the content of this ArrayStorage, loading m_heaps.ArrayStorage_numValuesInVector is OK.
> And since ArrayStorage is AuxiliaryBuffer memory, ArrayStorage itself is kept alive if it is in stack/register (while the content of ArrayStorage is not marked by the owner cell if the owner cell is dead).

After talking with Yusuke offline, I realized that I completely misunderstood the bug.  Just to clarify for others who may also read this patch: it's not the base object and it's butterfly that we're concerned about keeping alive.  It's the property object pointed to by the butterfly that we're loading.  We need to keep the base object alive so that its butterfly gets scanned, thereby keeping the property object alive until we load it into a register or stack location.  The register or stack location will keep it alive from that point on.

[Mark Lam]

Comment on attachment 396604 [details]
Patch

r=me

[Yusuke Suzuki]

Committed r260180: <https://trac.webkit.org/changeset/260180>