[webkit-reviews] review granted: [Bug 172880] bmalloc: Small and large objects should share memory : [Attachment 311885] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Jun 3 13:09:28 PDT 2017


Sam Weinig <sam at webkit.org> has granted Geoffrey Garen <ggaren at apple.com>'s
request for review:
Bug 172880: bmalloc: Small and large objects should share memory
https://bugs.webkit.org/show_bug.cgi?id=172880

Attachment 311885: Patch

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




--- Comment #3 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 311885
  --> https://bugs.webkit.org/attachment.cgi?id=311885
Patch

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

> Source/bmalloc/ChangeLog:10
> +	   This reduces our high-watermark memory usage on JetStream on macOS
> +	   by 10%-20%.

Very nice!

Probably worth stating there performance change or that it had no effect.

Also, I would find it useful (and future readers of the code might as well) to
have an explanation up top of the changes in the algorithm,

> Source/bmalloc/ChangeLog:20
> +	   (bmalloc::forEachPage):A new helper function for iterating the pages

Missing space after the : which is personally killing me.

> Source/bmalloc/ChangeLog:47
> +	   (bmalloc::Heap::deallocateSmallLine):  Updated for new APIs. Note
that
> +	   we save one chunk per page class a little cache. This avoids churn

I don't follow the second sentence.  I feel like maybe a word or some
punctuation is missing.

> Source/bmalloc/bmalloc/Chunk.h:85
> +    for (Object it = begin; it + pageSize <= end; it = it + pageSize)

I'm not sure what your auto usage is like in bmalloc, but, you could replace
Object it with auto it.

> Source/bmalloc/bmalloc/Heap.cpp:172
> +	   for (auto chunk = m_freePages[pageClass].begin(); chunk !=
m_freePages[pageClass].end(); ++chunk) {
> +	       for (auto page = chunk->freePages().begin(); page !=
chunk->freePages().end(); ++page) {

I think these two loops could be simplified to:

for (auto& chunk : m_freePages[pageClass]) {
    for (auto& page = chunk.freePages()) {
	...
    }
}

> Source/bmalloc/bmalloc/Heap.cpp:183
> +    for (size_t pageClass = 0; pageClass < pageClassCount; pageClass++) {

We usually use ++pageClass.

> Source/bmalloc/bmalloc/LargeRange.h:54
> +    size_t physicalSize() const { return m_physicalSize; } // True physical
size can be larger.

This comment is confusing. Perhaps you could clarify what "True physical size"
is and why it can be larger.


More information about the webkit-reviews mailing list