Summary: | Vector should consult allocator about ideal size when choosing capacity. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||||||||
Component: | Web Template Framework | Assignee: | Andreas Kling <kling> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | andersca, barraclough, benjamin, darin, eric, ggaren, jberlin, koivisto, ojan.autocc, sam, thakis, webkit-bug-importer, webkit.review.bot | ||||||||||
Priority: | P2 | Keywords: | InRadar, Performance | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Andreas Kling
2013-01-30 17:51:05 PST
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 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. |