Two consistency BASSERTs where added to LargeObject::merge in r196421. Those asserts are firing in four tests. fast/workers/dedicated-worker-lifecycle.html [ Crash ] inspector/model/remote-object-weak-collection.html [ Crash ] js/basic-map.html [ Crash ] js/duplicate-param-gc-crash.html [ Crash ] The crash is due to scavenging, either from the concurrent scavenger or when scavenging when called via WTF::releaseFastMallocFreeMemory(). The stack traces look like: 0 com.apple.JavaScriptCore 0x000000011060010d bmalloc::LargeObject::merge() const + 829 1 com.apple.JavaScriptCore 0x00000001105ff1b6 bmalloc::VMHeap::deallocateLargeObject(std::__1::unique_lock<bmalloc::StaticMutex>&, bmalloc::LargeObject&) + 54 2 com.apple.JavaScriptCore 0x00000001105fd112 bmalloc::Heap::scavengeLargeObjects(std::__1::unique_lock<bmalloc::StaticMutex>&, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000l> >) + 98 3 com.apple.JavaScriptCore 0x00000001105fcf04 bmalloc::Heap::scavenge(std::__1::unique_lock<bmalloc::StaticMutex>&, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000l> >) + 148
Created attachment 271188 [details] Patch
Created attachment 271189 [details] Really the patch
rdar://problem/24630709
Comment on attachment 271189 [details] Really the patch View in context: https://bugs.webkit.org/attachment.cgi?id=271189&action=review I think we need a more explicit policy about moving memory from VMHeap back to heap. In that case, change of Owner can reveal merging opportunities, which might turn into fragmentation or ASSERTs later. > Source/bmalloc/bmalloc/LargeObject.h:152 > +inline bool LargeObject::prevIsFreeAndOwnedBy(Owner owner) const > +{ > + EndTag* prev = m_beginTag->prev(); > + > + return prev->isFree() && prev->owner() == owner; > +} > + > +inline bool LargeObject::nextIsFreeAndOwnedBy(Owner owner) const > +{ > + BeginTag* next = m_endTag->next(); > + > + return next->isFree() && next->owner() == owner; > +} Let's call these two functions and the two above them "prevCanMerge" and "nextCanMerge". That's the question we're trying to ask. > Source/bmalloc/bmalloc/LargeObject.h:239 > + LargeObject result(beginTag, endTag, range.begin()); > + BASSERT(result.prevIsAllocated()); > + BASSERT(result.nextIsAllocated()); I think we shouldn't assert here. We don't thing we ever do this, but it wouldn't be so wrong to do a bunch of splits and then a bunch of merges -- as long as you never put the fragments back into the free list before merging them back together. > Source/bmalloc/bmalloc/VMHeap.h:110 > + // we don't hit an assert that we are inserting a free object next to another. Be sure to set the owner for the object we return before inserting the leftover back into the free list. The free list asserts that we never insert an object that could have merged with its neighbor. > Source/bmalloc/bmalloc/VMHeap.h:162 > inline void VMHeap::deallocateLargeObject(std::unique_lock<StaticMutex>& lock, LargeObject& largeObject) Let's take this argument by value, and then not make a copy with a different name. > Source/bmalloc/bmalloc/VMHeap.h:182 > + } while (objectToDeallocate.prevIsFreeAndOwnedBy(Owner::VMHeap) || objectToDeallocate.nextIsFreeAndOwnedBy(Owner::VMHeap)); No need to add a version of the function that takes an explicit owner. Our owner is VMHeap. Might be nice to add a helper that calls both for you: canMerge(). I guess we should add a comment explaining that multiple threads might scavenge concurrently, meaning that new merging opportunities become visible after we reacquire the lock.
Let's establish an invariant that the Heap never splits an object returned by the VMHeap. Our current splitting behavior for aligned allocation needlessly commits some dirty memory. Also, extra splitting might create surprising merge opportunities when moving objects between heaps, causing our fragmentation ASSERTs to fire. I think we can take the "split and allocate" code in Heap and move it into helper functions. Then, both Heap and VMHeap can have identical "split and allocate" logic, and Heap can just immediately return whatever VMHeap returns, without worrying about further splits. It looks like the inputs to the function are always a large object and a segregated free list.
(In reply to comment #5) > Let's establish an invariant that the Heap never splits an object returned > by the VMHeap. Our current splitting behavior for aligned allocation > needlessly commits some dirty memory. Also, extra splitting might create > surprising merge opportunities when moving objects between heaps, causing > our fragmentation ASSERTs to fire. > > I think we can take the "split and allocate" code in Heap and move it into > helper functions. Then, both Heap and VMHeap can have identical "split and > allocate" logic, and Heap can just immediately return whatever VMHeap > returns, without worrying about further splits. It looks like the inputs to > the function are always a large object and a segregated free list. Will handle this with a separate bug: <https://bugs.webkit.org/show_bug.cgi?id=154192> - bmalloc: Use common "split and allocate" code for large allocations in VMHeap and Heap
Created attachment 271213 [details] Patch for Landing
Attachment 271213 [details] did not pass style-queue: ERROR: Source/bmalloc/bmalloc/VMHeap.h:170: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 271217 [details] patch for Landing - Fixed comment style complaint
Comment on attachment 271217 [details] patch for Landing - Fixed comment style complaint Forgot to make change to VMHeap::deallocateLargeObject()
Created attachment 271242 [details] Last Patch for Landing
Comment on attachment 271242 [details] Last Patch for Landing Clearing flags on attachment: 271242 Committed r196536: <http://trac.webkit.org/changeset/196536>
All reviewed patches have been landed. Closing bug.