Reland "heap: Use generic flags for main-thread only flags"

This is a reland of 2a8e2a9b

Original change's description:
> heap: Use generic flags for main-thread only flags
>
> BasicMemoryChunk flags should only be mutated from the main thread
> when no concurrent task can access them. For that purpose it is enough
> to use regular non-atomic flags as they are immutable while the GC is
> running.
>
> Change-Id: I0a9f8ecb2eb2aafaf17e77626ae27604abd1b618
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3107230
> Reviewed-by: Hannes Payer <hpayer@chromium.org>
> Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#76471}

Change-Id: Iaf495546f82291ab674537f5d39e10d0495ac612
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3117486
Auto-Submit: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Hannes Payer <hpayer@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#76506}

src/heap/basic-memory-chunk.cc

@@ -25,6 +25,26 @@ STATIC_ASSERT(BasicMemoryChunk::kFlagsOffset ==
 STATIC_ASSERT(BasicMemoryChunk::kHeapOffset ==
               heap_internals::MemoryChunk::kHeapOffset);
 
+// static
+constexpr BasicMemoryChunk::MainThreadFlags BasicMemoryChunk::kAllFlagsMask;
+// static
+constexpr BasicMemoryChunk::MainThreadFlags
+    BasicMemoryChunk::kPointersToHereAreInterestingMask;
+// static
+constexpr BasicMemoryChunk::MainThreadFlags
+    BasicMemoryChunk::kPointersFromHereAreInterestingMask;
+// static
+constexpr BasicMemoryChunk::MainThreadFlags
+    BasicMemoryChunk::kEvacuationCandidateMask;
+// static
+constexpr BasicMemoryChunk::MainThreadFlags
+    BasicMemoryChunk::kIsInYoungGenerationMask;
+// static
+constexpr BasicMemoryChunk::MainThreadFlags BasicMemoryChunk::kIsLargePageMask;
+// static
+constexpr BasicMemoryChunk::MainThreadFlags
+    BasicMemoryChunk::kSkipEvacuationSlotsRecordingMask;
+
 BasicMemoryChunk::BasicMemoryChunk(size_t size, Address area_start,
                                    Address area_end) {
   size_ = size;
@@ -75,13 +95,11 @@ class BasicMemoryChunkValidator {
   STATIC_ASSERT(BasicMemoryChunk::kSizeOffset ==
                 offsetof(BasicMemoryChunk, size_));
   STATIC_ASSERT(BasicMemoryChunk::kFlagsOffset ==
-                offsetof(BasicMemoryChunk, flags_));
+                offsetof(BasicMemoryChunk, main_thread_flags_));
   STATIC_ASSERT(BasicMemoryChunk::kHeapOffset ==
                 offsetof(BasicMemoryChunk, heap_));
   STATIC_ASSERT(offsetof(BasicMemoryChunk, size_) ==
                 MemoryChunkLayout::kSizeOffset);
-  STATIC_ASSERT(offsetof(BasicMemoryChunk, flags_) ==
-                MemoryChunkLayout::kFlagsOffset);
   STATIC_ASSERT(offsetof(BasicMemoryChunk, heap_) ==
                 MemoryChunkLayout::kHeapOffset);
   STATIC_ASSERT(offsetof(BasicMemoryChunk, area_start_) ==

src/heap/basic-memory-chunk.h

@@ -9,6 +9,7 @@
 #include <unordered_map>
 
 #include "src/base/atomic-utils.h"
+#include "src/base/flags.h"
 #include "src/common/globals.h"
 #include "src/flags/flags.h"
 #include "src/heap/marking.h"
@@ -30,7 +31,7 @@ class BasicMemoryChunk {
     }
   };
 
-  enum Flag {
+  enum Flag : uintptr_t {
     NO_FLAGS = 0u,
     IS_EXECUTABLE = 1u << 0,
     POINTERS_TO_HERE_ARE_INTERESTING = 1u << 1,
@@ -105,6 +106,28 @@ class BasicMemoryChunk {
     IN_SHARED_HEAP = 1u << 23,
   };
 
+  using MainThreadFlags = base::Flags<Flag, uintptr_t>;
+
+  static constexpr MainThreadFlags kAllFlagsMask = ~MainThreadFlags(NO_FLAGS);
+
+  static constexpr MainThreadFlags kPointersToHereAreInterestingMask =
+      POINTERS_TO_HERE_ARE_INTERESTING;
+
+  static constexpr MainThreadFlags kPointersFromHereAreInterestingMask =
+      POINTERS_FROM_HERE_ARE_INTERESTING;
+
+  static constexpr MainThreadFlags kEvacuationCandidateMask =
+      EVACUATION_CANDIDATE;
+
+  static constexpr MainThreadFlags kIsInYoungGenerationMask =
+      MainThreadFlags(FROM_PAGE) | MainThreadFlags(TO_PAGE);
+
+  static constexpr MainThreadFlags kIsLargePageMask = LARGE_PAGE;
+
+  static constexpr MainThreadFlags kSkipEvacuationSlotsRecordingMask =
+      MainThreadFlags(kEvacuationCandidateMask) |
+      MainThreadFlags(kIsInYoungGenerationMask);
+
   static const intptr_t kAlignment =
       (static_cast<uintptr_t>(1) << kPageSizeBits);
 
@@ -151,54 +174,20 @@ class BasicMemoryChunk {
 
   void set_owner(BaseSpace* space) { owner_ = space; }
 
-  template <AccessMode access_mode = AccessMode::NON_ATOMIC>
-  void SetFlag(Flag flag) {
-    if (access_mode == AccessMode::NON_ATOMIC) {
-      flags_ |= flag;
-    } else {
-      base::AsAtomicWord::SetBits<uintptr_t>(&flags_, flag, flag);
-    }
+  void SetFlag(Flag flag) { main_thread_flags_ |= flag; }
+  bool IsFlagSet(Flag flag) const { return main_thread_flags_ & flag; }
+  void ClearFlag(Flag flag) {
+    main_thread_flags_ = main_thread_flags_.without(flag);
   }
-
-  template <AccessMode access_mode = AccessMode::NON_ATOMIC>
-  bool IsFlagSet(Flag flag) const {
-    return (GetFlags<access_mode>() & flag) != 0;
-  }
-
-  void ClearFlag(Flag flag) { flags_ &= ~flag; }
-
-  // Set or clear multiple flags at a time. The flags in the mask are set to
-  // the value in "flags", the rest retain the current value in |flags_|.
-  void SetFlags(uintptr_t flags, uintptr_t mask) {
-    flags_ = (flags_ & ~mask) | (flags & mask);
+  void ClearFlags(MainThreadFlags flags) { main_thread_flags_ &= ~flags; }
+  // Set or clear multiple flags at a time. `mask` indicates which flags are
+  // should be replaced with new `flags`.
+  void SetFlags(MainThreadFlags flags, MainThreadFlags mask) {
+    main_thread_flags_ = (main_thread_flags_ & ~mask) | (flags & mask);
   }
 
   // Return all current flags.
-  template <AccessMode access_mode = AccessMode::NON_ATOMIC>
-  uintptr_t GetFlags() const {
-    if (access_mode == AccessMode::NON_ATOMIC) {
-      return flags_;
-    } else {
-      return base::AsAtomicWord::Relaxed_Load(&flags_);
-    }
-  }
-
-  using Flags = uintptr_t;
-
-  static const Flags kPointersToHereAreInterestingMask =
-      POINTERS_TO_HERE_ARE_INTERESTING;
-
-  static const Flags kPointersFromHereAreInterestingMask =
-      POINTERS_FROM_HERE_ARE_INTERESTING;
-
-  static const Flags kEvacuationCandidateMask = EVACUATION_CANDIDATE;
-
-  static const Flags kIsInYoungGenerationMask = FROM_PAGE | TO_PAGE;
-
-  static const Flags kIsLargePageMask = LARGE_PAGE;
-
-  static const Flags kSkipEvacuationSlotsRecordingMask =
-      kEvacuationCandidateMask | kIsInYoungGenerationMask;
+  MainThreadFlags GetFlags() const { return main_thread_flags_; }
 
  private:
   bool InReadOnlySpaceRaw() const { return IsFlagSet(READ_ONLY_HEAP); }
@@ -221,16 +210,13 @@ class BasicMemoryChunk {
     return !IsEvacuationCandidate() && !IsFlagSet(NEVER_ALLOCATE_ON_PAGE);
   }
 
-  template <AccessMode access_mode = AccessMode::NON_ATOMIC>
   bool IsEvacuationCandidate() {
-    DCHECK(!(IsFlagSet<access_mode>(NEVER_EVACUATE) &&
-             IsFlagSet<access_mode>(EVACUATION_CANDIDATE)));
-    return IsFlagSet<access_mode>(EVACUATION_CANDIDATE);
+    DCHECK(!(IsFlagSet(NEVER_EVACUATE) && IsFlagSet(EVACUATION_CANDIDATE)));
+    return IsFlagSet(EVACUATION_CANDIDATE);
   }
 
-  template <AccessMode access_mode = AccessMode::NON_ATOMIC>
   bool ShouldSkipEvacuationSlotRecording() {
-    uintptr_t flags = GetFlags<access_mode>();
+    MainThreadFlags flags = GetFlags();
     return ((flags & kSkipEvacuationSlotsRecordingMask) != 0) &&
            ((flags & COMPACTION_WAS_ABORTED) == 0);
   }
@@ -354,7 +340,9 @@ class BasicMemoryChunk {
   // Overall size of the chunk, including the header and guards.
   size_t size_;
 
-  uintptr_t flags_ = NO_FLAGS;
+  // Flags that are only mutable from the main thread when no concurrent
+  // component (e.g. marker, sweeper) is running.
+  MainThreadFlags main_thread_flags_{NO_FLAGS};
 
   // TODO(v8:7464): Find a way to remove this.
   // This goes against the spirit for the BasicMemoryChunk, but until C++14/17
@@ -393,6 +381,8 @@ class BasicMemoryChunk {
   friend class PagedSpace;
 };
 
+DEFINE_OPERATORS_FOR_FLAGS(BasicMemoryChunk::MainThreadFlags)
+
 STATIC_ASSERT(std::is_standard_layout<BasicMemoryChunk>::value);
 
 }  // namespace internal

src/heap/heap.cc

@@ -7164,7 +7164,7 @@ void Heap::WriteBarrierForRange(HeapObject object, TSlot start_slot,
 
   if (incremental_marking()->IsMarking()) {
     mode |= kDoMarking;
-    if (!source_page->ShouldSkipEvacuationSlotRecording<AccessMode::ATOMIC>()) {
+    if (!source_page->ShouldSkipEvacuationSlotRecording()) {
       mode |= kDoEvacuationSlotRecording;
     }
   }

src/heap/mark-compact-inl.h

@@ -68,7 +68,7 @@ void MarkCompactCollector::RecordSlot(HeapObject object, ObjectSlot slot,
 void MarkCompactCollector::RecordSlot(HeapObject object, HeapObjectSlot slot,
                                       HeapObject target) {
   MemoryChunk* source_page = MemoryChunk::FromHeapObject(object);
-  if (!source_page->ShouldSkipEvacuationSlotRecording<AccessMode::ATOMIC>()) {
+  if (!source_page->ShouldSkipEvacuationSlotRecording()) {
     RecordSlot(source_page, slot, target);
   }
 }
@@ -76,7 +76,7 @@ void MarkCompactCollector::RecordSlot(HeapObject object, HeapObjectSlot slot,
 void MarkCompactCollector::RecordSlot(MemoryChunk* source_page,
                                       HeapObjectSlot slot, HeapObject target) {
   BasicMemoryChunk* target_page = BasicMemoryChunk::FromHeapObject(target);
-  if (target_page->IsEvacuationCandidate<AccessMode::ATOMIC>()) {
+  if (target_page->IsEvacuationCandidate()) {
     if (V8_EXTERNAL_CODE_SPACE_BOOL &&
         target_page->IsFlagSet(MemoryChunk::IS_EXECUTABLE)) {
       RememberedSet<OLD_TO_CODE>::Insert<AccessMode::ATOMIC>(source_page,

src/heap/new-spaces.cc

@@ -57,7 +57,7 @@ bool SemiSpace::EnsureCurrentCapacity() {
       memory_chunk_list_.Remove(current_page);
       // Clear new space flags to avoid this page being treated as a new
       // space page that is potentially being swept.
-      current_page->SetFlags(0, Page::kIsInYoungGenerationMask);
+      current_page->ClearFlags(Page::kIsInYoungGenerationMask);
       heap()->memory_allocator()->Free<MemoryAllocator::kPooledAndQueue>(
           current_page);
       current_page = next_current;
@@ -76,8 +76,7 @@ bool SemiSpace::EnsureCurrentCapacity() {
       DCHECK_NOT_NULL(current_page);
       memory_chunk_list_.PushBack(current_page);
       marking_state->ClearLiveness(current_page);
-      current_page->SetFlags(first_page()->GetFlags(),
-                             static_cast<uintptr_t>(Page::kCopyAllFlags));
+      current_page->SetFlags(first_page()->GetFlags(), Page::kAllFlagsMask);
       heap()->CreateFillerObjectAt(current_page->area_start(),
                                    static_cast<int>(current_page->area_size()),
                                    ClearRecordedSlots::kNo);
@@ -214,7 +213,8 @@ void SemiSpace::ShrinkTo(size_t new_capacity) {
   target_capacity_ = new_capacity;
 }
 
-void SemiSpace::FixPagesFlags(intptr_t flags, intptr_t mask) {
+void SemiSpace::FixPagesFlags(Page::MainThreadFlags flags,
+                              Page::MainThreadFlags mask) {
   for (Page* page : *this) {
     page->set_owner(this);
     page->SetFlags(flags, mask);
@@ -253,8 +253,7 @@ void SemiSpace::RemovePage(Page* page) {
 }
 
 void SemiSpace::PrependPage(Page* page) {
-  page->SetFlags(current_page()->GetFlags(),
-                 static_cast<uintptr_t>(Page::kCopyAllFlags));
+  page->SetFlags(current_page()->GetFlags(), Page::kAllFlagsMask);
   page->set_owner(this);
   memory_chunk_list_.PushFront(page);
   current_capacity_ += Page::kPageSize;
@@ -276,7 +275,7 @@ void SemiSpace::Swap(SemiSpace* from, SemiSpace* to) {
   DCHECK(from->first_page());
   DCHECK(to->first_page());
 
-  intptr_t saved_to_space_flags = to->current_page()->GetFlags();
+  auto saved_to_space_flags = to->current_page()->GetFlags();
 
   // We swap all properties but id_.
   std::swap(from->target_capacity_, to->target_capacity_);
@@ -289,7 +288,7 @@ void SemiSpace::Swap(SemiSpace* from, SemiSpace* to) {
             to->external_backing_store_bytes_);
 
   to->FixPagesFlags(saved_to_space_flags, Page::kCopyOnFlipFlagsMask);
-  from->FixPagesFlags(0, 0);
+  from->FixPagesFlags(Page::NO_FLAGS, Page::NO_FLAGS);
 }
 
 void SemiSpace::set_age_mark(Address mark) {

src/heap/new-spaces.h

@@ -173,7 +173,7 @@ class SemiSpace : public Space {
   void RewindPages(int num_pages);
 
   // Copies the flags into the masked positions on all pages in the space.
-  void FixPagesFlags(intptr_t flags, intptr_t flag_mask);
+  void FixPagesFlags(Page::MainThreadFlags flags, Page::MainThreadFlags mask);
 
   // The currently committed space capacity.
   size_t current_capacity_;

src/heap/spaces.cc

@@ -82,7 +82,7 @@ Page* Page::ConvertNewToOld(Page* old_page) {
   DCHECK(old_page->InNewSpace());
   OldSpace* old_space = old_page->heap()->old_space();
   old_page->set_owner(old_space);
-  old_page->SetFlags(0, static_cast<uintptr_t>(~0));
+  old_page->ClearFlags(Page::kAllFlagsMask);
   Page* new_page = old_space->InitializePage(old_page);
   old_space->AddPage(new_page);
   return new_page;

src/heap/spaces.h

@@ -211,13 +211,11 @@ STATIC_ASSERT(sizeof(std::atomic<intptr_t>) == kSystemPointerSize);
 //   Page* p = Page::FromAllocationAreaAddress(address);
 class Page : public MemoryChunk {
  public:
-  static const intptr_t kCopyAllFlags = ~0;
-
   // Page flags copied from from-space to to-space when flipping semispaces.
-  static const intptr_t kCopyOnFlipFlagsMask =
-      static_cast<intptr_t>(MemoryChunk::POINTERS_TO_HERE_ARE_INTERESTING) |
-      static_cast<intptr_t>(MemoryChunk::POINTERS_FROM_HERE_ARE_INTERESTING) |
-      static_cast<intptr_t>(MemoryChunk::INCREMENTAL_MARKING);
+  static constexpr MainThreadFlags kCopyOnFlipFlagsMask =
+      MainThreadFlags(MemoryChunk::POINTERS_TO_HERE_ARE_INTERESTING) |
+      MainThreadFlags(MemoryChunk::POINTERS_FROM_HERE_ARE_INTERESTING) |
+      MainThreadFlags(MemoryChunk::INCREMENTAL_MARKING);
 
   // Returns the page containing a given address. The address ranges
   // from [page_addr .. page_addr + kPageSize[. This only works if the object