Bug 128352 – ReallocatePropertyStorage in FTL

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

[Matthew Mirman]

patch forthcoming

[Matthew Mirman]

Created attachment 223487 [details]
ReallocatePropertyStorage in FTL patch

[Filip Pizlo]

Comment on attachment 223487 [details]
ReallocatePropertyStorage in FTL patch

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2666
> +            LBasicBlock failPath = FTL_NEW_BLOCK(m_out, ("ReallocatePropertyStorage failure"));

You don't need a crashing slow path here.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2710
> +                m_out.loadPtr(m_out.address(m_heaps.properties.atAnyNumber(), oldStorage, offset ));

I think it's better to use the style of access that GetByOffset uses. Ideally this would be like a look the repeatedly does something simar to an out-of-line GetByOffset followed by PutByOffset. The benefit is that this way you wouldn't be claiming to clobber the entire property storage abstract heap. Basically I want to see properties[number] rather than atAnyNunber. I know I suggested atAnyNumber previously but I was wrong.

[Matthew Mirman]

Created attachment 223743 [details]
ReallocatePropertyStorage in FTL patch 

Does not include svn file revision numbers.

[WebKit Commit Bot]

Attachment 223743 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2671:  Declaration has space between type name and * in Structure * previous  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2675:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2681:  Extra space after ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2681:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2682:  Extra space after ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2682:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2704:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2712:  Extra space after ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2712:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2714:  Extra space after ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2714:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2727:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2728:  Extra space before )  [whitespace/parens] [2]
Total errors found: 13 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Matthew Mirman]

Created attachment 223746 [details]
ReallocatePropertyStorage in FTL patch 

Fixes whitespace problems with parens.

[WebKit Commit Bot]

Attachment 223746 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2671:  Declaration has space between type name and * in Structure * previous  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2704:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Matthew Mirman]

Created attachment 223751 [details]
ReallocatePropertyStorage in FTL patch

One more patch which fixes whitespace.

[Filip Pizlo]

Comment on attachment 223751 [details]
ReallocatePropertyStorage in FTL patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223751&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);

Why is this here?

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2727
> +            ptrdiff_t off  = offsetRelativeToBase(prop.offset);
> +            LValue loaded = 
> +                m_out.loadPtr(m_out.address(m_heaps.properties[ident], oldStorage, -off));
> +            m_out.storePtr(loaded, m_out.address(m_heaps.properties[ident] , result, -off));

I think that the negation ("-off") is wrong.  offsetRelativeToBase() should already return an appropriately negated value.  Also, you shouldn't copy the inline properties, but this appears to do that.  Also, 'item' here shouldn't be used as an index into m_storageAccessData.  m_storageAccessData doesn't use indexes into property storage as an index.  We just add an entry into m_storageAccessData whenever we add a node that needs it; it has nothing to do with the properties of any particular type.

[Matthew Mirman]

Created attachment 223762 [details]
ReallocatePropertyStorage in FTL patch

testing a patch format.

[Filip Pizlo]

Comment on attachment 223762 [details]
ReallocatePropertyStorage in FTL patch

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.

[Matthew Mirman]

Created attachment 223769 [details]
Again, added ReallocatePropertyStorage

back to original loop.

[WebKit Commit Bot]

Comment on attachment 223769 [details]
Again, added ReallocatePropertyStorage

Clearing flags on attachment: 223769

Committed r163841: <http://trac.webkit.org/changeset/163841>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.