Changeset 204091 in webkit

Timestamp:

Aug 3, 2016 11:43:15 AM (8 years ago)

Author:

ggaren@apple.com

Message:

[bmalloc] Merging of XLargeRanges can leak the upper range
​https://bugs.webkit.org/show_bug.cgi?id=160403

Reviewed by Michael Saboff.

• bmalloc/Heap.cpp:

(bmalloc::Heap::scavengeLargeObjects): Don't use removePhysical().
Recorded physical size is a performance optimization. It is not the
truth. So it might be zero even if a range contains physical pages.

Instead, iterate each range in the map unconditionally.

The map can shrink when we release the lock, so we must clamp our
iterator each time through the loop.

The map can grow when we release the lock, but we don't care because
growth restarts the scavenger from the beginning.

• bmalloc/XLargeMap.cpp:

(bmalloc::XLargeMap::removePhysical): Deleted. Not used anymore.

• bmalloc/XLargeMap.h:

(bmalloc::XLargeMap::ranges): Added direct access for the sake of
scavengeLargeObjects. (This violates our naming conventions -- I'll do
a rename in a follow-up patch.)

Location:

trunk/Source/bmalloc

Files:

• ChangeLog (modified) (1 diff)
• bmalloc/Heap.cpp (modified) (1 diff)
• bmalloc/XLargeMap.cpp (modified) (1 diff)
• bmalloc/XLargeMap.h (modified) (1 diff)

trunk/Source/bmalloc/ChangeLog

@@ +1 @@
+2016-08-03  Geoffrey Garen  <ggaren@apple.com>
+
+        [bmalloc] Merging of XLargeRanges can leak the upper range
+        https://bugs.webkit.org/show_bug.cgi?id=160403
+
+        Reviewed by Michael Saboff.
+
+        * bmalloc/Heap.cpp:
+        (bmalloc::Heap::scavengeLargeObjects): Don't use removePhysical().
+        Recorded physical size is a performance optimization. It is not the
+        truth. So it might be zero even if a range contains physical pages.
+
+        Instead, iterate each range in the map unconditionally.
+
+        The map can shrink when we release the lock, so we must clamp our
+        iterator each time through the loop.
+
+        The map can grow when we release the lock, but we don't care because
+        growth restarts the scavenger from the beginning.
+
+        * bmalloc/XLargeMap.cpp:
+        (bmalloc::XLargeMap::removePhysical): Deleted. Not used anymore.
+
+        * bmalloc/XLargeMap.h:
+        (bmalloc::XLargeMap::ranges): Added direct access for the sake of
+        scavengeLargeObjects. (This violates our naming conventions -- I'll do
+        a rename in a follow-up patch.)
+
 2016-07-13  Enrica Casucci  <enrica@apple.com>
 

trunk/Source/bmalloc/bmalloc/Heap.cpp

@@ -132 +132 @@
 void Heap::scavengeLargeObjects(std::unique_lock<StaticMutex>& lock, std::chrono::milliseconds sleepDuration)
 {
-    while (XLargeRange range = m_largeFree.removePhysical()) {
+    auto& ranges = m_largeFree.ranges();
+    for (size_t i = ranges.size(); i-- > 0; i = std::min(i, ranges.size())) {
+        auto range = ranges.pop(i);
+
         lock.unlock();
         vmDeallocatePhysicalPagesSloppy(range.begin(), range.size());
         lock.lock();
-        
+
         range.setPhysicalSize(0);
-        m_largeFree.add(range);
+        ranges.push(range);
 
         waitUntilFalse(lock, sleepDuration, m_isAllocatingPages);

trunk/Source/bmalloc/bmalloc/XLargeMap.cpp

@@ -77 +77 @@
 }
 
-XLargeRange XLargeMap::removePhysical()
-{
-    auto it = std::find_if(m_free.begin(), m_free.end(), [](const XLargeRange& range) {
-        return range.physicalSize();
-    });
-
-    if (it == m_free.end())
-        return XLargeRange();
-
-    return m_free.pop(it);
-}
-
 } // namespace bmalloc

trunk/Source/bmalloc/bmalloc/XLargeMap.h

@@ -37 +37 @@
     void add(const XLargeRange&);
     XLargeRange remove(size_t alignment, size_t);
-    XLargeRange removePhysical();
+    Vector<XLargeRange>& ranges() { return m_free; }
 
 private: