Flaky API Test TestWTF.bmalloc.ScavengedMemoryShouldBeReused
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 11 Jul 2019 21:56:32 +0000 (21:56 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 11 Jul 2019 21:56:32 +0000 (21:56 +0000)
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):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@247368 268f45cc-cd09-0410-ab3c-d52691b4dbfc

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

index 73298c6..003c431 100644 (file)
@@ -1,3 +1,35 @@
+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>
 
         [WPE][GTK] Build failure with ENABLE_ACCESSIBILITY=OFF
index 4910233..1ae50a5 100644 (file)
@@ -313,10 +313,17 @@ TEST(bmalloc, ScavengedMemoryShouldBeReused)
     static IsoHeap<double> heap;
 
     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();
             CHECK(ptr);
@@ -325,24 +332,33 @@ TEST(bmalloc, ScavengedMemoryShouldBeReused)
                 break;
         }
 
-        std::set<void*> uniquedPtrs = toptrset(ptrs);
+        std::set<void*> uniquedPtrsOfUpperTiers = toptrset(ptrs);
+        CHECK(ptrs.size() == uniquedPtrsOfUpperTiers.size());
+
+        std::set<void*> uniquedPtrs = uniquedPtrsOfUpperTiers;
+        for (void* ptr : lowerTierPtrs)
+            uniquedPtrs.insert(ptr);
 
-        heap.deallocate(ptr1);
-        for (unsigned i = 0; i < IsoPage<decltype(heap)::Config>::numObjects - 1; i++) {
-            heap.deallocate(ptrs[i]);
-            uniquedPtrs.erase(ptrs[i]);
+        // 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);
         }
 
         scavenge();
         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);
 }
 
 template<size_t N>