[webkit-reviews] review granted: [Bug 178108] bmalloc should support strictly type-segregated isolated heaps : [Attachment 326049] 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 178108: bmalloc should support strictly type-segregated isolated heaps
https://bugs.webkit.org/show_bug.cgi?id=178108

Attachment 326049: the patch

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




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

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

r=me
I have a few comments and a couple questions.

> Source/bmalloc/ChangeLog:18
> +	   pages. Pages are corrected into directories. Directories track pages
using bitvectors, so

corrected => collected

> Source/bmalloc/bmalloc/Bits.h:1
> +/*

It's a bit unfortunate that this is probably almost identical to WTF's
implementation.

> Source/bmalloc/bmalloc/Bits.h:496
> +	   return BitReference(&this->m_words.word(index >> 5), 1 << (index &
31));

WHy 5?

> Source/bmalloc/bmalloc/DeferredDecommit.h:45
> +
> +
> +    

unneeded whitespace

> Source/bmalloc/bmalloc/IsoDirectory.h:37
> +class IsoDirectoryBaseBase {

BaseBase :(

> Source/bmalloc/bmalloc/IsoDirectory.h:42
> +    virtual void didDecommit(unsigned index) = 0;

Do we really need this to be virtual? There is only one implementation.

> Source/bmalloc/bmalloc/IsoDirectory.h:69
> +    void didBecome(IsoPage<Config>*, IsoPageTrigger) override;

This only has one implementation. Do we need it to be virtual?

> Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49
> +    unsigned pageIndex = (m_eligible |
~m_committed).findBit(m_firstEligible, true);;

style: extra semicolon at the end

> Source/bmalloc/bmalloc/IsoHeapImpl.h:67
> +    void didBecomeEligible(IsoDirectory<Config,
numPagesInInlineDirectory>*); // This one probably needs to set a bit or
something.
> +    void didBecomeEligible(IsoDirectory<Config,
IsoDirectoryPage<Config>::numPages>*); // This one needs to do things.

these comments are not helpful

> Source/bmalloc/bmalloc/IsoHeapImpl.h:96
> +    // The basic operations that we need to support are:

Part of this text seems to have been written before you implemented things.
Maybe you can clean some of it up now that you've made implementation
decisions?

> Source/bmalloc/bmalloc/IsoHeapImpl.h:121
> +    // - Deallocation should probably use a deallocation log, just like the
rest of bmalloc.
> +    //   -> This avoids having to use any kinds of atomics on the freeing
fast path. The slow path can
> +    //      just grab a lock around the whole isoheap or something.

This makes things seem up in the air, but are they?

> Source/bmalloc/bmalloc/IsoHeapImpl.h:122
> +    // - When we create an isoheap, we want to make sure that TLCs find out.

TLC?

> Source/bmalloc/bmalloc/IsoHeapImplInlines.h:46
> +	   if (result.kind == EligibilityKind::Full)

Do we want to assert it's not OOM kind?

> Source/bmalloc/bmalloc/IsoHeapImplInlines.h:62
> +	   if (result.kind != EligibilityKind::Full)

ditto w.r.t OOM?

> Source/bmalloc/bmalloc/IsoPage.h:40
> +    static constexpr size_t pageSize = 16384;

Do we want this to be 4096 on iOS? Doesn't bmalloc already have a notion of
this somewhere?

> Source/bmalloc/bmalloc/IsoPage.h:46
> +    static constexpr unsigned numObjects = pageSize / Config::objectSize;

Do we want to add static_assert(numObjects > 0)?

> Source/bmalloc/bmalloc/IsoPage.h:55
> +    // This must have a trivial destructor.

Nit: Maybe it's worth putting this comment by the field declarations?

> Source/bmalloc/bmalloc/IsoPageInlines.h:104
> +	   unsigned beginWord = begin >> 5;
> +	   unsigned endWord = end >> 5;

Maybe we can give this 5 a name? And also static cast that (sizeof(unsigned) *
8 == 1<<5)?

> Source/bmalloc/bmalloc/IsoPageInlines.h:124
> +	       unsigned endBeginSlop = (begin + 31) & ~31;
> +	       unsigned beginEndSlop = end & ~31;

seems like this may be related to the 5? as in 2^5 - 1?
Also, bmalloc already has roundUpToMultipleOf, can we use that here w/ 32
instead of it handwritten?

This math confuses me :(

> Source/bmalloc/bmalloc/IsoPageInlines.h:180
> +	   unsigned wordIndex = index >> 5;

why not / 32 here? llvm should do the right thing and it's easier to read

> Source/bmalloc/bmalloc/IsoPageInlines.h:184
> +	   unsigned bitMask = 1 << (index & 31);
> +	   if (word & bitMask)
> +	       continue;

This confuses me. What is the purpose of this?

> Source/bmalloc/bmalloc/IsoPageInlines.h:186
> +	   if (!word)
> +	       m_numNonEmptyWords++;

Shouldn't this condition be flipped?

> Source/bmalloc/bmalloc/IsoPageInlines.h:231
> +	   for (unsigned bitIndex = 0; bitIndex < 32; ++bitIndex) {

nit: maybe it's worth writing this as while(word) { ... }? Or is it worth
having this be unrolled perhaps?

> Source/bmalloc/bmalloc/IsoTLSInlines.h:109
> +    if (!handle.isInitialized()) {
> +	   std::lock_guard<StaticMutex> locker(handle.m_initializationLock);

Can't two threads race to enter here at the same time? Shouldn't we check
isInitialized again?