[Webkit-unassigned] [Bug 128352] ReallocatePropertyStorage in FTL

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=128352


Filip Pizlo <fpizlo at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #223762|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #10 from Filip Pizlo <fpizlo at apple.com>  2014-02-10 16:23:49 PST ---
(From update of attachment 223762)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.