[webkit-reviews] review denied: [Bug 128352] ReallocatePropertyStorage in FTL : [Attachment 223762] ReallocatePropertyStorage in FTL patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Matthew Mirman <mmirman at apple.com>'s
request for review:
Bug 128352: ReallocatePropertyStorage in FTL
https://bugs.webkit.org/show_bug.cgi?id=128352

Attachment 223762: ReallocatePropertyStorage in FTL patch
https://bugs.webkit.org/attachment.cgi?id=223762&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=223762&action=review


> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2692
> +	       LBasicBlock failPath = FTL_NEW_BLOCK(m_out,
("ReallocatePropertyStorage failure"));
> +	       LBasicBlock continuation = FTL_NEW_BLOCK(m_out,
("ReallocatePropertyStorage continuation"));
> +	       
> +	       m_out.branch(m_out.isNull(result) , failPath , continuation);
> +	       
> +	       m_out.appendTo(failPath, continuation);
> +	       m_out.crash();
> +	       
> +	       m_out.appendTo(continuation);

I still don't understand this.	When will result ever be null?	Why is crashing
a good idea if it is?

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2725
> +	       ptrdiff_t off =
-offsetRelativeToBase(offsetForPropertyNumber(item,
previous->inlineCapacity()));
> +	       LValue loaded = 
> +		   m_out.loadPtr(m_out.address(m_heaps.properties[item],
oldStorage, off));
> +	       m_out.storePtr(loaded, m_out.address(m_heaps.properties[item] ,
result, off));

Ugh.  I think we - but mostly me - have totally confused ourselves here.  The
indexing of m_heaps.properties is the identifier index from the current
CodeBlock.  There is no practical way to manufacture an identifier index for
every property that we're copying, since we wouldn't already have a need for
all of those identifiers.  Imagine if you're in a function foo() that adds a
property 'f' to an object, causing a reallocation of the storage; but the
object already had properties "x", "y" and "z".  We *could* have the FTL create
identifiers for those in order to get identifier indices, but that seems like
too much work.	So, my original suggestion was actually right:
m_heaps.properties.atAnyNumber() is your friend.

But that still leaves the calculation that you're doing.  You're using 'item'
as a "property number" in the sense that the PropertyTable uses it.  That's not
what you have here.  You have something that is sort of an index into
out-of-line storage.  I think that you should probably just go with something
that matches the DFGSpeculativeJIT.cpp approach:

    for (ptrdiff_t offset = 0; offset < static_cast<ptrdiff_t>(oldSize); offset
+= sizeof(void*)) {
	m_jit.loadPtr(JITCompiler::Address(oldStorageGPR, -(offset +
sizeof(JSValue) + sizeof(void*))), scratchGPR2);
	m_jit.storePtr(scratchGPR2, JITCompiler::Address(scratchGPR1, -(offset
+ sizeof(JSValue) + sizeof(void*))));
    }

Where oldSize is your oldSize * sizeof(JSValue).

Ugh, I have a feeling that you did that in one of your patches and I
incorrectly asked you to change it.  Sorry, this code is always confusing
because our object model is confusing.