cppgc: Wire up discarded size

So far, discarded size was maintained by the sweeper but not wired up
anywere.

Changes in this patch:
- Wire up resident size in heap statistics collection.
- Fix bugs in reporting committed and resident size.
- Sweeper test: Enforce some internal details. The details should not
  not be checked broadly but be kept as a detail to the sweeper
  itself.
- Stats collection: Test that committed and resident set size are
  reported and differ after discarding GCs.

Bug: chromium:1056170
Change-Id: Icf8871c7ea3b28253233485c736b2ca4816fd6f2
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3020971Reviewed-by: Anton Bikineev <bikineev@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#75684}

include/cppgc/heap-statistics.h

@@ -94,9 +94,6 @@ struct HeapStatistics final {
     std::vector<PageStatistics> page_stats;
     /** Statistics for the freelist of the space. */
     FreeListStatistics free_list_stats;
-    /** Statistics for object allocated on the page. Filled only when
-     * NameProvider::HideInternalNames() is false. */
-    std::vector<ObjectStatsEntry> object_statistics;
   };
 
   /** Overall committed amount of memory for the heap. */

src/heap/cppgc-js/cpp-heap.cc

@@ -462,8 +462,10 @@ void CppHeap::TraceEpilogue(TraceSummary* trace_summary) {
                   kIncrementalAndConcurrent,
         compactable_space_handling,
         current_flags_ & TraceFlags::kReduceMemory
-            ? cppgc::internal::FreeMemoryHandling::kDiscardWherePossible
-            : cppgc::internal::FreeMemoryHandling::kDoNotDiscard};
+            ? cppgc::internal::Sweeper::SweepingConfig::FreeMemoryHandling::
+                  kDiscardWherePossible
+            : cppgc::internal::Sweeper::SweepingConfig::FreeMemoryHandling::
+                  kDoNotDiscard};
     DCHECK_IMPLIES(
         !isolate_,
         cppgc::internal::Sweeper::SweepingConfig::SweepingType::kAtomic ==

src/heap/cppgc/garbage-collector.h

@@ -20,6 +20,7 @@ class GarbageCollector {
     using StackState = cppgc::Heap::StackState;
     using MarkingType = Marker::MarkingConfig::MarkingType;
     using SweepingType = Sweeper::SweepingConfig::SweepingType;
+    using FreeMemoryHandling = Sweeper::SweepingConfig::FreeMemoryHandling;
     using IsForcedGC = Marker::MarkingConfig::IsForcedGC;
 
     static constexpr Config ConservativeAtomicConfig() {
@@ -58,6 +59,7 @@ class GarbageCollector {
     StackState stack_state = StackState::kMayContainHeapPointers;
     MarkingType marking_type = MarkingType::kAtomic;
     SweepingType sweeping_type = SweepingType::kAtomic;
+    FreeMemoryHandling free_memory_handling = FreeMemoryHandling::kDoNotDiscard;
     IsForcedGC is_forced_gc = IsForcedGC::kNotForced;
   };
 

src/heap/cppgc/heap-base.cc

@@ -147,7 +147,7 @@ HeapStatistics HeapBase::CollectStatistics(
     HeapStatistics::DetailLevel detail_level) {
   if (detail_level == HeapStatistics::DetailLevel::kBrief) {
     return {stats_collector_->allocated_memory_size(),
-            stats_collector_->allocated_memory_size(),
+            stats_collector_->resident_memory_size(),
             stats_collector_->allocated_object_size(),
             HeapStatistics::DetailLevel::kBrief,
             {},
@@ -156,7 +156,7 @@ HeapStatistics HeapBase::CollectStatistics(
 
   sweeper_.FinishIfRunning();
   object_allocator_.ResetLinearAllocationBuffers();
-  return HeapStatisticsCollector().CollectStatistics(this);
+  return HeapStatisticsCollector().CollectDetailedStatistics(this);
 }
 
 }  // namespace internal

src/heap/cppgc/heap-statistics-collector.cc

@@ -52,6 +52,7 @@ void FinalizePage(HeapStatistics::SpaceStatistics* space_stats,
                   HeapStatistics::PageStatistics** page_stats) {
   if (*page_stats) {
     DCHECK_NOT_NULL(space_stats);
+    space_stats->committed_size_bytes += (*page_stats)->committed_size_bytes;
     space_stats->resident_size_bytes += (*page_stats)->resident_size_bytes;
     space_stats->used_size_bytes += (*page_stats)->used_size_bytes;
   }
@@ -64,6 +65,7 @@ void FinalizeSpace(HeapStatistics* stats,
   FinalizePage(*space_stats, page_stats);
   if (*space_stats) {
     DCHECK_NOT_NULL(stats);
+    stats->committed_size_bytes += (*space_stats)->committed_size_bytes;
     stats->resident_size_bytes += (*space_stats)->resident_size_bytes;
     stats->used_size_bytes += (*space_stats)->used_size_bytes;
   }
@@ -89,7 +91,8 @@ void RecordObjectType(
 
 }  // namespace
 
-HeapStatistics HeapStatisticsCollector::CollectStatistics(HeapBase* heap) {
+HeapStatistics HeapStatisticsCollector::CollectDetailedStatistics(
+    HeapBase* heap) {
   HeapStatistics stats;
   stats.detail_level = HeapStatistics::DetailLevel::kDetailed;
   current_stats_ = &stats;
@@ -142,7 +145,8 @@ bool HeapStatisticsCollector::VisitNormalPage(NormalPage& page) {
 
   current_page_stats_ = InitializePage(current_space_stats_);
   current_page_stats_->committed_size_bytes = kPageSize;
-  current_page_stats_->resident_size_bytes = kPageSize;
+  current_page_stats_->resident_size_bytes =
+      kPageSize - page.discarded_memory();
   return false;
 }
 

src/heap/cppgc/heap-statistics-collector.h

@@ -17,7 +17,7 @@ class HeapStatisticsCollector : private HeapVisitor<HeapStatisticsCollector> {
   friend class HeapVisitor<HeapStatisticsCollector>;
 
  public:
-  HeapStatistics CollectStatistics(HeapBase*);
+  HeapStatistics CollectDetailedStatistics(HeapBase*);
 
  private:
   bool VisitNormalPageSpace(NormalPageSpace&);

src/heap/cppgc/heap.cc

@@ -14,6 +14,7 @@
 #include "src/heap/cppgc/marking-verifier.h"
 #include "src/heap/cppgc/prefinalizer-handler.h"
 #include "src/heap/cppgc/stats-collector.h"
+#include "src/heap/cppgc/sweeper.h"
 
 namespace cppgc {
 
@@ -45,6 +46,8 @@ void Heap::ForceGarbageCollectionSlow(const char* source, const char* reason,
   internal::Heap::From(this)->CollectGarbage(
       {internal::GarbageCollector::Config::CollectionType::kMajor, stack_state,
        MarkingType::kAtomic, SweepingType::kAtomic,
+       internal::GarbageCollector::Config::FreeMemoryHandling::
+           kDiscardWherePossible,
        internal::GarbageCollector::Config::IsForcedGC::kForced});
 }
 
@@ -195,7 +198,8 @@ void Heap::FinalizeGarbageCollection(Config::StackState stack_state) {
   subtle::NoGarbageCollectionScope no_gc(*this);
   const Sweeper::SweepingConfig sweeping_config{
       config_.sweeping_type,
-      Sweeper::SweepingConfig::CompactableSpaceHandling::kSweep};
+      Sweeper::SweepingConfig::CompactableSpaceHandling::kSweep,
+      config_.free_memory_handling};
   sweeper_.Start(sweeping_config);
   in_atomic_pause_ = false;
   sweeper_.NotifyDoneIfNeeded();

src/heap/cppgc/stats-collector.cc

@@ -328,10 +328,18 @@ void StatsCollector::ResetDiscardedMemory() {
   discarded_bytes_.store(0, std::memory_order_relaxed);
 }
 
-size_t StatsCollector::discarded_memory() const {
+size_t StatsCollector::discarded_memory_size() const {
   return discarded_bytes_.load(std::memory_order_relaxed);
 }
 
+size_t StatsCollector::resident_memory_size() const {
+  const auto allocated = allocated_memory_size();
+  const auto discarded = discarded_memory_size();
+  DCHECK_IMPLIES(allocated == 0, discarded == 0);
+  DCHECK_IMPLIES(allocated > 0, allocated > discarded);
+  return allocated - discarded;
+}
+
 void StatsCollector::RecordHistogramSample(ScopeId scope_id_,
                                            v8::base::TimeDelta time) {
   switch (scope_id_) {

src/heap/cppgc/stats-collector.h

@@ -297,7 +297,8 @@ class V8_EXPORT_PRIVATE StatsCollector final {
   void IncrementDiscardedMemory(size_t);
   void DecrementDiscardedMemory(size_t);
   void ResetDiscardedMemory();
-  size_t discarded_memory() const;
+  size_t discarded_memory_size() const;
+  size_t resident_memory_size() const;
 
   void SetMetricRecorder(std::unique_ptr<MetricRecorder> histogram_recorder) {
     metric_recorder_ = std::move(histogram_recorder);

src/heap/cppgc/sweeper.cc

@@ -323,6 +323,8 @@ typename FinalizationBuilder::ResultType SweepNormalPage(
 // - returns (unmaps) empty pages;
 // - merges freelists to the space's freelist.
 class SweepFinalizer final {
+  using FreeMemoryHandling = Sweeper::SweepingConfig::FreeMemoryHandling;
+
  public:
   SweepFinalizer(cppgc::Platform* platform,
                  FreeMemoryHandling free_memory_handling)
@@ -413,6 +415,8 @@ class SweepFinalizer final {
 class MutatorThreadSweeper final : private HeapVisitor<MutatorThreadSweeper> {
   friend class HeapVisitor<MutatorThreadSweeper>;
 
+  using FreeMemoryHandling = Sweeper::SweepingConfig::FreeMemoryHandling;
+
  public:
   MutatorThreadSweeper(SpaceStates* states, cppgc::Platform* platform,
                        FreeMemoryHandling free_memory_handling)
@@ -518,6 +522,8 @@ class ConcurrentSweepTask final : public cppgc::JobTask,
                                   private HeapVisitor<ConcurrentSweepTask> {
   friend class HeapVisitor<ConcurrentSweepTask>;
 
+  using FreeMemoryHandling = Sweeper::SweepingConfig::FreeMemoryHandling;
+
  public:
   ConcurrentSweepTask(HeapBase& heap, SpaceStates* states, Platform* platform,
                       FreeMemoryHandling free_memory_handling)
@@ -637,6 +643,8 @@ class PrepareForSweepVisitor final
 }  // namespace
 
 class Sweeper::SweeperImpl final {
+  using FreeMemoryHandling = Sweeper::SweepingConfig::FreeMemoryHandling;
+
  public:
   SweeperImpl(RawHeap& heap, StatsCollector* stats_collector)
       : heap_(heap),
@@ -658,7 +666,7 @@ class Sweeper::SweeperImpl final {
 
     // If inaccessible memory is touched to check whether it is set up
     // correctly it cannot be discarded.
-    if (!CheckMemoryIsInaccessibleIsNoop()) {
+    if (!CanDiscardMemory()) {
       config_.free_memory_handling = FreeMemoryHandling::kDoNotDiscard;
     }
 

src/heap/cppgc/sweeper.h

@@ -10,6 +10,7 @@
 #include "include/cppgc/heap.h"
 #include "src/base/macros.h"
 #include "src/base/platform/time.h"
+#include "src/heap/cppgc/memory.h"
 
 namespace cppgc {
 
@@ -21,16 +22,12 @@ class HeapBase;
 class ConcurrentSweeperTest;
 class NormalPageSpace;
 
-enum class FreeMemoryHandling : uint8_t {
-  kDoNotDiscard,
-  kDiscardWherePossible
-};
-
 class V8_EXPORT_PRIVATE Sweeper final {
  public:
   struct SweepingConfig {
     using SweepingType = cppgc::Heap::SweepingType;
     enum class CompactableSpaceHandling { kSweep, kIgnore };
+    enum class FreeMemoryHandling { kDoNotDiscard, kDiscardWherePossible };
 
     SweepingType sweeping_type = SweepingType::kIncrementalAndConcurrent;
     CompactableSpaceHandling compactable_space_handling =
@@ -38,6 +35,10 @@ class V8_EXPORT_PRIVATE Sweeper final {
     FreeMemoryHandling free_memory_handling = FreeMemoryHandling::kDoNotDiscard;
   };
 
+  static constexpr bool CanDiscardMemory() {
+    return CheckMemoryIsInaccessibleIsNoop();
+  }
+
   explicit Sweeper(HeapBase&);
   ~Sweeper();
 

test/unittests/heap/cppgc/heap-statistics-collector-unittest.cc

@@ -5,6 +5,8 @@
 #include "src/heap/cppgc/heap-statistics-collector.h"
 
 #include "include/cppgc/heap-statistics.h"
+#include "include/cppgc/persistent.h"
+#include "src/base/logging.h"
 #include "src/base/macros.h"
 #include "src/heap/cppgc/globals.h"
 #include "test/unittests/heap/cppgc/tests.h"
@@ -69,6 +71,7 @@ TEST_F(HeapStatisticsCollectorTest, NonEmptyNormalPage) {
       HeapStatistics::DetailLevel::kDetailed);
   EXPECT_EQ(HeapStatistics::DetailLevel::kDetailed,
             detailed_stats.detail_level);
+  EXPECT_EQ(kPageSize, detailed_stats.committed_size_bytes);
   EXPECT_EQ(kPageSize, detailed_stats.resident_size_bytes);
   EXPECT_EQ(used_size, detailed_stats.used_size_bytes);
   EXPECT_EQ(RawHeap::kNumberOfRegularSpaces, detailed_stats.space_stats.size());
@@ -76,6 +79,7 @@ TEST_F(HeapStatisticsCollectorTest, NonEmptyNormalPage) {
   for (const HeapStatistics::SpaceStatistics& space_stats :
        detailed_stats.space_stats) {
     if (space_stats.page_stats.empty()) {
+      EXPECT_EQ(0u, space_stats.committed_size_bytes);
       EXPECT_EQ(0u, space_stats.resident_size_bytes);
       EXPECT_EQ(0u, space_stats.used_size_bytes);
       continue;
@@ -83,9 +87,11 @@ TEST_F(HeapStatisticsCollectorTest, NonEmptyNormalPage) {
     EXPECT_NE("LargePageSpace", space_stats.name);
     EXPECT_FALSE(found_non_empty_space);
     found_non_empty_space = true;
+    EXPECT_EQ(kPageSize, space_stats.committed_size_bytes);
     EXPECT_EQ(kPageSize, space_stats.resident_size_bytes);
     EXPECT_EQ(used_size, space_stats.used_size_bytes);
     EXPECT_EQ(1u, space_stats.page_stats.size());
+    EXPECT_EQ(kPageSize, space_stats.page_stats.back().committed_size_bytes);
     EXPECT_EQ(kPageSize, space_stats.page_stats.back().resident_size_bytes);
     EXPECT_EQ(used_size, space_stats.page_stats.back().used_size_bytes);
   }
@@ -97,34 +103,79 @@ TEST_F(HeapStatisticsCollectorTest, NonEmptyLargePage) {
       GetHeap()->GetAllocationHandle());
   static constexpr size_t used_size = RoundUp<kAllocationGranularity>(
       kLargeObjectSizeThreshold + sizeof(HeapObjectHeader));
-  static constexpr size_t physical_size =
+  static constexpr size_t committed_size =
       RoundUp<kAllocationGranularity>(used_size + sizeof(LargePage));
   HeapStatistics detailed_stats = Heap::From(GetHeap())->CollectStatistics(
       HeapStatistics::DetailLevel::kDetailed);
   EXPECT_EQ(HeapStatistics::DetailLevel::kDetailed,
             detailed_stats.detail_level);
-  EXPECT_EQ(physical_size, detailed_stats.resident_size_bytes);
+  EXPECT_EQ(committed_size, detailed_stats.committed_size_bytes);
+  EXPECT_EQ(committed_size, detailed_stats.resident_size_bytes);
   EXPECT_EQ(used_size, detailed_stats.used_size_bytes);
   EXPECT_EQ(RawHeap::kNumberOfRegularSpaces, detailed_stats.space_stats.size());
   bool found_non_empty_space = false;
   for (const HeapStatistics::SpaceStatistics& space_stats :
        detailed_stats.space_stats) {
     if (space_stats.page_stats.empty()) {
-      EXPECT_EQ(0u, space_stats.resident_size_bytes);
+      EXPECT_EQ(0u, space_stats.committed_size_bytes);
       EXPECT_EQ(0u, space_stats.used_size_bytes);
       continue;
     }
     EXPECT_EQ("LargePageSpace", space_stats.name);
     EXPECT_FALSE(found_non_empty_space);
     found_non_empty_space = true;
-    EXPECT_EQ(physical_size, space_stats.resident_size_bytes);
+    EXPECT_EQ(committed_size, space_stats.committed_size_bytes);
+    EXPECT_EQ(committed_size, space_stats.resident_size_bytes);
     EXPECT_EQ(used_size, space_stats.used_size_bytes);
     EXPECT_EQ(1u, space_stats.page_stats.size());
-    EXPECT_EQ(physical_size, space_stats.page_stats.back().resident_size_bytes);
+    EXPECT_EQ(committed_size,
+              space_stats.page_stats.back().committed_size_bytes);
+    EXPECT_EQ(committed_size,
+              space_stats.page_stats.back().resident_size_bytes);
     EXPECT_EQ(used_size, space_stats.page_stats.back().used_size_bytes);
   }
   EXPECT_TRUE(found_non_empty_space);
 }
 
+TEST_F(HeapStatisticsCollectorTest, BriefStatisticsWithDiscardingOnNormalPage) {
+  if (!Sweeper::CanDiscardMemory()) return;
+
+  Persistent<GCed<1>> holder =
+      MakeGarbageCollected<GCed<1>>(GetHeap()->GetAllocationHandle());
+  ConservativeMemoryDiscardingGC();
+  HeapStatistics brief_stats = Heap::From(GetHeap())->CollectStatistics(
+      HeapStatistics::DetailLevel::kBrief);
+  // Do not enforce exact resident_size_bytes here as this is an implementation
+  // detail of the sweeper.
+  EXPECT_GT(brief_stats.committed_size_bytes, brief_stats.resident_size_bytes);
+}
+
+TEST_F(HeapStatisticsCollectorTest,
+       DetailedStatisticsWithDiscardingOnNormalPage) {
+  if (!Sweeper::CanDiscardMemory()) return;
+
+  Persistent<GCed<1>> holder =
+      MakeGarbageCollected<GCed<1>>(GetHeap()->GetAllocationHandle());
+  ConservativeMemoryDiscardingGC();
+  HeapStatistics detailed_stats = Heap::From(GetHeap())->CollectStatistics(
+      HeapStatistics::DetailLevel::kDetailed);
+  // Do not enforce exact resident_size_bytes here as this is an implementation
+  // detail of the sweeper.
+  EXPECT_GT(detailed_stats.committed_size_bytes,
+            detailed_stats.resident_size_bytes);
+  bool found_page = false;
+  for (const auto& space_stats : detailed_stats.space_stats) {
+    if (space_stats.committed_size_bytes == 0) continue;
+
+    // We should find a single page here that contains memory that was
+    // discarded.
+    EXPECT_EQ(1u, space_stats.page_stats.size());
+    const auto& page_stats = space_stats.page_stats[0];
+    EXPECT_GT(page_stats.committed_size_bytes, page_stats.resident_size_bytes);
+    found_page = true;
+  }
+  EXPECT_TRUE(found_page);
+}
+
 }  // namespace internal
 }  // namespace cppgc

test/unittests/heap/cppgc/stats-collector-unittest.cc

@@ -236,5 +236,33 @@ TEST_F(StatsCollectorTest, AllocatedMemorySize) {
   EXPECT_EQ(0u, stats.allocated_memory_size());
 }
 
+TEST_F(StatsCollectorTest, DiscardedMemorySize) {
+  EXPECT_EQ(0u, stats.discarded_memory_size());
+  stats.IncrementDiscardedMemory(1024);
+  EXPECT_EQ(1024u, stats.discarded_memory_size());
+  stats.DecrementDiscardedMemory(1024);
+  EXPECT_EQ(0u, stats.discarded_memory_size());
+}
+
+TEST_F(StatsCollectorTest, ResidentMemorySizeWithoutDiscarded) {
+  EXPECT_EQ(0u, stats.resident_memory_size());
+  stats.NotifyAllocatedMemory(1024);
+  EXPECT_EQ(1024u, stats.resident_memory_size());
+  stats.NotifyFreedMemory(1024);
+  EXPECT_EQ(0u, stats.resident_memory_size());
+}
+
+TEST_F(StatsCollectorTest, ResidentMemorySizeWithDiscarded) {
+  EXPECT_EQ(0u, stats.resident_memory_size());
+  stats.NotifyAllocatedMemory(8192);
+  EXPECT_EQ(8192u, stats.resident_memory_size());
+  stats.IncrementDiscardedMemory(4096);
+  EXPECT_EQ(4096u, stats.resident_memory_size());
+  stats.DecrementDiscardedMemory(4096);
+  EXPECT_EQ(8192u, stats.resident_memory_size());
+  stats.NotifyFreedMemory(8192);
+  EXPECT_EQ(0u, stats.resident_memory_size());
+}
+
 }  // namespace internal
 }  // namespace cppgc

test/unittests/heap/cppgc/sweeper-unittest.cc

@@ -373,5 +373,32 @@ TEST_F(SweeperTest, AllocationDuringFinalizationIsNotSwept) {
   EXPECT_EQ(0u, g_destructor_callcount);
 }
 
+TEST_F(SweeperTest, DiscardingNormalPageMemory) {
+  if (!Sweeper::CanDiscardMemory()) return;
+
+  // Test ensures that free list payload is discarded and accounted for on page
+  // level.
+  auto* holder = MakeGarbageCollected<GCed<1>>(GetAllocationHandle());
+  ConservativeMemoryDiscardingGC();
+  auto* page = NormalPage::FromPayload(holder);
+  // Assume the `holder` object is the first on the page for simplifying exact
+  // discarded count.
+  ASSERT_EQ(static_cast<void*>(page->PayloadStart() + sizeof(HeapObjectHeader)),
+            holder);
+  // No other object on the page is live.
+  Address free_list_payload_start =
+      page->PayloadStart() +
+      HeapObjectHeader::FromObject(holder).AllocatedSize() +
+      sizeof(kFreeListEntrySize);
+  uintptr_t start =
+      RoundUp(reinterpret_cast<uintptr_t>(free_list_payload_start),
+              GetPlatform().GetPageAllocator()->CommitPageSize());
+  uintptr_t end = RoundDown(reinterpret_cast<uintptr_t>(page->PayloadEnd()),
+                            GetPlatform().GetPageAllocator()->CommitPageSize());
+  EXPECT_GT(end, start);
+  EXPECT_EQ(page->discarded_memory(), end - start);
+  USE(holder);
+}
+
 }  // namespace internal
 }  // namespace cppgc

test/unittests/heap/cppgc/tests.h

@@ -82,6 +82,16 @@ class TestWithHeap : public TestWithPlatform {
         "Testing", cppgc::Heap::StackState::kMayContainHeapPointers);
   }
 
+  // GC that also discards unused memory and thus changes the resident size
+  // size of the heap and corresponding pages.
+  void ConservativeMemoryDiscardingGC() {
+    internal::Heap::From(GetHeap())->CollectGarbage(
+        {GarbageCollector::Config::CollectionType::kMajor,
+         Heap::StackState::kMayContainHeapPointers, Heap::MarkingType::kAtomic,
+         Heap::SweepingType::kAtomic,
+         GarbageCollector::Config::FreeMemoryHandling::kDiscardWherePossible});
+  }
+
   cppgc::Heap* GetHeap() const { return heap_.get(); }
 
   cppgc::AllocationHandle& GetAllocationHandle() const {