⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 286029 in webkit


Ignore:
Timestamp:
Nov 18, 2021, 2:21:54 PM (5 years ago)
Author:
basuke.suzuki@sony.com
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:
Location:
trunk/Source/bmalloc
Files:
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/bmalloc/ChangeLog

    r285880 r286029  
     12021-11-18  Basuke Suzuki  <basuke.suzuki@sony.com>
     2
     3        [bmalloc] freeableMemory and footprint of Heap are completely broken
     4        https://bugs.webkit.org/show_bug.cgi?id=230245
     5        <rdar://problem/83339024>
     6
     7        Reviewed by Geoffrey Garen.
     8
     9        This introduced in r279922. The physical size of the newly allocated range was changed from zero
     10        to the size of the range on that commit and that causes the numbers wrong. That change itself is
     11        correct fix because the range has physical pages attached. That simply activated the bug which was
     12        there for a long time.
     13
     14        I've added the correction to adjust both numbers with newly allocated region. Also added an optional
     15        assertion to check the overflow of those values and the option to log those value change to the console.
     16
     17        Fortunately those numbers are used for debugging purpose. Scavenger will dump out those to stderr
     18        with its verbose mode. There's no practical cases affected by this bug.
     19
     20        Here is the example of footprint logging before fixing the bug:
     21
     22                >>> footprint: 18446744073709535232 (-16384) scavenge
     23                footprint: 18446744073709518848 (-16384) scavenge
     24                footprint: 18446744073709502464 (-16384) scavenge
     25                footprint: 18446744073709486080 (-16384) scavenge
     26                footprint: 18446744073709469696 (-16384) scavenge
     27                footprint: 18446744073709453312 (-16384) scavenge
     28                ...
     29
     30        It just began with negative number which overflows on unsigned. And following is the one with fix:
     31
     32                footprint: 1048576 (1048576) allocateLarge
     33                footprint: 2097152 (1048576) allocateLarge
     34                footprint: 3145728 (1048576) allocateLarge
     35                footprint: 4194304 (1048576) allocateLarge
     36                footprint: 5242880 (1048576) allocateLarge
     37                footprint: 6291456 (1048576) allocateLarge
     38                >>> footprint: 6275072 (-16384) scavenge
     39                footprint: 6258688 (-16384) scavenge
     40                footprint: 6242304 (-16384) scavenge
     41                footprint: 6225920 (-16384) scavenge
     42                footprint: 6209536 (-16384) scavenge
     43                footprint: 6193152 (-16384) scavenge
     44                ...
     45
     46        * bmalloc/Heap.cpp:
     47        (bmalloc::Heap::adjustStat):
     48        (bmalloc::Heap::logStat):
     49        (bmalloc::Heap::adjustFreeableMemory):
     50        (bmalloc::Heap::adjustFootprint):
     51        (bmalloc::Heap::decommitLargeRange):
     52        (bmalloc::Heap::scavenge):
     53        (bmalloc::Heap::allocateSmallChunk):
     54        (bmalloc::Heap::allocateSmallPage):
     55        (bmalloc::Heap::deallocateSmallLine):
     56        (bmalloc::Heap::splitAndAllocate):
     57        (bmalloc::Heap::allocateLarge):
     58        (bmalloc::Heap::deallocateLarge):
     59        (bmalloc::Heap::externalCommit):
     60        (bmalloc::Heap::externalDecommit):
     61        * bmalloc/Heap.h:
     62
    1632021-11-16  Yusuke Suzuki  <ysuzuki@apple.com>
    264

  • trunk/Source/bmalloc/bmalloc/Heap.cpp

    r285853 r286029  
    100100}
    101101
     102BINLINE void Heap::adjustStat(size_t& value, ssize_t amount)
     103{
     104    constexpr bool check = false;
     105
     106    auto result = value + amount;
     107    if constexpr (check) {
     108        // This assertion should be enabled by default after fixing all footprint/freeableMemory issues.
     109        BASSERT((amount >= 0 && result >= value) || (amount < 0 && result < value));
     110    }
     111    value = result;
     112}
     113
     114BINLINE void Heap::logStat(size_t value, ssize_t amount, const char* label, const char* note)
     115{
     116    fprintf(stderr, "%s: %lu (%ld) %s\n", label, value, amount, note);
     117}
     118
     119BINLINE void Heap::adjustFreeableMemory(UniqueLockHolder&, ssize_t amount, const char* note)
     120{
     121    constexpr bool verbose = false;
     122
     123    adjustStat(m_freeableMemory, amount);
     124
     125    if constexpr (verbose)
     126        logStat(m_freeableMemory, amount, "freeableMemory", note);
     127}
     128
     129BINLINE void Heap::adjustFootprint(UniqueLockHolder&, ssize_t amount, const char* note)
     130{
     131    constexpr bool verbose = false;
     132
     133    adjustStat(m_footprint, amount);
     134
     135    if constexpr (verbose)
     136        logStat(m_footprint, amount, "footprint", note);
     137}
     138
    102139void Heap::markAllLargeAsEligibile(const LockHolder&)
    103140{
    … …  
    107144}
    108145
    109 void Heap::decommitLargeRange(UniqueLockHolder&, LargeRange& range, BulkDecommit& decommitter)
     146void Heap::decommitLargeRange(UniqueLockHolder& lock, LargeRange& range, BulkDecommit& decommitter)
    110147{
    111148    BASSERT(range.hasPhysicalPages());
    112149
    113     m_footprint -= range.totalPhysicalSize();
    114     m_freeableMemory -= range.totalPhysicalSize();
     150    adjustFootprint(lock, -range.totalPhysicalSize(), "decommitLargeRange");
     151    adjustFreeableMemory(lock, -range.totalPhysicalSize(), "decommitLargeRange");
    115152    decommitter.addLazy(range.begin(), range.physicalEnd() - range.begin());
    116153    m_hasPendingDecommits = true;
    … …  
    140177                size_t pageSize = bmalloc::pageSize(&list - &m_freePages[0]);
    141178                size_t decommitSize = physicalPageSizeSloppy(page->begin()->begin(), pageSize);
    142                 m_freeableMemory -= decommitSize;
    143                 m_footprint -= decommitSize;
     179                adjustFootprint(lock, -decommitSize, "scavenge");
     180                adjustFreeableMemory(lock, -decommitSize, "scavenge");
    144181                decommitter.addEager(page->begin()->begin(), pageSize);
    145182                page->setHasPhysicalPages(false);
    … …  
    206243        });
    207244
    208         m_freeableMemory += chunkSize;
     245        adjustFreeableMemory(lock, chunkSize, "allocateSmallChunk");
    209246
    210247        m_scavenger->schedule(0);
    … …  
    276313        size_t physicalSize = physicalPageSizeSloppy(page->begin()->begin(), pageSize);
    277314        if (page->hasPhysicalPages())
    278             m_freeableMemory -= physicalSize;
     315            adjustFreeableMemory(lock, -physicalSize, "allocateSmallPage");
    279316        else {
    280317            m_scavenger->scheduleIfUnderMemoryPressure(pageSize);
    281             m_footprint += physicalSize;
     318            adjustFootprint(lock, physicalSize, "allocateSmallPage");
    282319            vmAllocatePhysicalPagesSloppy(page->begin()->begin(), pageSize);
    283320            page->setHasPhysicalPages(true);
    … …  
    315352    size_t pageClass = m_constants.pageClass(page->sizeClass());
    316353
    317     m_freeableMemory += physicalPageSizeSloppy(page->begin()->begin(), pageSize(pageClass));
     354    adjustFreeableMemory(lock, physicalPageSizeSloppy(page->begin()->begin(), pageSize(pageClass)), "deallocateSmallLine");
    318355
    319356    List<SmallPage>::remove(page); // 'page' may be in any thread's line cache.
    … …  
    491528    if (range.startPhysicalSize() < range.size()) {
    492529        m_scavenger->scheduleIfUnderMemoryPressure(range.size());
    493         m_footprint += range.size() - range.totalPhysicalSize();
     530        adjustFootprint(lock, range.size() - range.totalPhysicalSize(), "splitAndAllocate");
    494531        vmAllocatePhysicalPagesSloppy(range.begin() + range.startPhysicalSize(), range.size() - range.startPhysicalSize());
    495532        range.setStartPhysicalSize(range.size());
    … …  
    502539   
    503540    if (prev) {
    504         m_freeableMemory += prev.totalPhysicalSize();
     541        adjustFreeableMemory(lock, prev.totalPhysicalSize(), "splitAndAllocate.prev");
    505542        m_largeFree.add(prev);
    506543    }
    507544
    508545    if (next) {
    509         m_freeableMemory += next.totalPhysicalSize();
     546        adjustFreeableMemory(lock, next.totalPhysicalSize(), "splitAndAllocate.next");
    510547        m_largeFree.add(next);
    511548    }
    … …  
    553590       
    554591        m_largeFree.add(range);
     592        adjustFreeableMemory(lock, range.totalPhysicalSize(), "allocateLarge");
     593        adjustFootprint(lock, range.totalPhysicalSize(), "allocateLarge");
     594
    555595        range = m_largeFree.remove(alignment, size);
    556596    }
    557 
    558     m_freeableMemory -= range.totalPhysicalSize();
     597    adjustFreeableMemory(lock, -range.totalPhysicalSize(), "allocateLarge.reuse");
    559598
    560599    void* result = splitAndAllocate(lock, range, alignment, size).begin();
    … …  
    606645}
    607646
    608 void Heap::deallocateLarge(UniqueLockHolder&, void* object)
     647void Heap::deallocateLarge(UniqueLockHolder& lock, void* object)
    609648{
    610649    size_t size = m_largeAllocated.remove(object);
    611650    m_largeFree.add(LargeRange(object, size, size, size, static_cast<char*>(object) + size));
    612     m_freeableMemory += size;
     651    adjustFreeableMemory(lock, size, "deallocateLarge");
    613652    m_scavenger->schedule(size);
    614653}
    … …  
    620659}
    621660
    622 void Heap::externalCommit(UniqueLockHolder&, void* ptr, size_t size)
     661void Heap::externalCommit(UniqueLockHolder& lock, void* ptr, size_t size)
    623662{
    624663    BUNUSED_PARAM(ptr);
    625664
    626     m_footprint += size;
     665    adjustFootprint(lock, size, "externalCommit");
    627666#if ENABLE_PHYSICAL_PAGE_MAP
    628667    m_physicalPageMap.commit(ptr, size);
    … …  
    636675}
    637676
    638 void Heap::externalDecommit(UniqueLockHolder&, void* ptr, size_t size)
     677void Heap::externalDecommit(UniqueLockHolder& lock, void* ptr, size_t size)
    639678{
    640679    BUNUSED_PARAM(ptr);
    641680
    642     m_footprint -= size;
     681    adjustFootprint(lock, -size, "externalDecommit");
    643682#if ENABLE_PHYSICAL_PAGE_MAP
    644683    m_physicalPageMap.decommit(ptr, size);

  • trunk/Source/bmalloc/bmalloc/Heap.h

    r283264 r286029  
    118118    LargeRange splitAndAllocate(UniqueLockHolder&, LargeRange&, size_t alignment, size_t);
    119119
     120    inline void adjustFootprint(UniqueLockHolder&, ssize_t, const char* note);
     121    inline void adjustFreeableMemory(UniqueLockHolder&, ssize_t, const char* note);
     122    inline void adjustStat(size_t& value, ssize_t);
     123    inline void logStat(size_t value, ssize_t amount, const char* label, const char* note);
     124
    120125    HeapKind m_kind;
    121126    HeapConstants& m_constants;
Note: See TracChangeset for help on using the changeset viewer.