Log Message
[bmalloc] freeableMemory and footprint of Heap are completely broken https://bugs.webkit.org/show_bug.cgi?id=230245 <rdar://problem/83339024>
Reviewed by Geoffrey Garen. This introduced in r279922. The physical size of the newly allocated range was changed from zero to the size of the range on that commit and that causes the numbers wrong. That change itself is correct fix because the range has physical pages attached. That simply activated the bug which was there for a long time. I've added the correction to adjust both numbers with newly allocated region. Also added an optional assertion to check the overflow of those values and the option to log those value change to the console. Fortunately those numbers are used for debugging purpose. Scavenger will dump out those to stderr with its verbose mode. There's no practical cases affected by this bug. Here is the example of footprint logging before fixing the bug: >>> footprint: 18446744073709535232 (-16384) scavenge footprint: 18446744073709518848 (-16384) scavenge footprint: 18446744073709502464 (-16384) scavenge footprint: 18446744073709486080 (-16384) scavenge footprint: 18446744073709469696 (-16384) scavenge footprint: 18446744073709453312 (-16384) scavenge ... It just began with negative number which overflows on unsigned. And following is the one with fix: footprint: 1048576 (1048576) allocateLarge footprint: 2097152 (1048576) allocateLarge footprint: 3145728 (1048576) allocateLarge footprint: 4194304 (1048576) allocateLarge footprint: 5242880 (1048576) allocateLarge footprint: 6291456 (1048576) allocateLarge >>> footprint: 6275072 (-16384) scavenge footprint: 6258688 (-16384) scavenge footprint: 6242304 (-16384) scavenge footprint: 6225920 (-16384) scavenge footprint: 6209536 (-16384) scavenge footprint: 6193152 (-16384) scavenge ... * bmalloc/Heap.cpp: (bmalloc::Heap::adjustStat): (bmalloc::Heap::logStat): (bmalloc::Heap::adjustFreeableMemory): (bmalloc::Heap::adjustFootprint): (bmalloc::Heap::decommitLargeRange): (bmalloc::Heap::scavenge): (bmalloc::Heap::allocateSmallChunk): (bmalloc::Heap::allocateSmallPage): (bmalloc::Heap::deallocateSmallLine): (bmalloc::Heap::splitAndAllocate): (bmalloc::Heap::allocateLarge): (bmalloc::Heap::deallocateLarge): (bmalloc::Heap::externalCommit): (bmalloc::Heap::externalDecommit): * bmalloc/Heap.h:
Modified Paths
Diff
Modified: trunk/Source/bmalloc/ChangeLog (286028 => 286029)
--- trunk/Source/bmalloc/ChangeLog 2021-11-18 22:10:11 UTC (rev 286028)
+++ trunk/Source/bmalloc/ChangeLog 2021-11-18 22:21:54 UTC (rev 286029)
@@ -1,3 +1,65 @@
+2021-11-18 Basuke Suzuki <[email protected]>
+
+ [bmalloc] freeableMemory and footprint of Heap are completely broken
+ https://bugs.webkit.org/show_bug.cgi?id=230245
+ <rdar://problem/83339024>
+
+ Reviewed by Geoffrey Garen.
+
+ This introduced in r279922. The physical size of the newly allocated range was changed from zero
+ to the size of the range on that commit and that causes the numbers wrong. That change itself is
+ correct fix because the range has physical pages attached. That simply activated the bug which was
+ there for a long time.
+
+ I've added the correction to adjust both numbers with newly allocated region. Also added an optional
+ assertion to check the overflow of those values and the option to log those value change to the console.
+
+ Fortunately those numbers are used for debugging purpose. Scavenger will dump out those to stderr
+ with its verbose mode. There's no practical cases affected by this bug.
+
+ Here is the example of footprint logging before fixing the bug:
+
+ >>> footprint: 18446744073709535232 (-16384) scavenge
+ footprint: 18446744073709518848 (-16384) scavenge
+ footprint: 18446744073709502464 (-16384) scavenge
+ footprint: 18446744073709486080 (-16384) scavenge
+ footprint: 18446744073709469696 (-16384) scavenge
+ footprint: 18446744073709453312 (-16384) scavenge
+ ...
+
+ It just began with negative number which overflows on unsigned. And following is the one with fix:
+
+ footprint: 1048576 (1048576) allocateLarge
+ footprint: 2097152 (1048576) allocateLarge
+ footprint: 3145728 (1048576) allocateLarge
+ footprint: 4194304 (1048576) allocateLarge
+ footprint: 5242880 (1048576) allocateLarge
+ footprint: 6291456 (1048576) allocateLarge
+ >>> footprint: 6275072 (-16384) scavenge
+ footprint: 6258688 (-16384) scavenge
+ footprint: 6242304 (-16384) scavenge
+ footprint: 6225920 (-16384) scavenge
+ footprint: 6209536 (-16384) scavenge
+ footprint: 6193152 (-16384) scavenge
+ ...
+
+ * bmalloc/Heap.cpp:
+ (bmalloc::Heap::adjustStat):
+ (bmalloc::Heap::logStat):
+ (bmalloc::Heap::adjustFreeableMemory):
+ (bmalloc::Heap::adjustFootprint):
+ (bmalloc::Heap::decommitLargeRange):
+ (bmalloc::Heap::scavenge):
+ (bmalloc::Heap::allocateSmallChunk):
+ (bmalloc::Heap::allocateSmallPage):
+ (bmalloc::Heap::deallocateSmallLine):
+ (bmalloc::Heap::splitAndAllocate):
+ (bmalloc::Heap::allocateLarge):
+ (bmalloc::Heap::deallocateLarge):
+ (bmalloc::Heap::externalCommit):
+ (bmalloc::Heap::externalDecommit):
+ * bmalloc/Heap.h:
+
2021-11-16 Yusuke Suzuki <[email protected]>
Unreviewed, -latomic is only available for GCC
Modified: trunk/Source/bmalloc/bmalloc/Heap.cpp (286028 => 286029)
--- trunk/Source/bmalloc/bmalloc/Heap.cpp 2021-11-18 22:10:11 UTC (rev 286028)
+++ trunk/Source/bmalloc/bmalloc/Heap.cpp 2021-11-18 22:21:54 UTC (rev 286029)
@@ -99,6 +99,43 @@
return m_footprint;
}
+BINLINE void Heap::adjustStat(size_t& value, ssize_t amount)
+{
+ constexpr bool check = false;
+
+ auto result = value + amount;
+ if constexpr (check) {
+ // This assertion should be enabled by default after fixing all footprint/freeableMemory issues.
+ BASSERT((amount >= 0 && result >= value) || (amount < 0 && result < value));
+ }
+ value = result;
+}
+
+BINLINE void Heap::logStat(size_t value, ssize_t amount, const char* label, const char* note)
+{
+ fprintf(stderr, "%s: %lu (%ld) %s\n", label, value, amount, note);
+}
+
+BINLINE void Heap::adjustFreeableMemory(UniqueLockHolder&, ssize_t amount, const char* note)
+{
+ constexpr bool verbose = false;
+
+ adjustStat(m_freeableMemory, amount);
+
+ if constexpr (verbose)
+ logStat(m_freeableMemory, amount, "freeableMemory", note);
+}
+
+BINLINE void Heap::adjustFootprint(UniqueLockHolder&, ssize_t amount, const char* note)
+{
+ constexpr bool verbose = false;
+
+ adjustStat(m_footprint, amount);
+
+ if constexpr (verbose)
+ logStat(m_footprint, amount, "footprint", note);
+}
+
void Heap::markAllLargeAsEligibile(const LockHolder&)
{
m_largeFree.markAllAsEligibile();
@@ -106,12 +143,12 @@
m_condition.notify_all();
}
-void Heap::decommitLargeRange(UniqueLockHolder&, LargeRange& range, BulkDecommit& decommitter)
+void Heap::decommitLargeRange(UniqueLockHolder& lock, LargeRange& range, BulkDecommit& decommitter)
{
BASSERT(range.hasPhysicalPages());
- m_footprint -= range.totalPhysicalSize();
- m_freeableMemory -= range.totalPhysicalSize();
+ adjustFootprint(lock, -range.totalPhysicalSize(), "decommitLargeRange");
+ adjustFreeableMemory(lock, -range.totalPhysicalSize(), "decommitLargeRange");
decommitter.addLazy(range.begin(), range.physicalEnd() - range.begin());
m_hasPendingDecommits = true;
range.setStartPhysicalSize(0);
@@ -139,8 +176,8 @@
size_t pageSize = bmalloc::pageSize(&list - &m_freePages[0]);
size_t decommitSize = physicalPageSizeSloppy(page->begin()->begin(), pageSize);
- m_freeableMemory -= decommitSize;
- m_footprint -= decommitSize;
+ adjustFootprint(lock, -decommitSize, "scavenge");
+ adjustFreeableMemory(lock, -decommitSize, "scavenge");
decommitter.addEager(page->begin()->begin(), pageSize);
page->setHasPhysicalPages(false);
#if ENABLE_PHYSICAL_PAGE_MAP
@@ -205,7 +242,7 @@
chunk->freePages().push(page);
});
- m_freeableMemory += chunkSize;
+ adjustFreeableMemory(lock, chunkSize, "allocateSmallChunk");
m_scavenger->schedule(0);
@@ -275,10 +312,10 @@
size_t pageSize = bmalloc::pageSize(pageClass);
size_t physicalSize = physicalPageSizeSloppy(page->begin()->begin(), pageSize);
if (page->hasPhysicalPages())
- m_freeableMemory -= physicalSize;
+ adjustFreeableMemory(lock, -physicalSize, "allocateSmallPage");
else {
m_scavenger->scheduleIfUnderMemoryPressure(pageSize);
- m_footprint += physicalSize;
+ adjustFootprint(lock, physicalSize, "allocateSmallPage");
vmAllocatePhysicalPagesSloppy(page->begin()->begin(), pageSize);
page->setHasPhysicalPages(true);
#if ENABLE_PHYSICAL_PAGE_MAP
@@ -314,7 +351,7 @@
size_t pageClass = m_constants.pageClass(page->sizeClass());
- m_freeableMemory += physicalPageSizeSloppy(page->begin()->begin(), pageSize(pageClass));
+ adjustFreeableMemory(lock, physicalPageSizeSloppy(page->begin()->begin(), pageSize(pageClass)), "deallocateSmallLine");
List<SmallPage>::remove(page); // 'page' may be in any thread's line cache.
@@ -490,7 +527,7 @@
if (range.startPhysicalSize() < range.size()) {
m_scavenger->scheduleIfUnderMemoryPressure(range.size());
- m_footprint += range.size() - range.totalPhysicalSize();
+ adjustFootprint(lock, range.size() - range.totalPhysicalSize(), "splitAndAllocate");
vmAllocatePhysicalPagesSloppy(range.begin() + range.startPhysicalSize(), range.size() - range.startPhysicalSize());
range.setStartPhysicalSize(range.size());
range.setTotalPhysicalSize(range.size());
@@ -501,12 +538,12 @@
}
if (prev) {
- m_freeableMemory += prev.totalPhysicalSize();
+ adjustFreeableMemory(lock, prev.totalPhysicalSize(), "splitAndAllocate.prev");
m_largeFree.add(prev);
}
if (next) {
- m_freeableMemory += next.totalPhysicalSize();
+ adjustFreeableMemory(lock, next.totalPhysicalSize(), "splitAndAllocate.next");
m_largeFree.add(next);
}
@@ -552,11 +589,13 @@
ASSERT_OR_RETURN_ON_FAILURE(range);
m_largeFree.add(range);
+ adjustFreeableMemory(lock, range.totalPhysicalSize(), "allocateLarge");
+ adjustFootprint(lock, range.totalPhysicalSize(), "allocateLarge");
+
range = m_largeFree.remove(alignment, size);
}
+ adjustFreeableMemory(lock, -range.totalPhysicalSize(), "allocateLarge.reuse");
- m_freeableMemory -= range.totalPhysicalSize();
-
void* result = splitAndAllocate(lock, range, alignment, size).begin();
ASSERT_OR_RETURN_ON_FAILURE(result);
return result;
@@ -605,11 +644,11 @@
m_scavenger->schedule(size);
}
-void Heap::deallocateLarge(UniqueLockHolder&, void* object)
+void Heap::deallocateLarge(UniqueLockHolder& lock, void* object)
{
size_t size = m_largeAllocated.remove(object);
m_largeFree.add(LargeRange(object, size, size, size, static_cast<char*>(object) + size));
- m_freeableMemory += size;
+ adjustFreeableMemory(lock, size, "deallocateLarge");
m_scavenger->schedule(size);
}
@@ -619,11 +658,11 @@
externalCommit(lock, ptr, size);
}
-void Heap::externalCommit(UniqueLockHolder&, void* ptr, size_t size)
+void Heap::externalCommit(UniqueLockHolder& lock, void* ptr, size_t size)
{
BUNUSED_PARAM(ptr);
- m_footprint += size;
+ adjustFootprint(lock, size, "externalCommit");
#if ENABLE_PHYSICAL_PAGE_MAP
m_physicalPageMap.commit(ptr, size);
#endif
@@ -635,11 +674,11 @@
externalDecommit(lock, ptr, size);
}
-void Heap::externalDecommit(UniqueLockHolder&, void* ptr, size_t size)
+void Heap::externalDecommit(UniqueLockHolder& lock, void* ptr, size_t size)
{
BUNUSED_PARAM(ptr);
- m_footprint -= size;
+ adjustFootprint(lock, -size, "externalDecommit");
#if ENABLE_PHYSICAL_PAGE_MAP
m_physicalPageMap.decommit(ptr, size);
#endif
Modified: trunk/Source/bmalloc/bmalloc/Heap.h (286028 => 286029)
--- trunk/Source/bmalloc/bmalloc/Heap.h 2021-11-18 22:10:11 UTC (rev 286028)
+++ trunk/Source/bmalloc/bmalloc/Heap.h 2021-11-18 22:21:54 UTC (rev 286029)
@@ -117,6 +117,11 @@
LargeRange tryAllocateLargeChunk(size_t alignment, size_t);
LargeRange splitAndAllocate(UniqueLockHolder&, LargeRange&, size_t alignment, size_t);
+ inline void adjustFootprint(UniqueLockHolder&, ssize_t, const char* note);
+ inline void adjustFreeableMemory(UniqueLockHolder&, ssize_t, const char* note);
+ inline void adjustStat(size_t& value, ssize_t);
+ inline void logStat(size_t value, ssize_t amount, const char* label, const char* note);
+
HeapKind m_kind;
HeapConstants& m_constants;
_______________________________________________ webkit-changes mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-changes
