[webkit-reviews] review denied: [Bug 193993] [JSC] Shrink size of VM by lazily allocating IsoSubspaces for non-common types : [Attachment 361042] Patch

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has denied Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 193993: [JSC] Shrink size of VM by lazily allocating IsoSubspaces for
non-common types
https://bugs.webkit.org/show_bug.cgi?id=193993

Attachment 361042: Patch

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




--- Comment #31 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 361042
  --> https://bugs.webkit.org/attachment.cgi?id=361042
Patch

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

r- since I think we can do a fair amount of code de-duplication.

Can we forward the SubSpaceAccess to the VM's getter? Currently, both the
subspaceFor and the VM's getter methods have to know about concurrent access.
If you make the getter on VM be:

Subspace* webAssemblyFunctionSpace(SubspaceAccess mode)
{
    if (m_ webAssemblyFunctionSpace || mode == SubspaceAccess::Concurrent)
	return webAssemblyFunctionSpace.get();
    return ensureWebAssemblyFunctionSpace();
}

Then all the various subspaceFor can be:

template<typename CellType, SubspaceAccess mode>
static Subspace* subspaceFor(VM& vm)
{
    return vm. webAssemblyFunctionSpace(mode);
}

Which makes them a lot simpler and doesn't make the VM code any more
complicated since you'll only have one method instead of two.

> Source/JavaScriptCore/runtime/JSCell.h:308
> +inline auto subspaceForConcurrently(VM& vm)
> +{
> +    return Type::template subspaceFor<Type,
SubspaceAccess::Concurrently>(vm);

Why not have:

template<typename Type>
inline Allocator allocatorForNonVirtualConcurrently(VM& vm, size_t
allocationSize, AllocatorForMode mode)
{
    if (auto subspace = Type::template subspaceFor<Type,
SubspaceAccess::Concurrently>(vm))
	return subspace->allocatorForNonVirtual(allocationSize, mode);
   return { };
}

Then you wouldn't have to duplicate nearly as much code in the JITs.

> Source/JavaScriptCore/runtime/VM.cpp:1228
> +IsoSubspace& VM::ensureBoundFunctionSpaceSlow()
> +{
> +    ASSERT(!boundFunctionSpace);
> +    auto space = std::make_unique<IsoSubspace> ISO_SUBSPACE_INIT(heap,
cellHeapCellType.get(), JSBoundFunction);
> +    WTF::storeStoreFence();
> +    boundFunctionSpace = WTFMove(space);
> +    return *boundFunctionSpace;
> +}

Nit: Can we make a template function or worse-case a macro that does this? This
is a lot of duplicated code. :(