[webkit-reviews] review granted: [Bug 179288] GC should support isoheaps : [Attachment 327580] the patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 179288: GC should support isoheaps
https://bugs.webkit.org/show_bug.cgi?id=179288

Attachment 327580: the patch

https://bugs.webkit.org/attachment.cgi?id=327580&action=review




--- Comment #20 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 327580
  --> https://bugs.webkit.org/attachment.cgi?id=327580
the patch

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

r=me
My main comment is about your use of AllocationFailureMode inside allocatorFor
and allocatorForNonVirtual. It feels like a weird use since it can return
nullptr.

> Source/JavaScriptCore/heap/CompleteSubspace.cpp:83
> +    // enough: it will have 

unfinished thought here (I know this is a copied comment, but we might as well
clean it up now)

> Source/JavaScriptCore/heap/CompleteSubspace.cpp:126
> +    if (MarkedAllocator* allocator = allocatorFor(size,
AllocationFailureMode::Assert))
> +	   return allocator->allocate(deferralContext,
AllocationFailureMode::ReturnNull);

Ditto about weird use for AllocationFailureMode

> Source/JavaScriptCore/heap/CompleteSubspace.h:61
> +ALWAYS_INLINE MarkedAllocator*
CompleteSubspace::allocatorForNonVirtual(size_t size, AllocationFailureMode
failureMode)

ditto about weird use of AllocationFailureMode

> Source/JavaScriptCore/heap/HeapCellType.cpp:47
> +	   MethodTable::DestroyFunctionPtr destroy =
classInfo->methodTable.destroy;

style nit: I'd just write `auto` for the type here

> Source/JavaScriptCore/heap/Subspace.h:58
> +    virtual MarkedAllocator* allocatorFor(size_t, AllocationFailureMode) =
0;

This feels like a really weird use of AllocationFailureMode. We never really
assert if it's null in the subclasses of Subspace. This is allowed to return
nullptr even if AllocationFailurueMode == Assert.

> Source/JavaScriptCore/jit/AssemblyHelpers.h:1575
> +	   MarkedAllocator* allocator =
subspaceFor<ClassType>(vm)->allocatorForNonVirtual(size,
AllocationFailureMode::Assert);

See comments about AllocationFailureMode used for this function and the virtual
variant. I think we need some other type because this can return nullptr even
though it's called with Assert

> Source/JavaScriptCore/runtime/ExecutableBase.h:87
> +    static void subspaceFor(VM&) { }

Maybe also RELEASE_ASSERT_NOT_REACHED? (I get what you're doing w/ return type,
but might as well catch anyone calling this w/out caring for return type)

Alternatively, does =delete work on arbitrary member functions? If so, you
could just do that too I believe and the compiler will catch it in all
scenarios.