[webkit-reviews] review granted: [Bug 201908] [JSC] Make IsoSubspace scalable : [Attachment 383088] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Nov 7 18:13:28 PST 2019


Keith Miller <keith_miller at apple.com> has granted Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 201908: [JSC] Make IsoSubspace scalable
https://bugs.webkit.org/show_bug.cgi?id=201908

Attachment 383088: Patch

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




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

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

r=me with comments.

> Source/JavaScriptCore/ChangeLog:13
> +	   We use LargeAllocation for this lower-tier objects. Each IsoSubspace
holds up to 8 lower-tier objects

nit: this => these

> Source/JavaScriptCore/ChangeLog:15
> +	   allocated via LargeAllocation. And use it when MarkedBlock of
IsoSubspace exhausts (like, zero MarekdBlock
> +	   here). And once this LargeAllocation is allocated to a certain type,
we do not deallocate it until VM

Can you clarify this slightly. I think you are saying something like:

 The allocation order for any given cell type is:
try to allocate in an existing MarkedBlock (there won't be one to start).
try to allocate in an existing LargeAllocation.
allocate a new MarkedBlock and/or GC.

> Source/JavaScriptCore/ChangeLog:26
> +	   that is pointing at the middle of the JSCell in sampling profiler,
just registering cell address is enough. And we
> +	   maintain this hash-set only when sampling profiler is enabled to
save memory in major cases.

Did you benchmark how much this impacts the sampling profiler's regression on
the rest of the system?

>> Source/JavaScriptCore/ChangeLog:31
>> +	    We also make sizeof(LargeAllocation) small since it is now used for
non-large allocations.
> 
> The name `LargeAllocation` is a bit weird, but anyway, in this patch, I don't
change it.

Can you do a follow up patch to rename `LargeAllocation` to `CustomAllocation`
or something?

> Source/JavaScriptCore/heap/HeapUtil.h:165
> +    // It does not find the cell if the pointer is pointing at the middle of
a JSCell.

Nit: It does => This does.

> Source/JavaScriptCore/heap/IsoSubspace.cpp:123
> +    m_lowerTierFreeList.append(largeAllocation);
> +    m_space.m_sweptLowerTierCells.append(largeAllocation);

Can a largeAllocation ever be on the m_sweptLowerTierCells list but not in the
m_lowerTierFreeList? If not, why have the m_lowerTierFreeList at all? Why not
just take objects from m_sweptLowerTierCells? As far as I can tell they seem
like they contain the same information.

> Source/JavaScriptCore/heap/LargeAllocation.cpp:163
> +LargeAllocation* LargeAllocation::reuseForLowerTier()
> +{
> +    Heap& heap = *this->heap();
> +    size_t size = m_cellSize;
> +    Subspace* subspace = m_subspace;
> +    bool adjustedAlignment = m_adjustedAlignment;
> +    uint8_t lowerTierIndex = m_lowerTierIndex;
> +
> +    void* space = this->basePointer();
> +    this->~LargeAllocation();
> +
> +    LargeAllocation* largeAllocation = new (NotNull, space)
LargeAllocation(heap, size, subspace, 0, adjustedAlignment);
> +    largeAllocation->m_lowerTierIndex = lowerTierIndex;
> +    largeAllocation->m_hasValidCell = false;
> +    return largeAllocation;
> +}

Does this do anything? You only call this from sweepLowerTierCell() but that's
only called if the cell is already lower tier?


More information about the webkit-reviews mailing list