[webkit-reviews] review denied: [Bug 67494] Use bump allocator for initial property storage : [Attachment 106773] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 67494: Use bump allocator for initial property storage
https://bugs.webkit.org/show_bug.cgi?id=67494

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

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


Looking a lot better, but still some work to do here.

> Source/JavaScriptCore/heap/Heap.cpp:693
> +    m_newSpace.resetPropertyStorageNursery();

Please move the bump pointer allocator into a new class in a follow-up patch.

> Source/JavaScriptCore/heap/Heap.h:74
> +	   void addToVisitSet(const JSCell*) { }

Oliver said he's going to remove this.

> Source/JavaScriptCore/heap/NewSpace.h:49
> +	   static const size_t PropertyStorageNurserySize = 1024 * 1024 * 4;

Please use MB.

> Source/JavaScriptCore/runtime/JSObject.cpp:609
> +	       // If the allocation is too large we don't fit in the nursery,
or
> +	       // if allocatePropertyStorage to triggers a GC that promotes us
> +	       // we'll need to allocate storage directly

Not so grammatical here. How about: "If allocation failed because it's too big,
or it triggered a GC that promoted us to old space, we need to allocate our
property storage in old space."

Eventually, I think we can remove some of this complexity by moving more
responsibility for backing store allocation into the heap, which knows more
about these details.

> Source/JavaScriptCore/runtime/JSObject.h:75
> +    class StorageBarrier {

This class should be in its own file. Also, the set function and the
constructor need to call Heap::writeBarrier() on the object taking ownership of
the storage.