[oilpan] Move address cache to roots marking

Address cache is only actively used during roots marking. For
incremental marking we pass over roots multiple times, each time
potentially requires clearing the cache.

Drive-by:
- Add unit tests
- Rename to AddressCache
- Encapsulate handling fully in AddressCache
- Remove outdated caller

Bug: chromium:757440, chromium:830196
Change-Id: I51780e0bbbb8566ad0c83df98229e15133b42825
Reviewed-on: https://chromium-review.googlesource.com/1044214
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Keishi Hattori <keishi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556439}

third_party/blink/renderer/platform/heap/BUILD.gn

@@ -30,6 +30,8 @@ buildflag_header("blink_heap_buildflags") {
 
 blink_platform_sources("heap") {
   sources = [
+    "address_cache.cc",
+    "address_cache.h",
     "blink_gc.h",
     "blink_gc_memory_dump_provider.cc",
     "blink_gc_memory_dump_provider.h",
@@ -104,6 +106,7 @@ test("blink_heap_unittests") {
 jumbo_source_set("blink_heap_unittests_sources") {
   testonly = true
   sources = [
+    "address_cache_test.cc",
     "blink_gc_memory_dump_provider_test.cc",
     "heap_compact_test.cc",
     "heap_test.cc",

third_party/blink/renderer/platform/heap/address_cache.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "third_party/blink/renderer/platform/heap/address_cache.h"
+
+#include "third_party/blink/renderer/platform/heap/heap_page.h"
+
+namespace blink {
+
+void AddressCache::Flush() {
+  if (has_entries_) {
+    for (size_t i = 0; i < kNumberOfEntries; ++i)
+      entries_[i] = nullptr;
+    has_entries_ = false;
+  }
+  dirty_ = false;
+}
+
+void AddressCache::FlushIfDirty() {
+  if (dirty_) {
+    Flush();
+    dirty_ = false;
+  }
+}
+
+size_t AddressCache::GetHash(Address address) {
+  size_t value = (reinterpret_cast<size_t>(address) >> kBlinkPageSizeLog2);
+  value ^= value >> kNumberOfEntriesLog2;
+  value ^= value >> (kNumberOfEntriesLog2 * 2);
+  value &= kNumberOfEntries - 1;
+  return value & ~1;  // Returns only even number.
+}
+
+bool AddressCache::Lookup(Address address) {
+  DCHECK(enabled_);
+  DCHECK(!dirty_);
+
+  size_t index = GetHash(address);
+  DCHECK(!(index & 1));
+  Address cache_page = RoundToBlinkPageStart(address);
+  if (entries_[index] == cache_page)
+    return entries_[index];
+  if (entries_[index + 1] == cache_page)
+    return entries_[index + 1];
+  return false;
+}
+
+void AddressCache::AddEntry(Address address) {
+  has_entries_ = true;
+  size_t index = GetHash(address);
+  DCHECK(!(index & 1));
+  Address cache_page = RoundToBlinkPageStart(address);
+  entries_[index + 1] = entries_[index];
+  entries_[index] = cache_page;
+}
+
+}  // namespace blink

third_party/blink/renderer/platform/heap/address_cache.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef THIRD_PARTY_BLINK_RENDERER_PLATFORM_HEAP_ADDRESS_CACHE_H_
+#define THIRD_PARTY_BLINK_RENDERER_PLATFORM_HEAP_ADDRESS_CACHE_H_
+
+#include "third_party/blink/renderer/platform/heap/blink_gc.h"
+
+namespace blink {
+
+// Negative cache for addresses outside of Blink's garbage collected heap.
+// - Internally maps to pages (NormalPage) to cover a larger range of addresses.
+// - Requires flushing when adding new addresses.
+class PLATFORM_EXPORT AddressCache {
+  USING_FAST_MALLOC(AddressCache);
+
+ public:
+  AddressCache() : enabled_(false), has_entries_(false), dirty_(false) {
+    // Start by flushing the cache in a non-empty state to initialize all the
+    // cache entries.
+    for (size_t i = 0; i < kNumberOfEntries; ++i)
+      entries_[i] = nullptr;
+  }
+
+  void EnableLookup() { enabled_ = true; }
+  void DisableLookup() { enabled_ = false; }
+
+  void MarkDirty() { dirty_ = true; }
+  void Flush();
+  void FlushIfDirty();
+  bool IsEmpty() { return !has_entries_; }
+
+  // Perform a lookup in the cache. Returns true if the address is guaranteed
+  // to not in Blink's heap and false otherwise.
+  bool Lookup(Address);
+
+  // Add an entry to the cache.
+  void AddEntry(Address);
+
+ private:
+  static constexpr size_t kNumberOfEntriesLog2 = 12;
+  static constexpr size_t kNumberOfEntries = 1 << kNumberOfEntriesLog2;
+
+  static size_t GetHash(Address);
+
+  Address entries_[kNumberOfEntries];
+  bool enabled_ : 1;
+  bool has_entries_ : 1;
+  bool dirty_ : 1;
+};
+
+}  // namespace blink
+
+#endif  // THIRD_PARTY_BLINK_RENDERER_PLATFORM_HEAP_ADDRESS_CACHE_H_

third_party/blink/renderer/platform/heap/address_cache_test.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "third_party/blink/renderer/platform/heap/address_cache.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/renderer/platform/heap/heap_page.h"
+
+namespace blink {
+
+namespace {
+
+const Address kObjectAddress = reinterpret_cast<Address>(kBlinkPageSize);
+
+}  // namespace
+
+TEST(AddressCacheTest, InitialIsEmpty) {
+  AddressCache cache;
+  cache.EnableLookup();
+  EXPECT_TRUE(cache.IsEmpty());
+}
+
+TEST(AddressCacheTest, LookupOnEmpty) {
+  AddressCache cache;
+  cache.EnableLookup();
+  EXPECT_FALSE(cache.Lookup(kObjectAddress));
+}
+
+TEST(AddressCacheTest, LookupAfterAddEntry) {
+  AddressCache cache;
+  cache.EnableLookup();
+  cache.AddEntry(kObjectAddress);
+  EXPECT_TRUE(cache.Lookup(kObjectAddress));
+}
+
+TEST(AddressCacheTest, AddEntryAddsWholePage) {
+  AddressCache cache;
+  cache.EnableLookup();
+  cache.AddEntry(kObjectAddress);
+  for (Address current = kObjectAddress;
+       current < (kObjectAddress + kBlinkPageSize); current++) {
+    EXPECT_TRUE(cache.Lookup(current));
+  }
+}
+
+TEST(AddressCacheTest, AddEntryOnlyAddsPageForGivenAddress) {
+  AddressCache cache;
+  cache.EnableLookup();
+  cache.AddEntry(kObjectAddress);
+  EXPECT_FALSE(cache.Lookup(kObjectAddress - 1));
+  EXPECT_FALSE(cache.Lookup(kObjectAddress + kBlinkPageSize + 1));
+}
+
+TEST(AddressCacheTest, FlushIfDirtyIgnoresNonDirty) {
+  AddressCache cache;
+  cache.EnableLookup();
+  cache.AddEntry(kObjectAddress);
+  cache.FlushIfDirty();
+  // Cannot do lookup in dirty cache.
+  EXPECT_FALSE(cache.IsEmpty());
+}
+
+TEST(AddressCacheTest, FlushIfDirtyHandlesDirty) {
+  AddressCache cache;
+  cache.EnableLookup();
+  cache.AddEntry(kObjectAddress);
+  cache.MarkDirty();
+  cache.FlushIfDirty();
+  // Cannot do lookup in dirty cache.
+  EXPECT_TRUE(cache.IsEmpty());
+}
+
+}  // namespace blink

third_party/blink/renderer/platform/heap/heap.cc

@@ -37,6 +37,7 @@
 #include "base/trace_event/process_memory_dump.h"
 #include "third_party/blink/public/platform/platform.h"
 #include "third_party/blink/renderer/platform/bindings/script_forbidden_scope.h"
+#include "third_party/blink/renderer/platform/heap/address_cache.h"
 #include "third_party/blink/renderer/platform/heap/blink_gc_memory_dump_provider.h"
 #include "third_party/blink/renderer/platform/heap/heap_compact.h"
 #include "third_party/blink/renderer/platform/heap/marking_visitor.h"
@@ -58,10 +59,6 @@ namespace blink {
 HeapAllocHooks::AllocationHook* HeapAllocHooks::allocation_hook_ = nullptr;
 HeapAllocHooks::FreeHook* HeapAllocHooks::free_hook_ = nullptr;
 
-void ThreadHeap::FlushHeapDoesNotContainCache() {
-  heap_does_not_contain_cache_->Flush();
-}
-
 ThreadHeapStats::ThreadHeapStats()
     : allocated_space_(0),
       allocated_object_size_(0),
@@ -132,14 +129,13 @@ double ThreadHeapStats::LiveObjectRateSinceLastGC() const {
 ThreadHeap::ThreadHeap(ThreadState* thread_state)
     : thread_state_(thread_state),
       region_tree_(std::make_unique<RegionTree>()),
-      heap_does_not_contain_cache_(std::make_unique<HeapDoesNotContainCache>()),
+      address_cache_(std::make_unique<AddressCache>()),
       free_page_pool_(std::make_unique<PagePool>()),
       marking_worklist_(nullptr),
       not_fully_constructed_worklist_(nullptr),
       weak_callback_worklist_(nullptr),
       vector_backing_arena_index_(BlinkGC::kVector1ArenaIndex),
-      current_arena_ages_(0),
-      should_flush_heap_does_not_contain_cache_(false) {
+      current_arena_ages_(0) {
   if (ThreadState::Current()->IsMainThread())
     main_thread_heap_ = this;
 
@@ -164,7 +160,7 @@ Address ThreadHeap::CheckAndMarkPointer(MarkingVisitor* visitor,
   DCHECK(thread_state_->InAtomicMarkingPause());
 
 #if !DCHECK_IS_ON()
-  if (heap_does_not_contain_cache_->Lookup(address))
+  if (address_cache_->Lookup(address))
     return nullptr;
 #endif
 
@@ -172,17 +168,17 @@ Address ThreadHeap::CheckAndMarkPointer(MarkingVisitor* visitor,
 #if DCHECK_IS_ON()
     DCHECK(page->Contains(address));
 #endif
-    DCHECK(!heap_does_not_contain_cache_->Lookup(address));
+    DCHECK(!address_cache_->Lookup(address));
     DCHECK(&visitor->Heap() == &page->Arena()->GetThreadState()->Heap());
     visitor->ConservativelyMarkAddress(page, address);
     return address;
   }
 
 #if !DCHECK_IS_ON()
-  heap_does_not_contain_cache_->AddEntry(address);
+  address_cache_->AddEntry(address);
 #else
-  if (!heap_does_not_contain_cache_->Lookup(address))
-    heap_does_not_contain_cache_->AddEntry(address);
+  if (!address_cache_->Lookup(address))
+    address_cache_->AddEntry(address);
 #endif
   return nullptr;
 }
@@ -199,13 +195,13 @@ Address ThreadHeap::CheckAndMarkPointer(
 
   if (BasePage* page = LookupPageForAddress(address)) {
     DCHECK(page->Contains(address));
-    DCHECK(!heap_does_not_contain_cache_->Lookup(address));
+    DCHECK(!address_cache_->Lookup(address));
     DCHECK(&visitor->Heap() == &page->Arena()->GetThreadState()->Heap());
     visitor->ConservativelyMarkAddress(page, address, callback);
     return address;
   }
-  if (!heap_does_not_contain_cache_->Lookup(address))
-    heap_does_not_contain_cache_->AddEntry(address);
+  if (!address_cache_->Lookup(address))
+    address_cache_->AddEntry(address);
   return nullptr;
 }
 #endif  // DCHECK_IS_ON()
@@ -449,26 +445,6 @@ size_t ThreadHeap::ObjectPayloadSizeForTesting() {
   return object_payload_size;
 }
 
-void ThreadHeap::ShouldFlushHeapDoesNotContainCache() {
-  should_flush_heap_does_not_contain_cache_ = true;
-}
-
-void ThreadHeap::FlushHeapDoesNotContainCacheIfNeeded() {
-  if (should_flush_heap_does_not_contain_cache_) {
-    FlushHeapDoesNotContainCache();
-    should_flush_heap_does_not_contain_cache_ = false;
-  }
-}
-
-bool ThreadHeap::IsAddressInHeapDoesNotContainCache(Address address) {
-  // If the cache has been marked as invalidated, it's cleared prior
-  // to performing the next GC. Hence, consider the cache as being
-  // effectively empty.
-  if (should_flush_heap_does_not_contain_cache_)
-    return false;
-  return heap_does_not_contain_cache_->Lookup(address);
-}
-
 void ThreadHeap::VisitPersistentRoots(Visitor* visitor) {
   DCHECK(thread_state_->InAtomicMarkingPause());
   TRACE_EVENT0("blink_gc", "ThreadHeap::visitPersistentRoots");
@@ -478,7 +454,10 @@ void ThreadHeap::VisitPersistentRoots(Visitor* visitor) {
 void ThreadHeap::VisitStackRoots(MarkingVisitor* visitor) {
   DCHECK(thread_state_->InAtomicMarkingPause());
   TRACE_EVENT0("blink_gc", "ThreadHeap::visitStackRoots");
+  address_cache_->FlushIfDirty();
+  address_cache_->EnableLookup();
   thread_state_->VisitStack(visitor);
+  address_cache_->DisableLookup();
 }
 
 BasePage* ThreadHeap::LookupPageForAddress(Address address) {

third_party/blink/renderer/platform/heap/heap.h

@@ -55,6 +55,7 @@ namespace incremental_marking_test {
 class IncrementalMarkingScopeBase;
 }  // namespace incremental_marking_test
 
+class AddressCache;
 class PagePool;
 class RegionTree;
 
@@ -358,10 +359,7 @@ class PLATFORM_EXPORT ThreadHeap {
 
   size_t ObjectPayloadSizeForTesting();
 
-  void FlushHeapDoesNotContainCache();
-  bool IsAddressInHeapDoesNotContainCache(Address);
-  void FlushHeapDoesNotContainCacheIfNeeded();
-  void ShouldFlushHeapDoesNotContainCache();
+  AddressCache* address_cache() { return address_cache_.get(); }
 
   PagePool* GetFreePagePool() { return free_page_pool_.get(); }
 
@@ -499,7 +497,7 @@ class PLATFORM_EXPORT ThreadHeap {
   ThreadState* thread_state_;
   ThreadHeapStats stats_;
   std::unique_ptr<RegionTree> region_tree_;
-  std::unique_ptr<HeapDoesNotContainCache> heap_does_not_contain_cache_;
+  std::unique_ptr<AddressCache> address_cache_;
   std::unique_ptr<PagePool> free_page_pool_;
   std::unique_ptr<MarkingWorklist> marking_worklist_;
   std::unique_ptr<NotFullyConstructedWorklist> not_fully_constructed_worklist_;
@@ -515,7 +513,6 @@ class PLATFORM_EXPORT ThreadHeap {
   int vector_backing_arena_index_;
   size_t arena_ages_[BlinkGC::kNumberOfArenas];
   size_t current_arena_ages_;
-  bool should_flush_heap_does_not_contain_cache_;
 
   // Ideally we want to allocate an array of size |gcInfoTableMax| but it will
   // waste memory. Thus we limit the array size to 2^8 and share one entry

third_party/blink/renderer/platform/heap/heap_page.cc

@@ -33,6 +33,7 @@
 #include "base/trace_event/process_memory_dump.h"
 #include "third_party/blink/public/platform/platform.h"
 #include "third_party/blink/renderer/platform/bindings/script_forbidden_scope.h"
+#include "third_party/blink/renderer/platform/heap/address_cache.h"
 #include "third_party/blink/renderer/platform/heap/blink_gc_memory_dump_provider.h"
 #include "third_party/blink/renderer/platform/heap/heap_compact.h"
 #include "third_party/blink/renderer/platform/heap/marking_verifier.h"
@@ -659,7 +660,7 @@ void NormalPageArena::TakeFreelistSnapshot(const String& dump_name) {
 }
 
 void NormalPageArena::AllocatePage() {
-  GetThreadState()->Heap().ShouldFlushHeapDoesNotContainCache();
+  GetThreadState()->Heap().address_cache()->MarkDirty();
   PageMemory* page_memory =
       GetThreadState()->Heap().GetFreePagePool()->Take(ArenaIndex());
 
@@ -1014,7 +1015,7 @@ Address LargeObjectArena::DoAllocateLargeObjectPage(size_t allocation_size,
   large_object_size += kAllocationGranularity;
 #endif
 
-  GetThreadState()->Heap().ShouldFlushHeapDoesNotContainCache();
+  GetThreadState()->Heap().address_cache()->MarkDirty();
   PageMemory* page_memory = PageMemory::Allocate(
       large_object_size, GetThreadState()->Heap().GetRegionTree());
   Address large_object_address = page_memory->WritableStart();
@@ -1775,44 +1776,4 @@ bool LargeObjectPage::Contains(Address object) {
 }
 #endif
 
-void HeapDoesNotContainCache::Flush() {
-  if (has_entries_) {
-    for (size_t i = 0; i < kNumberOfEntries; ++i)
-      entries_[i] = nullptr;
-    has_entries_ = false;
-  }
-}
-
-size_t HeapDoesNotContainCache::GetHash(Address address) {
-  size_t value = (reinterpret_cast<size_t>(address) >> kBlinkPageSizeLog2);
-  value ^= value >> kNumberOfEntriesLog2;
-  value ^= value >> (kNumberOfEntriesLog2 * 2);
-  value &= kNumberOfEntries - 1;
-  return value & ~1;  // Returns only even number.
-}
-
-bool HeapDoesNotContainCache::Lookup(Address address) {
-  DCHECK(ThreadState::Current()->InAtomicMarkingPause());
-
-  size_t index = GetHash(address);
-  DCHECK(!(index & 1));
-  Address cache_page = RoundToBlinkPageStart(address);
-  if (entries_[index] == cache_page)
-    return entries_[index];
-  if (entries_[index + 1] == cache_page)
-    return entries_[index + 1];
-  return false;
-}
-
-void HeapDoesNotContainCache::AddEntry(Address address) {
-  DCHECK(ThreadState::Current()->InAtomicMarkingPause());
-
-  has_entries_ = true;
-  size_t index = GetHash(address);
-  DCHECK(!(index & 1));
-  Address cache_page = RoundToBlinkPageStart(address);
-  entries_[index + 1] = entries_[index];
-  entries_[index] = cache_page;
-}
-
 }  // namespace blink

third_party/blink/renderer/platform/heap/heap_page.h

@@ -646,54 +646,6 @@ class LargeObjectPage final : public BasePage {
 #endif
 };
 
-// |HeapDoesNotContainCache| provides a fast way to determine whether an
-// aribtrary pointer-sized word can be interpreted as a pointer to an area that
-// is managed by the garbage collected Blink heap. This is a cache of 'pages'
-// that have previously been determined to be wholly outside of the heap. The
-// size of these pages must be smaller than the allocation alignment of the heap
-// pages. We determine off-heap-ness by rounding down the pointer to the nearest
-// page and looking up the page in the cache. If there is a miss in the cache we
-// can determine the status of the pointer precisely using the heap
-// |RegionTree|.
-//
-// This is a negative cache, so it must be flushed when memory is added to the
-// heap.
-class HeapDoesNotContainCache {
-  USING_FAST_MALLOC(HeapDoesNotContainCache);
-
- public:
-  HeapDoesNotContainCache() : has_entries_(false) {
-    // Start by flushing the cache in a non-empty state to initialize all the
-    // cache entries.
-    for (size_t i = 0; i < kNumberOfEntries; ++i)
-      entries_[i] = nullptr;
-  }
-
-  void Flush();
-  bool IsEmpty() { return !has_entries_; }
-
-  // Perform a lookup in the cache.
-  //
-  // If lookup returns false, the argument address was not found in the cache
-  // and it is unknown if the address is in the Blink heap.
-  //
-  // If lookup returns true, the argument address was found in the cache which
-  // means the address is not in the heap.
-  PLATFORM_EXPORT bool Lookup(Address);
-
-  // Add an entry to the cache.
-  PLATFORM_EXPORT void AddEntry(Address);
-
- private:
-  static constexpr size_t kNumberOfEntriesLog2 = 12;
-  static constexpr size_t kNumberOfEntries = 1 << kNumberOfEntriesLog2;
-
-  static size_t GetHash(Address);
-
-  Address entries_[kNumberOfEntries];
-  bool has_entries_;
-};
-
 class FreeList {
   DISALLOW_NEW();
 

third_party/blink/renderer/platform/heap/heap_test.cc

@@ -38,6 +38,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/platform/platform.h"
 #include "third_party/blink/renderer/platform/cross_thread_functional.h"
+#include "third_party/blink/renderer/platform/heap/address_cache.h"
 #include "third_party/blink/renderer/platform/heap/handle.h"
 #include "third_party/blink/renderer/platform/heap/heap.h"
 #include "third_party/blink/renderer/platform/heap/heap_linked_stack.h"
@@ -4010,7 +4011,8 @@ TEST(HeapTest, CheckAndMarkPointer) {
     TestGCScope scope(BlinkGC::kHeapPointersOnStack);
     MarkingVisitor visitor(ThreadState::Current(),
                            MarkingVisitor::kGlobalMarking);
-    heap.FlushHeapDoesNotContainCache();
+    heap.address_cache()->EnableLookup();
+    heap.address_cache()->Flush();
     for (size_t i = 0; i < object_addresses.size(); i++) {
       EXPECT_TRUE(heap.CheckAndMarkPointer(&visitor, object_addresses[i],
                                            ReportMarkedPointer));
@@ -4034,7 +4036,8 @@ TEST(HeapTest, CheckAndMarkPointer) {
     TestGCScope scope(BlinkGC::kHeapPointersOnStack);
     MarkingVisitor visitor(ThreadState::Current(),
                            MarkingVisitor::kGlobalMarking);
-    heap.FlushHeapDoesNotContainCache();
+    heap.address_cache()->EnableLookup();
+    heap.address_cache()->Flush();
     for (size_t i = 0; i < object_addresses.size(); i++) {
       // We would like to assert that checkAndMarkPointer returned false
       // here because the pointers no longer point into a valid object

third_party/blink/renderer/platform/heap/page_memory.h

@@ -169,15 +169,6 @@ class PageMemory {
 
   WARN_UNUSED_RESULT bool Commit() {
     reserved_->MarkPageUsed(WritableStart());
-    // Check that in-use page isn't also marked as being a non-heap page
-    // by the current heap's negative cache. That cache is invalidated
-    // when allocating new pages, but crbug.com/649485 suggests that
-    // we do get out of sync somehow.
-    //
-    // TODO(sof): consider removing check once bug has been diagnosed
-    // and addressed.
-    CHECK(!ThreadState::Current()->Heap().IsAddressInHeapDoesNotContainCache(
-        WritableStart()));
     return writable_.Commit();
   }
 

third_party/blink/renderer/platform/heap/thread_state.cc

@@ -1423,7 +1423,6 @@ void ThreadState::MarkPhasePrologue(BlinkGC::StackState stack_state,
 
   DCHECK(InAtomicMarkingPause());
   Heap().MakeConsistentForGC();
-  Heap().FlushHeapDoesNotContainCacheIfNeeded();
   Heap().ClearArenaAges();
 
   if (marking_type != BlinkGC::kTakeSnapshot)