[webkit-reviews] review denied: [Bug 76665] Use copying collector for out-of-line JSObject property storage : [Attachment 123213] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 76665: Use copying collector for out-of-line JSObject property storage
https://bugs.webkit.org/show_bug.cgi?id=76665

Attachment 123213: Patch
https://bugs.webkit.org/attachment.cgi?id=123213&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=123213&action=review


On the right track, but I think this could use another cut.

> Source/JavaScriptCore/runtime/JSObject.cpp:108
> +    if (storage) {

It shouldn't be possible for an object to have a NULL storage pointer. Please
remove this test.

> Source/JavaScriptCore/runtime/JSObject.cpp:113
> +	       visitor.copyAndAppend(reinterpret_cast<void**>(&storage),
thisObject->structure()->propertyStorageCapacity() *
sizeof(WriteBarrierBase<Unknown>), storage->slot(), storageSize);

It occurs to me now that it's a little awkward for copyAndAppend to take a
number of bytes as its argument -- would be more appropriate to take a
JSValue** and a count, since it assumes the contents are JSValue anyway (in
order to do marking).

> Source/JavaScriptCore/runtime/JSObject.cpp:663
> +	   newPropertyStorage[i].clear();

Instead of calling clear(), you should call operator new(NotNullTag, void*).
That's the efficient and semantically correct C++ way to say "I want a new
object in this location".