Skip to content

Commit

Permalink
Merge r180797 - bmalloc: Pathological madvise churn on the free(mallo…
Browse files Browse the repository at this point in the history 
…c(x)) benchmark

https://bugs.webkit.org/show_bug.cgi?id=142058

Reviewed by Andreas Kling.

The churn was caused by repeatedly splitting an object with physical
pages from an object without, and then merging them back together again.
The merge would conservatively forget that we had physical pages, forcing
a new call to madvise on the next allocation.

This patch more strictly segregates objects in the heap from objects in
the VM heap, with these changes:

(1) Objects in the heap are not allowed to merge with objects in the VM
heap, and vice versa -- since that would erase our precise knowledge of
which physical pages had been allocated.

(2) The VM heap is exclusively responsible for allocating and deallocating
physical pages.

(3) The heap free list must consider entries for objects that are in the
VM heap to be invalid, and vice versa. (This condition can arise
because the free list does not eagerly remove items.)

With these changes, we can know that any valid object in the heap's free
list already has physical pages, and does not need to call madvise.

Note that the VM heap -- as before -- might sometimes contain ranges
or pieces of ranges that have physical pages, since we allow splitting
of ranges at granularities smaller than the VM page size. These ranges
can eventually merge with ranges in the heap during scavenging.

* bmalloc.xcodeproj/project.pbxproj:

* bmalloc/BoundaryTag.h:
(bmalloc::BoundaryTag::owner):
(bmalloc::BoundaryTag::setOwner):
(bmalloc::BoundaryTag::initSentinel):
(bmalloc::BoundaryTag::hasPhysicalPages): Deleted.
(bmalloc::BoundaryTag::setHasPhysicalPages): Deleted. Replaced the concept
of "has physical pages" with a bit indicating which heap owns the large
object. This is a more precise concept, since the old bit was really a
Yes / Maybe bit.

* bmalloc/Deallocator.cpp:

* bmalloc/FreeList.cpp: Adopt
(bmalloc::FreeList::takeGreedy):
(bmalloc::FreeList::take):
(bmalloc::FreeList::removeInvalidAndDuplicateEntries):
* bmalloc/FreeList.h:
(bmalloc::FreeList::push): Added API for considering the owner when
deciding if a free list entry is valid.

* bmalloc/Heap.cpp:
(bmalloc::Heap::Heap): Adopt new API.

(bmalloc::Heap::scavengeLargeRanges): Scavenge all ranges with no minimum,
since some ranges might be able to merge with ranges in the VM heap, and
they won't be allowed to until we scavenge them.

(bmalloc::Heap::allocateSmallPage):
(bmalloc::Heap::allocateMediumPage):
(bmalloc::Heap::allocateLarge): New VM heap API makes this function
simpler, since we always get back physical pages now.

* bmalloc/Heap.h:
* bmalloc/LargeObject.h:
(bmalloc::LargeObject::end):
(bmalloc::LargeObject::owner):
(bmalloc::LargeObject::setOwner):
(bmalloc::LargeObject::isValidAndFree):
(bmalloc::LargeObject::merge): Do not merge objects across heaps since
that causes madvise churn.
(bmalloc::LargeObject::validateSelf):
(bmalloc::LargeObject::init):
(bmalloc::LargeObject::hasPhysicalPages): Deleted.
(bmalloc::LargeObject::setHasPhysicalPages): Deleted. Propogate the Owner API.

* bmalloc/Owner.h: Added.

* bmalloc/SegregatedFreeList.cpp:
(bmalloc::SegregatedFreeList::SegregatedFreeList):
(bmalloc::SegregatedFreeList::insert):
(bmalloc::SegregatedFreeList::takeGreedy):
(bmalloc::SegregatedFreeList::take):
* bmalloc/SegregatedFreeList.h: Propogate the owner API.

* bmalloc/VMAllocate.h:
(bmalloc::vmDeallocatePhysicalPagesSloppy):
(bmalloc::vmAllocatePhysicalPagesSloppy): Clarified these functions and
removed an edge case.

* bmalloc/VMHeap.cpp:
(bmalloc::VMHeap::VMHeap):
* bmalloc/VMHeap.h:
(bmalloc::VMHeap::allocateSmallPage):
(bmalloc::VMHeap::allocateMediumPage):
(bmalloc::VMHeap::allocateLargeObject):
(bmalloc::VMHeap::deallocateLargeObject): Be sure to give each object
a new chance to merge, since it might have been prohibited from merging
before by virtue of not being in the VM heap.

(bmalloc::VMHeap::allocateLargeRange): Deleted.
(bmalloc::VMHeap::deallocateLargeRange): Deleted.
  • Loading branch information
@geoffreygaren @carlosgcampos
geoffreygaren authored and carlosgcampos committed Mar 1, 2015
1 parent 8fdb767 commit 76aab4b
Show file tree
Hide file tree
Showing 15 changed files with 289 additions and 110 deletions.
123 changes: 123 additions & 0 deletions Source/bmalloc/ChangeLog
View file
@@ -1,3 +1,126 @@
2015-02-27 Ryosuke Niwa <rniwa@webkit.org>

Fixed a typo in the previous commit.

* bmalloc/BoundaryTag.h:
(bmalloc::BoundaryTag::setOwner):

2015-02-27 Ryosuke Niwa <rniwa@webkit.org>

EFL build fix after r180797.

* bmalloc/BoundaryTag.h:
(bmalloc::BoundaryTag::owner):
(bmalloc::BoundaryTag::setOwner):

2015-02-27 Geoffrey Garen <ggaren@apple.com>

bmalloc: Pathological madvise churn on the free(malloc(x)) benchmark
https://bugs.webkit.org/show_bug.cgi?id=142058

Reviewed by Andreas Kling.

The churn was caused by repeatedly splitting an object with physical
pages from an object without, and then merging them back together again.
The merge would conservatively forget that we had physical pages, forcing
a new call to madvise on the next allocation.

This patch more strictly segregates objects in the heap from objects in
the VM heap, with these changes:

(1) Objects in the heap are not allowed to merge with objects in the VM
heap, and vice versa -- since that would erase our precise knowledge of
which physical pages had been allocated.

(2) The VM heap is exclusively responsible for allocating and deallocating
physical pages.

(3) The heap free list must consider entries for objects that are in the
VM heap to be invalid, and vice versa. (This condition can arise
because the free list does not eagerly remove items.)

With these changes, we can know that any valid object in the heap's free
list already has physical pages, and does not need to call madvise.

Note that the VM heap -- as before -- might sometimes contain ranges
or pieces of ranges that have physical pages, since we allow splitting
of ranges at granularities smaller than the VM page size. These ranges
can eventually merge with ranges in the heap during scavenging.

* bmalloc.xcodeproj/project.pbxproj:

* bmalloc/BoundaryTag.h:
(bmalloc::BoundaryTag::owner):
(bmalloc::BoundaryTag::setOwner):
(bmalloc::BoundaryTag::initSentinel):
(bmalloc::BoundaryTag::hasPhysicalPages): Deleted.
(bmalloc::BoundaryTag::setHasPhysicalPages): Deleted. Replaced the concept
of "has physical pages" with a bit indicating which heap owns the large
object. This is a more precise concept, since the old bit was really a
Yes / Maybe bit.

* bmalloc/Deallocator.cpp:

* bmalloc/FreeList.cpp: Adopt
(bmalloc::FreeList::takeGreedy):
(bmalloc::FreeList::take):
(bmalloc::FreeList::removeInvalidAndDuplicateEntries):
* bmalloc/FreeList.h:
(bmalloc::FreeList::push): Added API for considering the owner when
deciding if a free list entry is valid.

* bmalloc/Heap.cpp:
(bmalloc::Heap::Heap): Adopt new API.

(bmalloc::Heap::scavengeLargeRanges): Scavenge all ranges with no minimum,
since some ranges might be able to merge with ranges in the VM heap, and
they won't be allowed to until we scavenge them.

(bmalloc::Heap::allocateSmallPage):
(bmalloc::Heap::allocateMediumPage):
(bmalloc::Heap::allocateLarge): New VM heap API makes this function
simpler, since we always get back physical pages now.

* bmalloc/Heap.h:
* bmalloc/LargeObject.h:
(bmalloc::LargeObject::end):
(bmalloc::LargeObject::owner):
(bmalloc::LargeObject::setOwner):
(bmalloc::LargeObject::isValidAndFree):
(bmalloc::LargeObject::merge): Do not merge objects across heaps since
that causes madvise churn.
(bmalloc::LargeObject::validateSelf):
(bmalloc::LargeObject::init):
(bmalloc::LargeObject::hasPhysicalPages): Deleted.
(bmalloc::LargeObject::setHasPhysicalPages): Deleted. Propogate the Owner API.

* bmalloc/Owner.h: Added.

* bmalloc/SegregatedFreeList.cpp:
(bmalloc::SegregatedFreeList::SegregatedFreeList):
(bmalloc::SegregatedFreeList::insert):
(bmalloc::SegregatedFreeList::takeGreedy):
(bmalloc::SegregatedFreeList::take):
* bmalloc/SegregatedFreeList.h: Propogate the owner API.

* bmalloc/VMAllocate.h:
(bmalloc::vmDeallocatePhysicalPagesSloppy):
(bmalloc::vmAllocatePhysicalPagesSloppy): Clarified these functions and
removed an edge case.

* bmalloc/VMHeap.cpp:
(bmalloc::VMHeap::VMHeap):
* bmalloc/VMHeap.h:
(bmalloc::VMHeap::allocateSmallPage):
(bmalloc::VMHeap::allocateMediumPage):
(bmalloc::VMHeap::allocateLargeObject):
(bmalloc::VMHeap::deallocateLargeObject): Be sure to give each object
a new chance to merge, since it might have been prohibited from merging
before by virtue of not being in the VM heap.

(bmalloc::VMHeap::allocateLargeRange): Deleted.
(bmalloc::VMHeap::deallocateLargeRange): Deleted.

2015-02-26 Geoffrey Garen <ggaren@apple.com>

bmalloc: Large object free list can grow infinitely
Expand Down
4 changes: 4 additions & 0 deletions Source/bmalloc/bmalloc.xcodeproj/project.pbxproj
View file
Expand Up @@ -26,6 +26,7 @@
14C6216F1A9A9A6200E72293 /* LargeObject.h in Headers */ = {isa = PBXBuildFile; fileRef = 14C6216E1A9A9A6200E72293 /* LargeObject.h */; settings = {ATTRIBUTES = (Private, ); }; };
14C919C918FCC59F0028DB43 /* BPlatform.h in Headers */ = {isa = PBXBuildFile; fileRef = 14C919C818FCC59F0028DB43 /* BPlatform.h */; settings = {ATTRIBUTES = (Private, ); }; };
14CC394C18EA8858004AFE34 /* libbmalloc.a in Frameworks */ = {isa = PBXBuildFile; fileRef = 14F271BE18EA3963008C152F /* libbmalloc.a */; };
14D2CD9B1AA12CFB00770440 /* Owner.h in Headers */ = {isa = PBXBuildFile; fileRef = 14D2CD9A1AA12CFB00770440 /* Owner.h */; settings = {ATTRIBUTES = (Private, ); }; };
14DD788C18F48CAE00950702 /* LargeChunk.h in Headers */ = {isa = PBXBuildFile; fileRef = 147AAA8818CD17CE002201E4 /* LargeChunk.h */; settings = {ATTRIBUTES = (Private, ); }; };
14DD788D18F48CC600950702 /* BeginTag.h in Headers */ = {isa = PBXBuildFile; fileRef = 1417F64518B54A700076FA3F /* BeginTag.h */; settings = {ATTRIBUTES = (Private, ); }; };
14DD788E18F48CCD00950702 /* BoundaryTag.h in Headers */ = {isa = PBXBuildFile; fileRef = 1485655E18A43AF900ED6942 /* BoundaryTag.h */; settings = {ATTRIBUTES = (Private, ); }; };
Expand Down Expand Up @@ -138,6 +139,7 @@
14C6216E1A9A9A6200E72293 /* LargeObject.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = LargeObject.h; path = bmalloc/LargeObject.h; sourceTree = "<group>"; };
14C919C818FCC59F0028DB43 /* BPlatform.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = BPlatform.h; path = bmalloc/BPlatform.h; sourceTree = "<group>"; };
14CC394418EA8743004AFE34 /* libmbmalloc.dylib */ = {isa = PBXFileReference; explicitFileType = "compiled.mach-o.dylib"; includeInIndex = 0; path = libmbmalloc.dylib; sourceTree = BUILT_PRODUCTS_DIR; };
14D2CD9A1AA12CFB00770440 /* Owner.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = Owner.h; path = bmalloc/Owner.h; sourceTree = "<group>"; };
14D9DB4517F2447100EAAB79 /* FixedVector.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; lineEnding = 0; name = FixedVector.h; path = bmalloc/FixedVector.h; sourceTree = "<group>"; xcLanguageSpecificationIdentifier = xcode.lang.objcpp; };
14DA32071885F9E6007269E0 /* Line.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; lineEnding = 0; name = Line.h; path = bmalloc/Line.h; sourceTree = "<group>"; xcLanguageSpecificationIdentifier = xcode.lang.objcpp; };
14DA320C18875B09007269E0 /* Heap.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = Heap.h; path = bmalloc/Heap.h; sourceTree = "<group>"; };
Expand Down Expand Up @@ -224,6 +226,7 @@
143EF9AE1A9FABF6004F5C77 /* FreeList.h */,
147AAA8818CD17CE002201E4 /* LargeChunk.h */,
14C6216E1A9A9A6200E72293 /* LargeObject.h */,
14D2CD9A1AA12CFB00770440 /* Owner.h */,
146BEE2118C845AE0002D5A2 /* SegregatedFreeList.cpp */,
146BEE1E18C841C50002D5A2 /* SegregatedFreeList.h */,
);
Expand Down Expand Up @@ -353,6 +356,7 @@
14DD788E18F48CCD00950702 /* BoundaryTag.h in Headers */,
14DD78C818F48D7500950702 /* FixedVector.h in Headers */,
14DD78B718F48D6B00950702 /* MediumLine.h in Headers */,
14D2CD9B1AA12CFB00770440 /* Owner.h in Headers */,
14DD78B618F48D6B00950702 /* MediumChunk.h in Headers */,
14DD78BC18F48D6B00950702 /* SmallLine.h in Headers */,
14DD789818F48D4A00950702 /* Allocator.h in Headers */,
Expand Down
8 changes: 5 additions & 3 deletions Source/bmalloc/bmalloc/BoundaryTag.h
View file
Expand Up @@ -27,6 +27,7 @@
#define BoundaryTag_h

#include "BAssert.h"
#include "Owner.h"
#include "Range.h"
#include "Sizes.h"
#include <cstring>
Expand All @@ -49,8 +50,8 @@ class BoundaryTag {
bool isEnd() { return m_isEnd; }
void setEnd(bool isEnd) { m_isEnd = isEnd; }

bool hasPhysicalPages() { return m_hasPhysicalPages; }
void setHasPhysicalPages(bool hasPhysicalPages) { m_hasPhysicalPages = hasPhysicalPages; }
Owner owner() { return m_ownerIsHeap ? Owner::Heap : Owner::VMHeap; }
void setOwner(Owner owner) { m_ownerIsHeap = (owner == Owner::Heap); }

bool isMarked() { return m_isMarked; }
void setMarked(bool isMarked) { m_isMarked = isMarked; }
Expand Down Expand Up @@ -84,7 +85,7 @@ class BoundaryTag {

bool m_isFree: 1;
bool m_isEnd: 1;
bool m_hasPhysicalPages: 1;
bool m_ownerIsHeap: 1;
bool m_isMarked: 1;
unsigned m_compactBegin: compactBeginBits;
unsigned m_size: sizeBits;
Expand Down Expand Up @@ -121,6 +122,7 @@ inline void BoundaryTag::initSentinel()
{
setRange(Range(nullptr, largeMin));
setFree(false);
setOwner(Owner::VMHeap);
}

} // namespace bmalloc
Expand Down
1 change: 0 additions & 1 deletion Source/bmalloc/bmalloc/Deallocator.cpp
View file
Expand Up @@ -24,7 +24,6 @@
*/

#include "BAssert.h"
#include "BeginTag.h"
#include "LargeChunk.h"
#include "Deallocator.h"
#include "Heap.h"
Expand Down
20 changes: 8 additions & 12 deletions Source/bmalloc/bmalloc/FreeList.cpp
View file
Expand Up @@ -23,43 +23,39 @@
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/

#include "BeginTag.h"
#include "LargeChunk.h"
#include "FreeList.h"
#include "Vector.h"

namespace bmalloc {

LargeObject FreeList::takeGreedy(size_t size)
LargeObject FreeList::takeGreedy(Owner owner)
{
for (size_t i = m_vector.size(); i-- > 0; ) {
// We don't eagerly remove items when we merge and/or split ranges,
// so we need to validate each free list entry before using it.
LargeObject largeObject(LargeObject::DoNotValidate, m_vector[i].begin());
if (!largeObject.isValidAndFree(m_vector[i].size())) {
if (!largeObject.isValidAndFree(owner, m_vector[i].size())) {
m_vector.pop(i);
continue;
}

if (largeObject.size() < size)
continue;

m_vector.pop(i);
return largeObject;
}

return LargeObject();
}

LargeObject FreeList::take(size_t size)
LargeObject FreeList::take(Owner owner, size_t size)
{
LargeObject first;
size_t end = m_vector.size() > freeListSearchDepth ? m_vector.size() - freeListSearchDepth : 0;
for (size_t i = m_vector.size(); i-- > end; ) {
// We don't eagerly remove items when we merge and/or split ranges, so
// we need to validate each free list entry before using it.
LargeObject largeObject(LargeObject::DoNotValidate, m_vector[i].begin());
if (!largeObject.isValidAndFree(m_vector[i].size())) {
if (!largeObject.isValidAndFree(owner, m_vector[i].size())) {
m_vector.pop(i);
continue;
}
Expand All @@ -76,7 +72,7 @@ LargeObject FreeList::take(size_t size)
return first;
}

LargeObject FreeList::take(size_t alignment, size_t size, size_t unalignedSize)
LargeObject FreeList::take(Owner owner, size_t alignment, size_t size, size_t unalignedSize)
{
BASSERT(isPowerOfTwo(alignment));
size_t alignmentMask = alignment - 1;
Expand All @@ -87,7 +83,7 @@ LargeObject FreeList::take(size_t alignment, size_t size, size_t unalignedSize)
// We don't eagerly remove items when we merge and/or split ranges, so
// we need to validate each free list entry before using it.
LargeObject largeObject(LargeObject::DoNotValidate, m_vector[i].begin());
if (!largeObject.isValidAndFree(m_vector[i].size())) {
if (!largeObject.isValidAndFree(owner, m_vector[i].size())) {
m_vector.pop(i);
continue;
}
Expand All @@ -107,11 +103,11 @@ LargeObject FreeList::take(size_t alignment, size_t size, size_t unalignedSize)
return first;
}

void FreeList::removeInvalidAndDuplicateEntries()
void FreeList::removeInvalidAndDuplicateEntries(Owner owner)
{
for (size_t i = m_vector.size(); i-- > 0; ) {
LargeObject largeObject(LargeObject::DoNotValidate, m_vector[i].begin());
if (!largeObject.isValidAndFree(m_vector[i].size())) {
if (!largeObject.isValidAndFree(owner, m_vector[i].size())) {
m_vector.pop(i);
continue;
}
Expand Down
14 changes: 7 additions & 7 deletions Source/bmalloc/bmalloc/FreeList.h
View file
Expand Up @@ -37,14 +37,14 @@ class FreeList {
public:
FreeList();

void push(const LargeObject&);
void push(Owner, const LargeObject&);

LargeObject take(size_t);
LargeObject take(size_t alignment, size_t, size_t unalignedSize);
LargeObject take(Owner, size_t);
LargeObject take(Owner, size_t alignment, size_t, size_t unalignedSize);

LargeObject takeGreedy(size_t);
LargeObject takeGreedy(Owner);

void removeInvalidAndDuplicateEntries();
void removeInvalidAndDuplicateEntries(Owner);

private:
Vector<Range> m_vector;
Expand All @@ -57,11 +57,11 @@ inline FreeList::FreeList()
{
}

inline void FreeList::push(const LargeObject& largeObject)
inline void FreeList::push(Owner owner, const LargeObject& largeObject)
{
BASSERT(largeObject.isFree());
if (m_vector.size() == m_limit) {
removeInvalidAndDuplicateEntries();
removeInvalidAndDuplicateEntries(owner);
m_limit = std::max(m_vector.size() * freeListGrowFactor, freeListSearchDepth);
}
m_vector.push(largeObject.range());
Expand Down

0 comments on commit 76aab4b

Please sign in to comment.