Bug 204402 – [JSC] Remove JSFixedArray, and use JSImmutableButterfly instead

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

[Yusuke Suzuki]

We could remove unnecessary allocation :)

[Yusuke Suzuki]

Created attachment 384388 [details]
Patch

WIP

[EWS Watchlist]

Comment on attachment 384388 [details]
Patch

Attachment 384388 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13280401

New failing tests:
fast/custom-elements/custom-elements-reaction-queue-retains-js-wrapper.html
http/wpt/entries-api/interfaces.html
js/slow-stress/call-spread.html
js/arrowfunction-lexical-bind-arguments-strict.html
js/slow-stress/new-spread.html
js/regress-151279.html

[EWS Watchlist]

Created attachment 384389 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Yusuke Suzuki]

Created attachment 384398 [details]
Patch

[Yusuke Suzuki]

Created attachment 384399 [details]
Patch

[Yusuke Suzuki]

Created attachment 384401 [details]
Patch

[Yusuke Suzuki]

Created attachment 384402 [details]
Patch

[Yusuke Suzuki]

Created attachment 384407 [details]
Patch

[Yusuke Suzuki]

Created attachment 384653 [details]
Patch

[Keith Miller]

Are the failing Win tests related?

[Yusuke Suzuki]

(In reply to Keith Miller from comment #10)
> Are the failing Win tests related?

It is unrelated.

[Yusuke Suzuki]

Can I get review? The main purpose of this is reducing # of variable-sized cell kinds.

[Mark Lam]

Comment on attachment 384653 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2668
> +        setForNode(node, m_vm.immutableButterflyStructures[arrayIndexFromIndexingType(CopyOnWriteArrayWithContiguous) - NumberOfIndexingShapes].get());

Do we have to watch HavingABadTime to ensure that the structure doesn't change?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7926
> +        m_jit.emitAllocateVariableSizedCell<JSImmutableButterfly>(vm(), resultGPR, TrustedImmPtr(m_jit.graph().registerStructure(m_jit.graph().m_vm.immutableButterflyStructures[arrayIndexFromIndexingType(CopyOnWriteArrayWithContiguous) - NumberOfIndexingShapes].get())), scratch1GPR, scratch1GPR, scratch2GPR, slowPath);

Do we have to watch HavingABadTime?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6682
> +            // FIXME: JSImmutableButterfly::createFromArray should support non contiguous indexing types.

I suggest rephrasing this as:
// FIXME: JSImmutableButterfly::createFromArray should support re-using non contiguous indexing types as well.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6694
> +                m_graph.m_vm.immutableButterflyStructures[arrayIndexFromIndexingType(CopyOnWriteArrayWithContiguous) - NumberOfIndexingShapes].get(), slowAllocation);

Don't we have to watch HavingABadTime in order for this to be safe?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6742
> +            LValue fastArrayValue = allocateVariableSizedCell<JSImmutableButterfly>(size, m_graph.m_vm.immutableButterflyStructures[arrayIndexFromIndexingType(CopyOnWriteArrayWithContiguous) - NumberOfIndexingShapes].get(), slowAllocation);

Ditto.  Need to watch HavingABadTime?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6813
> +            LValue fastAllocation = allocateVariableSizedCell<JSImmutableButterfly>(size, m_graph.m_vm.immutableButterflyStructures[arrayIndexFromIndexingType(CopyOnWriteArrayWithContiguous) - NumberOfIndexingShapes].get(), slowPath);

Ditto.  Need to watch HavingABadTime?  I had previously presumed that m_graph.canDoFastSpread() guarantees that we're also watching for HavingABadTime, but upon reading its code, I don't see that.  Am I missing something?

> Source/JavaScriptCore/runtime/JSImmutableButterfly.h:78
> +        // FIXME: JSImmutableButterfly::createFromArray should support non contiguous indexing types.

I suggest rephrasing this as:
// FIXME: JSImmutableButterfly::createFromArray should support re-using non contiguous indexing types as well.

[Mark Lam]

Comment on attachment 384653 [details]
Patch

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

r=me

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2668
>> +        setForNode(node, m_vm.immutableButterflyStructures[arrayIndexFromIndexingType(CopyOnWriteArrayWithContiguous) - NumberOfIndexingShapes].get());
> 
> Do we have to watch HavingABadTime to ensure that the structure doesn't change?

Talked to Yusuke and Saam offline.  There's no HavingABadTime issue here because m_vm.immutableButterflyStructures does not change with HavingABadTime.  I conflated this with m_arrayStructureForIndexingShapeDuringAllocation.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7926
>> +        m_jit.emitAllocateVariableSizedCell<JSImmutableButterfly>(vm(), resultGPR, TrustedImmPtr(m_jit.graph().registerStructure(m_jit.graph().m_vm.immutableButterflyStructures[arrayIndexFromIndexingType(CopyOnWriteArrayWithContiguous) - NumberOfIndexingShapes].get())), scratch1GPR, scratch1GPR, scratch2GPR, slowPath);
> 
> Do we have to watch HavingABadTime?

Ditto.  No HavingABadTime issue here because m_vm.immutableButterflyStructures does not mutate with HavingABadTime.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6694
>> +                m_graph.m_vm.immutableButterflyStructures[arrayIndexFromIndexingType(CopyOnWriteArrayWithContiguous) - NumberOfIndexingShapes].get(), slowAllocation);
> 
> Don't we have to watch HavingABadTime in order for this to be safe?

There's no issue here because m_vm.immutableButterflyStructures does not mutate with HavingABadTime.  Also, the source we're reading from is an immutableButterfly which does not mutate with HavingABadTime.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6742
>> +            LValue fastArrayValue = allocateVariableSizedCell<JSImmutableButterfly>(size, m_graph.m_vm.immutableButterflyStructures[arrayIndexFromIndexingType(CopyOnWriteArrayWithContiguous) - NumberOfIndexingShapes].get(), slowAllocation);
> 
> Ditto.  Need to watch HavingABadTime?

There's no HavindABadTime issue here because PhantomCreateRest is only introduced in ArgumentsEliminationPhase, and ArgumentsEliminationPhase will only due this if the function isWatchingHavingABadTimeWatchpoint().

Please add an assertion here that isWatchingHavingABadTimeWatchpoint().

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6813
>> +            LValue fastAllocation = allocateVariableSizedCell<JSImmutableButterfly>(size, m_graph.m_vm.immutableButterflyStructures[arrayIndexFromIndexingType(CopyOnWriteArrayWithContiguous) - NumberOfIndexingShapes].get(), slowPath);
> 
> Ditto.  Need to watch HavingABadTime?  I had previously presumed that m_graph.canDoFastSpread() guarantees that we're also watching for HavingABadTime, but upon reading its code, I don't see that.  Am I missing something?

There's no HavingABadTime issue here because m_vm.immutableButterflyStructures is immutable.  Also, the loading of values below is fine because we already did the isOKIndexingType check above.

[Yusuke Suzuki]

Comment on attachment 384653 [details]
Patch

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

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6682
>> +            // FIXME: JSImmutableButterfly::createFromArray should support non contiguous indexing types.
> 
> I suggest rephrasing this as:
> // FIXME: JSImmutableButterfly::createFromArray should support re-using non contiguous indexing types as well.

OK, changed.

>>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6742
>>> +            LValue fastArrayValue = allocateVariableSizedCell<JSImmutableButterfly>(size, m_graph.m_vm.immutableButterflyStructures[arrayIndexFromIndexingType(CopyOnWriteArrayWithContiguous) - NumberOfIndexingShapes].get(), slowAllocation);
>> 
>> Ditto.  Need to watch HavingABadTime?
> 
> There's no HavindABadTime issue here because PhantomCreateRest is only introduced in ArgumentsEliminationPhase, and ArgumentsEliminationPhase will only due this if the function isWatchingHavingABadTimeWatchpoint().
> 
> Please add an assertion here that isWatchingHavingABadTimeWatchpoint().

Added assertion here.

>> Source/JavaScriptCore/runtime/JSImmutableButterfly.h:78
>> +        // FIXME: JSImmutableButterfly::createFromArray should support non contiguous indexing types.
> 
> I suggest rephrasing this as:
> // FIXME: JSImmutableButterfly::createFromArray should support re-using non contiguous indexing types as well.

Fixed.

[Yusuke Suzuki]

Committed r253520: <https://trac.webkit.org/changeset/253520>

[Radar WebKit Bug Importer]

<rdar://problem/57933969>