Changeset 247368 in webkit

Timestamp:

Jul 11, 2019 2:56:32 PM (5 years ago)

Author:

ysuzuki@apple.com

Message:

Flaky API Test TestWTF.bmalloc.ScavengedMemoryShouldBeReused
​https://bugs.webkit.org/show_bug.cgi?id=199524
<rdar://problem/52783816>

Reviewed by Saam Barati.

This test is white-box one and it has strong assumption how IsoHeap allocates pages.
But this test has several problems.

1. IsoPage::numObjects is not the exact number of how many we allocate objects. This number is calculated by pageSize / sizeof(T), and this does not account the header size of IsoPage. So, # of objects per IsoPage is less than numObjects. Since sizeof(double) is very small, we can have many objects in one IsoPage. As a result, we need a large bitmap in IsoPage. This reduces # of objects in IsoPage largely. So, ptrs.size() becomes less than numObjects.

2. We now have lower tier of allocation in IsoHeap. It means that we allocate 8 objects in shared page (page is shared, but memory is pinned for a specific type) before using IsoHeap's page. This also makes the intention of this test wrong.

Due to (1), we access OoB of ptrs vector, passing a garbage to IsoHeap::deallocate, and crashing.

We make this test robust while we still keep this test white-box one to test the critical feature
of IsoHeap. We first exhaust lower tier of IsoHeap, and after that, start testing the memory. We
allocate many pointers, deallocate them, allocate one pointer while keeping pointers in the lower
tier live, and check whether the deallocated memory is reused.

• TestWebKitAPI/Tests/WTF/bmalloc/IsoHeap.cpp:

(TEST):

Location:

trunk/Tools

Files:

• ChangeLog (modified) (1 diff)
• TestWebKitAPI/Tests/WTF/bmalloc/IsoHeap.cpp (modified) (3 diffs)

trunk/Tools/ChangeLog

@@ +1 @@
+2019-07-11  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        Flaky API Test TestWTF.bmalloc.ScavengedMemoryShouldBeReused
+        https://bugs.webkit.org/show_bug.cgi?id=199524
+        <rdar://problem/52783816>
+
+        Reviewed by Saam Barati.
+
+        This test is white-box one and it has strong assumption how IsoHeap allocates pages.
+        But this test has several problems.
+
+        1. IsoPage::numObjects is not the exact number of how many we allocate objects. This
+           number is calculated by pageSize / sizeof(T), and this does not account the header
+           size of IsoPage. So, # of objects per IsoPage is less than numObjects. Since sizeof(double)
+           is very small, we can have many objects in one IsoPage. As a result, we need a large
+           bitmap in IsoPage. This reduces # of objects in IsoPage largely. So, `ptrs.size()` becomes
+           less than numObjects.
+
+        2. We now have lower tier of allocation in IsoHeap. It means that we allocate 8 objects in
+           shared page (page is shared, but memory is pinned for a specific type) before using IsoHeap's
+           page. This also makes the intention of this test wrong.
+
+        Due to (1), we access OoB of ptrs vector, passing a garbage to IsoHeap::deallocate, and crashing.
+
+        We make this test robust while we still keep this test white-box one to test the critical feature
+        of IsoHeap. We first exhaust lower tier of IsoHeap, and after that, start testing the memory. We
+        allocate many pointers, deallocate them, allocate one pointer while keeping pointers in the lower
+        tier live, and check whether the deallocated memory is reused.
+
+        * TestWebKitAPI/Tests/WTF/bmalloc/IsoHeap.cpp:
+        (TEST):
+
 2019-07-11  Pablo Saavedra  <psaavedra@igalia.com>
 

trunk/Tools/TestWebKitAPI/Tests/WTF/bmalloc/IsoHeap.cpp

@@ -314 +314 @@
 
     auto run = [] (unsigned numPagesToCommit) {
-        auto* ptr1 = heap.allocate();
-
+        std::vector<void*> lowerTierPtrs;
         std::vector<void*> ptrs;
 
+        // Let's exhaust the capacity of the lower tier.
+        for (unsigned i = 0; i < IsoPage<decltype(heap)::Config>::numObjects; ++i) {
+            void* ptr = heap.allocate();
+            CHECK(ptr);
+            lowerTierPtrs.push_back(ptr);
+        }
+
+        // After that, allocating pointers in the upper tier.
         for (unsigned i = 0; ;i++) {
             void* ptr = heap.allocate();
@@ -326 +333 @@
         }
 
-        std::set<void*> uniquedPtrs = toptrset(ptrs);
-
-        heap.deallocate(ptr1);
-        for (unsigned i = 0; i < IsoPage<decltype(heap)::Config>::numObjects - 1; i++) {
-            heap.deallocate(ptrs[i]);
-            uniquedPtrs.erase(ptrs[i]);
+        std::set<void*> uniquedPtrsOfUpperTiers = toptrset(ptrs);
+        CHECK(ptrs.size() == uniquedPtrsOfUpperTiers.size());
+
+        std::set<void*> uniquedPtrs = uniquedPtrsOfUpperTiers;
+        for (void* ptr : lowerTierPtrs)
+            uniquedPtrs.insert(ptr);
+
+        // We do keep pointers in the lower tier while deallocating pointers in the upper tier.
+        // Then, after the scavenge, the pages of the upper tier should be reused.
+
+        for (void* ptr : ptrs) {
+            heap.deallocate(ptr);
+            uniquedPtrs.erase(ptr);
         }
 
@@ -337 +351 @@
         assertHasOnlyObjects(heap, uniquedPtrs);
 
-        // FIXME: This only seems to pass when lldb is attached but the scavenger thread isn't running...
-        // see: https://bugs.webkit.org/show_bug.cgi?id=198384
-        // auto* ptr2 = heap.allocate();
-        // CHECK(ptr1 == ptr2);
+        auto* ptr2 = heap.allocate();
+        CHECK(uniquedPtrsOfUpperTiers.find(ptr2) != uniquedPtrsOfUpperTiers.end());
+        heap.deallocate(ptr2);
+
+        for (void* ptr : lowerTierPtrs)
+            heap.deallocate(ptr);
     };
 
-    run(2);
+    run(5);
 }