[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