Vector should consult allocator about ideal size when choosing capacity.
Created attachment 185640 [details] Patch
Comment on attachment 185640 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185640&action=review > Source/WTF/wtf/Vector.h:255 > + newCapacity = fastMallocGoodSize(newCapacity * sizeof(T)) / sizeof(T); Why not also tryAllocateBuffer() and reallocateBuffer()?
(In reply to comment #2) > (From update of attachment 185640 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185640&action=review > > > Source/WTF/wtf/Vector.h:255 > > + newCapacity = fastMallocGoodSize(newCapacity * sizeof(T)) / sizeof(T); > > Why not also tryAllocateBuffer() and reallocateBuffer()? Because blind spot. :(
Created attachment 185645 [details] Patch Kinda curious how EWS likes this.
Comment on attachment 185645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185645&action=review I love this patch. Overflow? > Source/JavaScriptCore/ChangeLog:8 > + Remove assertion about Vector capacity that won't hold anymore since. "that won't hold anymore since."? > Source/WTF/wtf/Deque.h:387 > - size_t newCapacity = std::max(static_cast<size_t>(16), oldCapacity + oldCapacity / 4 + 1); > T* oldBuffer = m_buffer.buffer(); > - m_buffer.allocateBuffer(newCapacity); > + m_buffer.allocateBuffer(std::max(static_cast<size_t>(16), oldCapacity + oldCapacity / 4 + 1)); I liked the variable newCapacity :)
Created attachment 185652 [details] Patch So much thrashing. I blame the coffee. @Benjamin: I removed the 'newCapacity' local in Deque since I have to read back m_buffer.capacity() for the correct value.
Comment on attachment 185652 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185652&action=review > Source/WTF/wtf/Vector.h:259 > + newCapacity = fastMallocGoodSize(newCapacity * sizeof(T)) / sizeof(T); > + m_capacity = newCapacity; > m_buffer = static_cast<T*>(fastMalloc(newCapacity * sizeof(T))); Okay, now, what about we can remove the /sizeof(T)? :-D size_t sizeToAllocate = fastMallocGoodSize(newCapacity * sizeof(T)); m_capacity = sizeToAllocate / sizeof(T) m_buffer = mallo(sizeToAllocate). > Source/WTF/wtf/Vector.h:271 > + newCapacity = fastMallocGoodSize(newCapacity * sizeof(T)) / sizeof(T); > T* newBuffer; > if (tryFastMalloc(newCapacity * sizeof(T)).getValue(newBuffer)) { > m_capacity = newCapacity; ditto > Source/WTF/wtf/Vector.h:290 > ASSERT(shouldReallocateBuffer(newCapacity)); > - m_capacity = newCapacity; > if (newCapacity > std::numeric_limits<size_t>::max() / sizeof(T)) > CRASH(); > + newCapacity = fastMallocGoodSize(newCapacity * sizeof(T)) / sizeof(T); > + m_capacity = newCapacity; > m_buffer = static_cast<T*>(fastRealloc(m_buffer, newCapacity * sizeof(T))); ditto.
(In reply to comment #7) > (From update of attachment 185652 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185652&action=review > > > Source/WTF/wtf/Vector.h:259 > > + newCapacity = fastMallocGoodSize(newCapacity * sizeof(T)) / sizeof(T); > > + m_capacity = newCapacity; > > m_buffer = static_cast<T*>(fastMalloc(newCapacity * sizeof(T))); > > Okay, now, what about we can remove the /sizeof(T)? :-D > > size_t sizeToAllocate = fastMallocGoodSize(newCapacity * sizeof(T)); > m_capacity = sizeToAllocate / sizeof(T) > m_buffer = mallo(sizeToAllocate). > > > Source/WTF/wtf/Vector.h:271 > > + newCapacity = fastMallocGoodSize(newCapacity * sizeof(T)) / sizeof(T); > > T* newBuffer; > > if (tryFastMalloc(newCapacity * sizeof(T)).getValue(newBuffer)) { > > m_capacity = newCapacity; > > ditto > > > Source/WTF/wtf/Vector.h:290 > > ASSERT(shouldReallocateBuffer(newCapacity)); > > - m_capacity = newCapacity; > > if (newCapacity > std::numeric_limits<size_t>::max() / sizeof(T)) > > CRASH(); > > + newCapacity = fastMallocGoodSize(newCapacity * sizeof(T)) / sizeof(T); > > + m_capacity = newCapacity; > > m_buffer = static_cast<T*>(fastRealloc(m_buffer, newCapacity * sizeof(T))); > > ditto. Sure, when I'm awake again. No need to reupload for that :)
Comment on attachment 185652 [details] Patch Attachment 185652 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16271120
<rdar://problem/13124002>
Committed r141407: <http://trac.webkit.org/changeset/141407>
(In reply to comment #9) > (From update of attachment 185652 [details]) > Attachment 185652 [details] did not pass win-ews (win): > Output: http://queues.webkit.org/results/16271120 Landed with Benjamin's comments addressed. This ews failure looks like a rebuild of QTMovieWin is needed.
I had to roll this patch out in http://trac.webkit.org/changeset/141504 because it was causing crashes in release builds.
Created attachment 186239 [details] Patch Finally tracked down the crash, it was due to fastMallocGoodSize() getting called before the TCMalloc page heap was fully initialized. Fixed this by calling TCMalloc_ThreadCache::InitModule() in fastMallocGoodSize() if needed.
Comment on attachment 186239 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186239&action=review > Source/WTF/wtf/FastMalloc.cpp:2574 > + if (!phinited) > + TCMalloc_ThreadCache::InitModule(); > + return AllocationSize(bytes); > +} Any reason we couldn't generate class_to_size and class_to_pages at compile time?
Comment on attachment 186239 [details] Patch Attachment 186239 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16358080
Committed r141716: <http://trac.webkit.org/changeset/141716>
(In reply to comment #15) > (From update of attachment 186239 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186239&action=review > > > Source/WTF/wtf/FastMalloc.cpp:2574 > > + if (!phinited) > > + TCMalloc_ThreadCache::InitModule(); > > + return AllocationSize(bytes); > > +} > > Any reason we couldn't generate class_to_size and class_to_pages at compile time? We might be able to do that. Let's take that in a separate patch though.
I made an attempt at this for String back in 2008 when strings were resizable. I guess there’s a chance that StringBuilder might benefit from this, even today.