[webkit-reviews] review denied: [Bug 128352] ReallocatePropertyStorage in FTL : [Attachment 223487] 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 223487: ReallocatePropertyStorage in FTL patch
https://bugs.webkit.org/attachment.cgi?id=223487&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
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.