[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