[webkit-reviews] review denied: [Bug 77761] Split MarkedSpace into destructor and destructor-free subspaces : [Attachment 125389] Trying to fix builds

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 77761: Split MarkedSpace into destructor and destructor-free subspaces
https://bugs.webkit.org/show_bug.cgi?id=77761

Attachment 125389: Trying to fix builds
https://bugs.webkit.org/attachment.cgi?id=125389&action=review

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


Patch looks correct. Some architecture comments below. Also, I'd like to see
results from those two micro benchmarks I mentioned by email.

> Source/JavaScriptCore/heap/MarkedSpace.h:95
> +    struct Subspace {
> +	   FixedArray<MarkedAllocator, preciseCount> preciseSizeClasses;
> +	   FixedArray<MarkedAllocator, impreciseCount> impreciseSizeClasses;
> +    };

Minor nit: You should rename all "size class" variables to "allocator" to match
your class renaming.

> Source/JavaScriptCore/runtime/JSObject.h:399
> +template<> inline void* allocateCell<JSFinalObject>(Heap& heap)

Let's not duplicate this function for each destructor-free class. Instead, you
can specialize allocateCell<T> based on class traits, much like hash traits:

template<class T, NeedsDestructorArg = NeedsDestructor<T>> allocateCell(...) 
{
    if (NeedsDestructorArg::value) { ... } else { ... }
}

// Write manual specializations for this struct template if you care about
non-clang compilers.
template<class T> struct NeedsDestructor {
    static bool value = !has_non_trivial_destructor(T);
};

This would also eliminate the need for class friendship and the
DestructorFreeAllocator helper. You can make both allocateWithDestructor and
allocateWithoutDestructor private, and make allocateCell<T> a friend.