[webkit-reviews] review granted: [Bug 204402] [JSC] Remove JSFixedArray, and use JSImmutableButterfly instead : [Attachment 384653] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 204402: [JSC] Remove JSFixedArray, and use JSImmutableButterfly instead
https://bugs.webkit.org/show_bug.cgi?id=204402

Attachment 384653: Patch

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




--- Comment #14 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 384653
  --> https://bugs.webkit.org/attachment.cgi?id=384653
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(CopyOnWriteArrayWi
thContiguous) - 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.immutableButte
rflyStructures[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(CopyOnWrit
eArrayWithContiguous) - 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(CopyOnWrit
eArrayWithContiguous) - 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(CopyOnWrit
eArrayWithContiguous) - 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.