[webkit-reviews] review requested: [Bug 213071] Replace JSC::FreeList linked list with a Bitmap. : [Attachment 402164] proposed patch.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jun 17 19:15:52 PDT 2020
Robin Morisset <rmorisset at apple.com> has asked for review:
Bug 213071: Replace JSC::FreeList linked list with a Bitmap.
https://bugs.webkit.org/show_bug.cgi?id=213071
Attachment 402164: proposed patch.
https://bugs.webkit.org/attachment.cgi?id=402164&action=review
--- Comment #11 from Robin Morisset <rmorisset at apple.com> ---
Comment on attachment 402164
--> https://bugs.webkit.org/attachment.cgi?id=402164
proposed patch.
View in context: https://bugs.webkit.org/attachment.cgi?id=402164&action=review
This patch looks perfectly reasonable to me, but I am not familiar enough with
this code to feel comfortable r+-ing it alone.
> Source/JavaScriptCore/ChangeLog:98
> + there is n the location of m_cellSize. It is now moved up next
to m_remaining,
typo: "n" ?
> Source/JavaScriptCore/heap/FreeList.h:108
> + // if there atoms still available for allocation. See comment blob
below
typo: "there atoms" => "there are atoms"
> Source/JavaScriptCore/heap/FreeList.h:119
> + static ptrdiff_t offsetOfBitmapRows() { return OBJECT_OFFSETOF(FreeList,
m_bitmap) - sizeof(AtomsBitmap::Word); }
Maybe rename to offsetOfBitmapRowsMinusOne, or offsetOneBeforeBitmapRows, or
something like this? Just in case anyone tries to use it without looking at
where it is defined.
> Source/JavaScriptCore/heap/FreeListInlines.h:100
> + while (rowBitmap) {
It probably does not matter, but I can think of a way to make this loop a tad
more efficient:
```
unsigned atomIndexInRow = 0;
while (rowBitmap) {
atomIndexInRow += ctz(rowBitmap);
auto* cell =
bitwise_cast<HeapCell*>(¤tMarkedBlockRowAddress[atomIndexInRow]);
rowBitmap >>= (++atomIndexInRow);
func(cell);
}
```
> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:561
> +#if CPU(ARM64)
I am a bit wary that this code will never be tested since BITMAP_FREELIST is
currently only set on x86_64. I don't have a better solution though.
It is also a bit weird to have both "#if CPU(ARM64)" and "if (isARM64())", but
just "if (isx86_64())". What is the criterion for picking one of these ways of
checking the CPU?
More information about the webkit-reviews
mailing list