Revert "[heap] Reduce size of possibly empty buckets"

This reverts commit 80caf2cf.

Reason for revert: Breaks gpu tests:
https://ci.chromium.org/p/v8/builders/ci/Win%20V8%20FYI%20Release%20(NVIDIA)/5570
# Debug check failed: !possibly_empty_buckets->Contains(bucket_index).

Original change's description:
> [heap] Reduce size of possibly empty buckets
> 
> Before this CL a byte was used per bucket to store whether the bucket
> is possibly empty or not. This CL changes this such that each bucket
> only needs a single bit.
> 
> PossiblyEmptyBuckets is now a word in the page header. If more bits
> are needed than fit into a single word, an external bitmap is
> allocated using AlignedAlloc. Storing this on the page header, allows
> to remove initial_buckets from the SlotSet. The SlotSet allocation is
> then again a power-of-2 in release mode.
> 
> Change-Id: If61fd5cfa153f98757beeb444a530f6e2803fdb6
> Bug: chromium:1023139
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1906376
> Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
> Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#64991}

TBR=ulan@chromium.org,dinfuehr@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: chromium:1023139
Change-Id: Ia90b07b9562af934dacba012da31e4f172f2922d
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1918258Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65001}

src/heap/remembered-set.h

@@ -41,6 +41,20 @@ class RememberedSetOperations {
     return slots;
   }
 
+  template <typename Callback>
+  static int IterateAndTrackEmptyBuckets(
+      SlotSet* slot_set, MemoryChunk* chunk, Callback callback,
+      Worklist<MemoryChunk*, 64>::View empty_chunks) {
+    int slots = 0;
+    if (slot_set != nullptr) {
+      bool found_empty_bucket = false;
+      slots += slot_set->IterateAndTrackEmptyBuckets(
+          chunk->address(), chunk->buckets(), callback, &found_empty_bucket);
+      if (found_empty_bucket) empty_chunks.Push(chunk);
+    }
+    return slots;
+  }
+
   static void Remove(SlotSet* slot_set, MemoryChunk* chunk, Address slot_addr) {
     if (slot_set != nullptr) {
       uintptr_t offset = slot_addr - chunk->address();
@@ -154,16 +168,12 @@ class RememberedSet : public AllStatic {
   static int IterateAndTrackEmptyBuckets(
       MemoryChunk* chunk, Callback callback,
       Worklist<MemoryChunk*, 64>::View empty_chunks) {
-    SlotSet* slot_set = chunk->slot_set<type>();
-    int slots = 0;
-    if (slot_set != nullptr) {
-      PossiblyEmptyBuckets* possibly_empty_buckets =
-          chunk->possibly_empty_buckets();
-      slots += slot_set->IterateAndTrackEmptyBuckets(
-          chunk->address(), chunk->buckets(), callback, possibly_empty_buckets);
-      if (!possibly_empty_buckets->IsEmpty()) empty_chunks.Push(chunk);
-    }
-    return slots;
+    SlotSet* slots = chunk->slot_set<type>();
+    bool empty_bucket_found = false;
+    int slot_count = RememberedSetOperations::IterateAndTrackEmptyBuckets(
+        slots, chunk, callback, empty_chunks);
+    if (empty_bucket_found) empty_chunks.Push(chunk);
+    return slot_count;
   }
 
   static void FreeEmptyBuckets(MemoryChunk* chunk) {
@@ -178,8 +188,7 @@ class RememberedSet : public AllStatic {
     DCHECK(type == OLD_TO_NEW);
     SlotSet* slot_set = chunk->slot_set<type, AccessMode::NON_ATOMIC>();
     if (slot_set != nullptr &&
-        slot_set->CheckPossiblyEmptyBuckets(chunk->buckets(),
-                                            chunk->possibly_empty_buckets())) {
+        slot_set->CheckPossiblyEmptyBuckets(chunk->buckets())) {
       chunk->ReleaseSlotSet<type>();
       return true;
     }

src/heap/scavenger.cc

@@ -373,7 +373,9 @@ void ScavengerCollector::CollectGarbage() {
 #ifdef DEBUG
     RememberedSet<OLD_TO_NEW>::IterateMemoryChunks(
         heap_, [](MemoryChunk* chunk) {
-          DCHECK(chunk->possibly_empty_buckets()->IsEmpty());
+          SlotSet* slot_set = chunk->slot_set<OLD_TO_NEW>();
+          DCHECK_IMPLIES(slot_set != nullptr,
+                         slot_set->IsPossiblyEmptyCleared());
         });
 #endif
   }

src/heap/slot-set.h

@@ -23,105 +23,6 @@ namespace internal {
 
 enum SlotCallbackResult { KEEP_SLOT, REMOVE_SLOT };
 
-// Possibly empty buckets (buckets that do not contain any slots) are discovered
-// by the scavenger. Buckets might become non-empty when promoting objects later
-// or in another thread, so all those buckets need to be revisited.
-// Track possibly empty buckets within a SlotSet in this data structure. The
-// class contains a word-sized bitmap, in case more bits are needed the bitmap
-// is replaced with a pointer to a malloc-allocated bitmap.
-class PossiblyEmptyBuckets {
- public:
-  PossiblyEmptyBuckets() : bitmap_(kNullAddress) {}
-  PossiblyEmptyBuckets(PossiblyEmptyBuckets&& other) V8_NOEXCEPT
-      : bitmap_(other.bitmap_) {
-    other.bitmap_ = kNullAddress;
-  }
-
-  ~PossiblyEmptyBuckets() { Release(); }
-
-  void Initialize() {
-    bitmap_ = kNullAddress;
-    DCHECK(!IsAllocated());
-  }
-
-  void Release() {
-    if (IsAllocated()) {
-      AlignedFree(BitmapArray());
-    }
-    bitmap_ = kNullAddress;
-    DCHECK(!IsAllocated());
-  }
-
-  void Insert(size_t bucket_index, size_t buckets) {
-    if (IsAllocated()) {
-      InsertAllocated(bucket_index);
-    } else if (bucket_index + 1 < kBitsPerWord) {
-      bitmap_ |= static_cast<uintptr_t>(1) << (bucket_index + 1);
-    } else {
-      Allocate(buckets);
-      InsertAllocated(bucket_index);
-    }
-  }
-
-  bool Contains(size_t bucket_index) {
-    if (IsAllocated()) {
-      size_t word_idx = bucket_index / kBitsPerWord;
-      uintptr_t* word = BitmapArray() + word_idx;
-      return *word &
-             (static_cast<uintptr_t>(1) << (bucket_index % kBitsPerWord));
-    } else if (bucket_index + 1 < kBitsPerWord) {
-      return bitmap_ & (static_cast<uintptr_t>(1) << (bucket_index + 1));
-    } else {
-      return false;
-    }
-  }
-
-  bool IsEmpty() { return bitmap_ == kNullAddress; }
-
- private:
-  Address bitmap_;
-  static const Address kPointerTag = 1;
-  static const int kWordSize = sizeof(uintptr_t);
-  static const int kBitsPerWord = kWordSize * kBitsPerByte;
-
-  bool IsAllocated() { return bitmap_ & kPointerTag; }
-
-  void Allocate(size_t buckets) {
-    DCHECK(!IsAllocated());
-    size_t words = WordsForBuckets(buckets);
-    uintptr_t* ptr = reinterpret_cast<uintptr_t*>(
-        AlignedAlloc(words * kWordSize, kSystemPointerSize));
-    ptr[0] = bitmap_ >> 1;
-
-    for (size_t word_idx = 1; word_idx < words; word_idx++) {
-      ptr[word_idx] = 0;
-    }
-    bitmap_ = reinterpret_cast<Address>(ptr) + kPointerTag;
-    DCHECK(IsAllocated());
-  }
-
-  void InsertAllocated(size_t bucket_index) {
-    DCHECK(IsAllocated());
-    size_t word_idx = bucket_index / kBitsPerWord;
-    uintptr_t* word = BitmapArray() + word_idx;
-    *word |= static_cast<uintptr_t>(1) << (bucket_index % kBitsPerWord);
-  }
-
-  static size_t WordsForBuckets(size_t buckets) {
-    return (buckets / kBitsPerByte + kWordSize - 1) & ~(kWordSize - 1);
-  }
-
-  uintptr_t* BitmapArray() {
-    DCHECK(IsAllocated());
-    return reinterpret_cast<uintptr_t*>(bitmap_ & ~kPointerTag);
-  }
-
-  DISALLOW_COPY_AND_ASSIGN(PossiblyEmptyBuckets);
-};
-
-STATIC_ASSERT(std::is_standard_layout<PossiblyEmptyBuckets>::value);
-STATIC_ASSERT(sizeof(PossiblyEmptyBuckets) == kSystemPointerSize);
-
 // Data structure for maintaining a set of slots in a standard (non-large)
 // page.
 // The data structure assumes that the slots are pointer size aligned and
@@ -134,33 +35,42 @@ class SlotSet {
     KEEP_EMPTY_BUCKETS   // An empty bucket will be kept.
   };
 
+  enum class PossiblyEmpty : uint8_t {
+    kYes,       // Bucket is non-null but might be empty.
+    kNoOrNull,  // Bucket is null or cannot be empty.
+  };
+
   SlotSet() = delete;
 
   static SlotSet* Allocate(size_t buckets) {
-    //  SlotSet* slot_set --+
-    //                      |
-    //                      v
-    //    +-----------------+-------------------------+
-    //    | initial buckets |     buckets array       |
-    //    +-----------------+-------------------------+
-    //       pointer-sized    pointer-sized * buckets
+    //  SlotSet* slot_set ----------------------+
+    //                                          |
+    //                                          v
+    // +----------------------+-----------------+-------------------------+
+    // | possibly empty array | initial buckets |     buckets array       |
+    // +----------------------+-----------------+-------------------------+
+    //    1 byte * buckets       pointer-sized    pointer-sized * buckets
     //
     //
     // The SlotSet pointer points to the beginning of the buckets array for
     // faster access in the write barrier. The number of buckets is needed for
     // calculating the size of this data structure.
+    // Since pages can shrink we also store the initial_buckets size.
+    //
+    size_t possibly_empty_array_size = PossiblyEmptyArraySize(buckets);
     size_t buckets_size = buckets * sizeof(Bucket*);
-    size_t size = kInitialBucketsSize + buckets_size;
+    size_t size =
+        possibly_empty_array_size + kInitialBucketsSize + buckets_size;
     void* allocation = AlignedAlloc(size, kSystemPointerSize);
     SlotSet* slot_set = reinterpret_cast<SlotSet*>(
-        reinterpret_cast<uint8_t*>(allocation) + kInitialBucketsSize);
+        reinterpret_cast<uint8_t*>(allocation) + possibly_empty_array_size +
+        kInitialBucketsSize);
     DCHECK(
         IsAligned(reinterpret_cast<uintptr_t>(slot_set), kSystemPointerSize));
-#ifdef DEBUG
     *slot_set->initial_buckets() = buckets;
-#endif
     for (size_t i = 0; i < buckets; i++) {
       *slot_set->bucket(i) = nullptr;
+      *slot_set->possibly_empty(i) = PossiblyEmpty::kNoOrNull;
     }
     return slot_set;
   }
@@ -172,15 +82,17 @@ class SlotSet {
       slot_set->ReleaseBucket(i);
     }
 
-#ifdef DEBUG
     size_t initial_buckets = *slot_set->initial_buckets();
 
+#ifdef DEBUG
     for (size_t i = buckets; i < initial_buckets; i++) {
       DCHECK_NULL(*slot_set->bucket(i));
     }
 #endif
 
-    AlignedFree(reinterpret_cast<uint8_t*>(slot_set) - kInitialBucketsSize);
+    size_t possibly_empty_array_size = PossiblyEmptyArraySize(initial_buckets);
+    AlignedFree(reinterpret_cast<uint8_t*>(slot_set) - kInitialBucketsSize -
+                possibly_empty_array_size);
   }
 
   static size_t BucketsForSize(size_t size) {
@@ -348,12 +260,13 @@ class SlotSet {
   // Assumes that the possibly empty-array was already cleared by
   // CheckPossiblyEmptyBuckets.
   template <typename Callback>
-  size_t IterateAndTrackEmptyBuckets(
-      Address chunk_start, size_t buckets, Callback callback,
-      PossiblyEmptyBuckets* possibly_empty_buckets) {
+  size_t IterateAndTrackEmptyBuckets(Address chunk_start, size_t buckets,
+                                     Callback callback,
+                                     bool* empty_bucket_found) {
     return Iterate(chunk_start, buckets, callback,
-                   [possibly_empty_buckets, buckets](size_t bucket_index) {
-                     possibly_empty_buckets->Insert(bucket_index, buckets);
+                   [this, empty_bucket_found](size_t bucket_index) {
+                     *possibly_empty(bucket_index) = PossiblyEmpty::kYes;
+                     *empty_bucket_found = true;
                    });
   }
 
@@ -370,31 +283,42 @@ class SlotSet {
 
   // Check whether possibly empty buckets are really empty. Empty buckets are
   // freed and the possibly empty state is cleared for all buckets.
-  bool CheckPossiblyEmptyBuckets(size_t buckets,
-                                 PossiblyEmptyBuckets* possibly_empty_buckets) {
+  bool CheckPossiblyEmptyBuckets(size_t buckets) {
     bool empty = true;
     for (size_t bucket_index = 0; bucket_index < buckets; bucket_index++) {
       Bucket* bucket = LoadBucket<AccessMode::NON_ATOMIC>(bucket_index);
       if (bucket) {
-        if (possibly_empty_buckets->Contains(bucket_index)) {
+        if (*possibly_empty(bucket_index) == PossiblyEmpty::kYes) {
           if (bucket->IsEmpty()) {
             ReleaseBucket<AccessMode::NON_ATOMIC>(bucket_index);
           } else {
             empty = false;
           }
+          *possibly_empty(bucket_index) = PossiblyEmpty::kNoOrNull;
         } else {
           empty = false;
         }
       } else {
-        DCHECK(!possibly_empty_buckets->Contains(bucket_index));
+        DCHECK_EQ(*possibly_empty(bucket_index), PossiblyEmpty::kNoOrNull);
       }
     }
 
-    possibly_empty_buckets->Release();
-
     return empty;
   }
 
+  // Check wether all possibly empty entries are cleared. Only used
+  // for testing in debug-builds.
+  bool IsPossiblyEmptyCleared() {
+    size_t buckets = *initial_buckets();
+    for (size_t bucket_index = 0; bucket_index < buckets; bucket_index++) {
+      if (*possibly_empty(bucket_index) != PossiblyEmpty::kNoOrNull) {
+        return false;
+      }
+    }
+
+    return true;
+  }
+
   static const int kCellsPerBucket = 32;
   static const int kCellsPerBucketLog2 = 5;
   static const int kCellSizeBytesLog2 = 2;
@@ -576,15 +500,20 @@ class SlotSet {
     *bit_index = static_cast<int>(slot & (kBitsPerCell - 1));
   }
 
+  static size_t PossiblyEmptyArraySize(size_t buckets) {
+    return (sizeof(PossiblyEmpty) * buckets + (kSystemPointerSize - 1)) /
+           kSystemPointerSize * kSystemPointerSize;
+  }
+
   Bucket** buckets() { return reinterpret_cast<Bucket**>(this); }
   Bucket** bucket(size_t bucket_index) { return buckets() + bucket_index; }
+  PossiblyEmpty* possibly_empty(size_t bucket_index) {
+    return reinterpret_cast<PossiblyEmpty*>(buckets()) - kInitialBucketsSize -
+           1 - bucket_index;
+  }
 
-#ifdef DEBUG
   size_t* initial_buckets() { return reinterpret_cast<size_t*>(this) - 1; }
   static const int kInitialBucketsSize = sizeof(size_t);
-#else
-  static const int kInitialBucketsSize = 0;
-#endif
 };
 
 STATIC_ASSERT(std::is_standard_layout<SlotSet>::value);

src/heap/spaces.cc

@@ -750,8 +750,6 @@ MemoryChunk* MemoryChunk::Initialize(Heap* heap, Address base, size_t size,
     chunk->code_object_registry_ = nullptr;
   }
 
-  chunk->possibly_empty_buckets_.Initialize();
-
   return chunk;
 }
 
@@ -1396,7 +1394,6 @@ void MemoryChunk::ReleaseAllocatedMemoryNeededForWritableChunk() {
     code_object_registry_ = nullptr;
   }
 
-  possibly_empty_buckets_.Release();
   ReleaseSlotSet<OLD_TO_NEW>();
   ReleaseSweepingSlotSet();
   ReleaseSlotSet<OLD_TO_OLD>();

src/heap/spaces.h

@@ -24,7 +24,6 @@
 #include "src/heap/heap.h"
 #include "src/heap/invalidated-slots.h"
 #include "src/heap/marking.h"
-#include "src/heap/slot-set.h"
 #include "src/objects/free-space.h"
 #include "src/objects/heap-object.h"
 #include "src/objects/map.h"
@@ -623,8 +622,7 @@ class MemoryChunk : public BasicMemoryChunk {
       + kSystemPointerSize      // LocalArrayBufferTracker* local_tracker_
       + kIntptrSize  // std::atomic<intptr_t> young_generation_live_byte_count_
       + kSystemPointerSize   // Bitmap* young_generation_bitmap_
-      + kSystemPointerSize   // CodeObjectRegistry* code_object_registry_
-      + kSystemPointerSize;  // PossiblyEmptyBuckets possibly_empty_buckets_
+      + kSystemPointerSize;  // CodeObjectRegistry* code_object_registry_
 
   // Page size in bytes.  This must be a multiple of the OS page size.
   static const int kPageSize = 1 << kPageSizeBits;
@@ -871,10 +869,6 @@ class MemoryChunk : public BasicMemoryChunk {
 
   FreeList* free_list() { return owner()->free_list(); }
 
-  PossiblyEmptyBuckets* possibly_empty_buckets() {
-    return &possibly_empty_buckets_;
-  }
-
  protected:
   static MemoryChunk* Initialize(Heap* heap, Address base, size_t size,
                                  Address area_start, Address area_end,
@@ -970,8 +964,6 @@ class MemoryChunk : public BasicMemoryChunk {
 
   CodeObjectRegistry* code_object_registry_;
 
-  PossiblyEmptyBuckets possibly_empty_buckets_;
-
  private:
   void InitializeReservedMemory() { reservation_.Reset(); }
 

test/unittests/heap/slot-set-unittest.cc

@@ -110,20 +110,6 @@ TEST(SlotSet, Remove) {
   SlotSet::Delete(set, SlotSet::kBucketsRegularPage);
 }
 
-TEST(PossiblyEmptyBuckets, ContainsAndInsert) {
-  static const int kBuckets = 100;
-  PossiblyEmptyBuckets possibly_empty_buckets;
-  possibly_empty_buckets.Insert(0, kBuckets);
-  int last = sizeof(uintptr_t) * kBitsPerByte - 2;
-  possibly_empty_buckets.Insert(last, kBuckets);
-  EXPECT_TRUE(possibly_empty_buckets.Contains(0));
-  EXPECT_TRUE(possibly_empty_buckets.Contains(last));
-  possibly_empty_buckets.Insert(last + 1, kBuckets);
-  EXPECT_TRUE(possibly_empty_buckets.Contains(0));
-  EXPECT_TRUE(possibly_empty_buckets.Contains(last));
-  EXPECT_TRUE(possibly_empty_buckets.Contains(last + 1));
-}
-
 void CheckRemoveRangeOn(uint32_t start, uint32_t end) {
   SlotSet* set = SlotSet::Allocate(SlotSet::kBucketsRegularPage);
   uint32_t first = start == 0 ? 0 : start - kTaggedSize;