[PA] Commit slot span pages lazily

Currently pages are committed as soon as the slot span is allocated.
There is already code that cleverly avoids writing in-slot free list
entries on pages of the span that aren't used yet, to prevent them from
being faulted in. This code can be easily repurposed to commit pages
as needed. An adjustment had to be made to not provision a slot (and add
it to the free list) if it doesn't fully fit in a committed page
(previously it was enough for the slot's free list entry to fit within
the page).

Change-Id: Ic9585aa6001a1cf53233ae5edc4932be780a93ad
Cq-Do-Not-Cancel-Tryjobs: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2563385
Commit-Queue: Bartek Nowierski <bartekn@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Anton Bikineev <bikineev@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833127}

base/allocator/partition_allocator/partition_alloc_unittest.cc

@@ -1120,8 +1120,46 @@ TEST_F(PartitionAllocTest, PartialPageFreelists) {
   EXPECT_TRUE(slot_span2->freelist_head);
   EXPECT_EQ(0, slot_span2->num_allocated_slots);
 
-  // And test a couple of sizes that do not cross SystemPageSize() with a single
-  // allocation.
+  // Size that doesn't divide the system page size.
+  size_t non_dividing_size = 2100 - kExtraAllocSize;
+  bucket_index = SizeToIndex(non_dividing_size + kExtraAllocSize);
+  bucket = &allocator.root()->buckets[bucket_index];
+  EXPECT_EQ(nullptr, bucket->empty_slot_spans_head);
+
+  ptr = allocator.root()->Alloc(non_dividing_size, type_name);
+  EXPECT_TRUE(ptr);
+
+  slot_span = SlotSpan::FromPointer(
+      allocator.root()->AdjustPointerForExtrasSubtract(ptr));
+  total_slots =
+      (slot_span->bucket->num_system_pages_per_slot_span * SystemPageSize()) /
+      bucket->slot_size;
+  EXPECT_EQ(16u, total_slots);
+  EXPECT_FALSE(slot_span->freelist_head);
+  EXPECT_EQ(1, slot_span->num_allocated_slots);
+  EXPECT_EQ(15, slot_span->num_unprovisioned_slots);
+
+  ptr2 = allocator.root()->Alloc(non_dividing_size, type_name);
+  EXPECT_TRUE(ptr2);
+  EXPECT_TRUE(slot_span->freelist_head);
+  EXPECT_EQ(2, slot_span->num_allocated_slots);
+  EXPECT_EQ(13, slot_span->num_unprovisioned_slots);
+
+  ptr3 = allocator.root()->Alloc(non_dividing_size, type_name);
+  EXPECT_TRUE(ptr3);
+  EXPECT_FALSE(slot_span->freelist_head);
+  EXPECT_EQ(3, slot_span->num_allocated_slots);
+  EXPECT_EQ(13, slot_span->num_unprovisioned_slots);
+
+  allocator.root()->Free(ptr);
+  allocator.root()->Free(ptr2);
+  allocator.root()->Free(ptr3);
+  EXPECT_NE(-1, slot_span->empty_cache_index);
+  EXPECT_TRUE(slot_span2->freelist_head);
+  EXPECT_EQ(0, slot_span2->num_allocated_slots);
+
+  // And test a couple of sizes that do not cross SystemPageSize() with a
+  // single allocation.
   size_t medium_size = (SystemPageSize() / 2) - kExtraAllocSize;
   bucket_index = SizeToIndex(medium_size + kExtraAllocSize);
   bucket = &allocator.root()->buckets[bucket_index];
@@ -1196,12 +1234,23 @@ TEST_F(PartitionAllocTest, PartialPageFreelists) {
   slot_span = SlotSpan::FromPointer(
       allocator.root()->AdjustPointerForExtrasSubtract(ptr));
   EXPECT_EQ(1, slot_span->num_allocated_slots);
-  EXPECT_TRUE(slot_span->freelist_head);
+  // Only the first slot was provisioned, and that's the one that was just
+  // allocated so the free list is empty.
+  EXPECT_TRUE(!slot_span->freelist_head);
   total_slots =
       (slot_span->bucket->num_system_pages_per_slot_span * SystemPageSize()) /
       (page_and_a_half_size + kExtraAllocSize);
+  EXPECT_EQ(total_slots - 1, slot_span->num_unprovisioned_slots);
+  ptr2 = allocator.root()->Alloc(page_and_a_half_size, type_name);
+  EXPECT_TRUE(ptr);
+  slot_span = SlotSpan::FromPointer(
+      allocator.root()->AdjustPointerForExtrasSubtract(ptr));
+  EXPECT_EQ(2, slot_span->num_allocated_slots);
+  // As above, only one slot was provisioned.
+  EXPECT_TRUE(!slot_span->freelist_head);
   EXPECT_EQ(total_slots - 2, slot_span->num_unprovisioned_slots);
   allocator.root()->Free(ptr);
+  allocator.root()->Free(ptr2);
 
   // And then make sure than exactly the page size only faults one page.
   size_t page_size = SystemPageSize() - kExtraAllocSize;
@@ -1383,7 +1432,7 @@ TEST_F(PartitionAllocTest, FreeCache) {
       allocator.root()->AdjustPointerForExtrasSubtract(ptr));
   EXPECT_EQ(nullptr, bucket->empty_slot_spans_head);
   EXPECT_EQ(1, slot_span->num_allocated_slots);
-  size_t expected_committed_size = PartitionPageSize();
+  size_t expected_committed_size = SystemPageSize();
   EXPECT_EQ(expected_committed_size,
             allocator.root()->get_total_size_of_committed_pages());
   allocator.root()->Free(ptr);
@@ -1397,11 +1446,7 @@ TEST_F(PartitionAllocTest, FreeCache) {
   EXPECT_FALSE(slot_span->freelist_head);
   EXPECT_EQ(-1, slot_span->empty_cache_index);
   EXPECT_EQ(0, slot_span->num_allocated_slots);
-  PartitionBucket<base::internal::ThreadSafe>* cycle_free_cache_bucket =
-      &allocator.root()->buckets[test_bucket_index_];
-  size_t expected_size =
-      cycle_free_cache_bucket->num_system_pages_per_slot_span *
-      SystemPageSize();
+  size_t expected_size = SystemPageSize();
   EXPECT_EQ(expected_size,
             allocator.root()->get_total_size_of_committed_pages());
 
@@ -2520,7 +2565,7 @@ TEST_F(PartitionAllocTest, MAYBE_Bookkeeping) {
 
   // A full slot span of size 1 partition page is committed.
   void* ptr = root.Alloc(small_size - kExtraAllocSize, type_name);
-  size_t expected_committed_size = PartitionPageSize();
+  size_t expected_committed_size = SystemPageSize();
   size_t expected_super_pages_size = kSuperPageSize;
   EXPECT_EQ(expected_committed_size, root.total_size_of_committed_pages);
   EXPECT_EQ(expected_super_pages_size, root.total_size_of_super_pages);
@@ -2542,7 +2587,7 @@ TEST_F(PartitionAllocTest, MAYBE_Bookkeeping) {
 
   // Allocating another size commits another slot span.
   ptr = root.Alloc(2 * small_size - kExtraAllocSize, type_name);
-  expected_committed_size += PartitionPageSize();
+  expected_committed_size += SystemPageSize();
   EXPECT_EQ(expected_committed_size, root.total_size_of_committed_pages);
   EXPECT_EQ(expected_super_pages_size, root.total_size_of_super_pages);
 

base/allocator/partition_allocator/partition_bucket.cc

@@ -216,16 +216,10 @@ ALWAYS_INLINE void* PartitionBucket<thread_safe>::AllocNewSlotSpan(
   }
 
   char* ret = root->next_partition_page;
-
-  // System pages in the super page come in a decommited state. Commit them
-  // before vending them back.
-  root->RecommitSystemPagesForData(ret, slot_span_committed_size,
-                                   PageUpdatePermissions);
   root->next_partition_page += slot_span_reserved_size;
   // Double check that we had enough space in the super page for the new slot
   // span.
   PA_DCHECK(root->next_partition_page <= root->next_partition_page_end);
-
   return ret;
 }
 
@@ -355,7 +349,8 @@ ALWAYS_INLINE void PartitionBucket<thread_safe>::InitializeSlotSpan(
 }
 
 template <bool thread_safe>
-ALWAYS_INLINE char* PartitionBucket<thread_safe>::AllocAndFillFreelist(
+ALWAYS_INLINE char* PartitionBucket<thread_safe>::ProvisionMoreSlotsAndAllocOne(
+    PartitionRoot<thread_safe>* root,
     SlotSpanMetadata<thread_safe>* slot_span) {
   PA_DCHECK(slot_span !=
             SlotSpanMetadata<thread_safe>::get_sentinel_slot_span());
@@ -372,56 +367,55 @@ ALWAYS_INLINE char* PartitionBucket<thread_safe>::AllocAndFillFreelist(
   size_t size = slot_size;
   char* base = reinterpret_cast<char*>(
       SlotSpanMetadata<thread_safe>::ToPointer(slot_span));
-  char* return_object = base + (size * slot_span->num_allocated_slots);
-  char* first_freelist_pointer = return_object + size;
-  char* first_freelist_pointer_extent =
-      first_freelist_pointer + sizeof(PartitionFreelistEntry*);
-  // Our goal is to fault as few system pages as possible. We calculate the
-  // page containing the "end" of the returned slot, and then allow freelist
-  // pointers to be written up to the end of that page.
-  char* sub_page_limit = reinterpret_cast<char*>(
-      RoundUpToSystemPage(reinterpret_cast<size_t>(first_freelist_pointer)));
-  char* slots_limit = return_object + (size * num_slots);
-  char* freelist_limit = sub_page_limit;
-  if (UNLIKELY(slots_limit < freelist_limit))
-    freelist_limit = slots_limit;
-
-  uint16_t num_new_freelist_entries = 0;
-  if (LIKELY(first_freelist_pointer_extent <= freelist_limit)) {
-    // Only consider used space in the slot span. If we consider wasted
-    // space, we may get an off-by-one when a freelist pointer fits in the
-    // wasted space, but a slot does not.
-    // We know we can fit at least one freelist pointer.
-    num_new_freelist_entries = 1;
-    // Any further entries require space for the whole slot span.
-    num_new_freelist_entries += static_cast<uint16_t>(
-        (freelist_limit - first_freelist_pointer_extent) / size);
-  }
+  // If we got here, the first unallocated slot is either partially or fully on
+  // an uncommitted page. If the latter, it must be at the start of that page.
+  char* return_slot = base + (size * slot_span->num_allocated_slots);
+  char* next_slot = return_slot + size;
+  char* commit_start = bits::Align(return_slot, SystemPageSize());
+  PA_DCHECK(next_slot > commit_start);
+  char* commit_end = bits::Align(next_slot, SystemPageSize());
+  // If the slot was partially committed, |return_slot| and |next_slot| fall
+  // in different pages. If the slot was fully uncommitted, |return_slot| points
+  // to the page start and |next_slot| doesn't, thus only the latter gets
+  // rounded up.
+  PA_DCHECK(commit_end > commit_start);
+  // System pages in the slot span come in an initially decommitted state.
+  // Can't use PageKeepPermissionsIfPossible, because we have no knowledge
+  // which pages have been committed before.
+  root->RecommitSystemPagesForData(commit_start, commit_end - commit_start,
+                                   PageUpdatePermissions);
 
-  // We always return an object slot -- that's the +1 below.
-  // We do not neccessarily create any new freelist entries, because we cross
-  // sub page boundaries frequently for large bucket sizes.
-  PA_DCHECK(num_new_freelist_entries + 1 <= num_slots);
-  num_slots -= (num_new_freelist_entries + 1);
-  slot_span->num_unprovisioned_slots = num_slots;
+  // The slot being returned is considered allocated, and no longer
+  // unprovisioned.
   slot_span->num_allocated_slots++;
-
-  if (LIKELY(num_new_freelist_entries)) {
-    char* freelist_pointer = first_freelist_pointer;
-    auto* entry = reinterpret_cast<PartitionFreelistEntry*>(freelist_pointer);
-    slot_span->SetFreelistHead(entry);
-    while (--num_new_freelist_entries) {
-      freelist_pointer += size;
-      auto* next_entry =
-          reinterpret_cast<PartitionFreelistEntry*>(freelist_pointer);
-      entry->SetNext(next_entry);
-      entry = next_entry;
+  slot_span->num_unprovisioned_slots--;
+
+  // Add all slots that fit within so far committed pages to the free list.
+  PartitionFreelistEntry* prev_entry = nullptr;
+  PartitionFreelistEntry* entry =
+      reinterpret_cast<PartitionFreelistEntry*>(next_slot);
+  char* next_slot_end = next_slot + size;
+  while (next_slot_end <= commit_end) {
+    if (!slot_span->freelist_head) {
+      PA_DCHECK(!prev_entry);
+      slot_span->SetFreelistHead(entry);
+    } else {
+      prev_entry->SetNext(entry);
     }
-    entry->SetNext(nullptr);
-  } else {
-    slot_span->SetFreelistHead(nullptr);
+    next_slot = next_slot_end;
+    next_slot_end = next_slot + size;
+    prev_entry = entry;
+    entry = reinterpret_cast<PartitionFreelistEntry*>(next_slot);
+    slot_span->num_unprovisioned_slots--;
   }
-  return return_object;
+  // Null-terminate the list, if any slot made it to the list.
+  // One might think that this isn't needed as the page was just committed thus
+  // zeroed, but it isn't always the case on OS_APPLE.
+  if (prev_entry) {
+    prev_entry->SetNext(nullptr);
+  }
+
+  return return_slot;
 }
 
 template <bool thread_safe>
@@ -566,10 +560,6 @@ void* PartitionBucket<thread_safe>::SlowPathAlloc(
       PA_DCHECK(new_slot_span->bucket == this);
       PA_DCHECK(new_slot_span->is_decommitted());
       decommitted_slot_spans_head = new_slot_span->next_slot_span;
-      void* addr = SlotSpanMetadata<thread_safe>::ToPointer(new_slot_span);
-      root->RecommitSystemPagesForData(
-          addr, new_slot_span->bucket->get_bytes_per_span(),
-          PageKeepPermissionsIfPossible);
       new_slot_span->Reset();
       *is_already_zeroed = kDecommittedPagesAreAlwaysZeroed;
     }
@@ -618,9 +608,11 @@ void* PartitionBucket<thread_safe>::SlowPathAlloc(
     new_slot_span->num_allocated_slots++;
     return entry;
   }
-  // Otherwise, we need to build the freelist.
+
+  // Otherwise, we need to provision more slots by committing more pages. Build
+  // the free list for the newly provisioned slots.
   PA_DCHECK(new_slot_span->num_unprovisioned_slots);
-  return AllocAndFillFreelist(new_slot_span);
+  return ProvisionMoreSlotsAndAllocOne(root, new_slot_span);
 }
 
 template struct PartitionBucket<ThreadSafe>;

base/allocator/partition_allocator/partition_bucket.h

@@ -168,11 +168,18 @@ struct PartitionBucket {
   ALWAYS_INLINE void InitializeSlotSpan(
       SlotSpanMetadata<thread_safe>* slot_span);
 
-  // Allocates one slot from the given |slot_span| and then adds the remainder
-  // to the current bucket. If the |slot_span| was freshly allocated, it must
-  // have been passed through InitializeSlotSpan() first.
-  ALWAYS_INLINE char* AllocAndFillFreelist(
-      SlotSpanMetadata<thread_safe>* slot_span);
+  // Commit 1 or more pages in |slot_span|, enough to get the next slot, which
+  // is returned by this function. If more slots fit into the committed pages,
+  // they'll be added to the free list of the slot span (note that next pointers
+  // are stored inside the slots).
+  // The free list must be empty when calling this function.
+  //
+  // If |slot_span| was freshly allocated, it must have been passed through
+  // InitializeSlotSpan() first.
+  ALWAYS_INLINE char* ProvisionMoreSlotsAndAllocOne(
+      PartitionRoot<thread_safe>* root,
+      SlotSpanMetadata<thread_safe>* slot_span)
+      EXCLUSIVE_LOCKS_REQUIRED(root->lock_);
 };
 
 }  // namespace internal

base/allocator/partition_allocator/partition_page.cc

@@ -6,12 +6,14 @@
 
 #include "base/allocator/partition_allocator/address_pool_manager.h"
 #include "base/allocator/partition_allocator/page_allocator.h"
+#include "base/allocator/partition_allocator/page_allocator_constants.h"
 #include "base/allocator/partition_allocator/partition_address_space.h"
 #include "base/allocator/partition_allocator/partition_alloc.h"
 #include "base/allocator/partition_allocator/partition_alloc_check.h"
 #include "base/allocator/partition_allocator/partition_alloc_features.h"
 #include "base/allocator/partition_allocator/partition_alloc_forward.h"
 #include "base/allocator/partition_allocator/partition_direct_map_extent.h"
+#include "base/bits.h"
 #include "base/check.h"
 #include "base/feature_list.h"
 #include "base/notreached.h"
@@ -167,7 +169,10 @@ void SlotSpanMetadata<thread_safe>::Decommit(PartitionRoot<thread_safe>* root) {
   PA_DCHECK(is_empty());
   PA_DCHECK(!bucket->is_direct_mapped());
   void* addr = SlotSpanMetadata::ToPointer(this);
-  root->DecommitSystemPagesForData(addr, bucket->get_bytes_per_span(),
+  size_t size_to_decommit = bits::Align(GetProvisionedSize(), SystemPageSize());
+  // Not decommitted slot span must've had at least 1 allocation.
+  PA_DCHECK(size_to_decommit > 0);
+  root->DecommitSystemPagesForData(addr, size_to_decommit,
                                    PageKeepPermissionsIfPossible);
 
   // We actually leave the decommitted slot span in the active list. We'll sweep

base/allocator/partition_allocator/partition_page.h

@@ -132,6 +132,15 @@ struct __attribute__((packed)) SlotSpanMetadata {
     return GetRawSize();
   }
 
+  // Returns the total size of the slots that are currently provisioned.
+  ALWAYS_INLINE size_t GetProvisionedSize() const {
+    size_t num_provisioned_slots =
+        bucket->get_slots_per_span() - num_unprovisioned_slots;
+    size_t provisioned_size = num_provisioned_slots * bucket->slot_size;
+    PA_DCHECK(provisioned_size <= bucket->get_bytes_per_span());
+    return provisioned_size;
+  }
+
   ALWAYS_INLINE void Reset();
 
   // TODO(ajwong): Can this be made private?  https://crbug.com/787153

base/allocator/partition_allocator/pcscan.cc

@@ -17,6 +17,7 @@
 #include "base/allocator/partition_allocator/page_allocator_constants.h"
 #include "base/allocator/partition_allocator/partition_address_space.h"
 #include "base/allocator/partition_allocator/partition_alloc.h"
+#include "base/allocator/partition_allocator/partition_alloc_check.h"
 #include "base/allocator/partition_allocator/partition_alloc_constants.h"
 #include "base/allocator/partition_allocator/partition_alloc_features.h"
 #include "base/allocator/partition_allocator/partition_page.h"
@@ -404,9 +405,11 @@ PCScan<thread_safe>::PCScanTask::PCScanTask(PCScan& pcscan) : pcscan_(pcscan) {
             super_page, true /*with pcscan*/, [this](SlotSpan* slot_span) {
               auto* payload_begin =
                   static_cast<uintptr_t*>(SlotSpan::ToPointer(slot_span));
+              size_t provisioned_size = slot_span->GetProvisionedSize();
+              // Free & decommitted slot spans are skipped.
+              PA_DCHECK(provisioned_size > 0);
               auto* payload_end =
-                  payload_begin +
-                  (slot_span->bucket->get_bytes_per_span() / sizeof(uintptr_t));
+                  payload_begin + (provisioned_size / sizeof(uintptr_t));
               if (slot_span->bucket->slot_size >= kLargeScanAreaThreshold) {
                 large_scan_areas_.push_back(
                     {payload_begin, payload_end, slot_span->bucket->slot_size});