Reland "[PartitionAlloc] Don't sort already-sorted freelists."

This reverts commit 47a4e7d.

Reason for reland: Fixed the "alloc doesn't destroy ordering" property.

Original change's description:
> [PartitionAlloc] Don't sort already-sorted freelists.
>
> If a slot span hasn't been touched since the last sort, there is no need
> to sort it again. This is intended to reduce the CPU cost of freelist
> sorting.
>
> This is motivated by stack-sampled metrics showing that the cost of
> freelist sorting is non-trivial in the browser process. It also allows
> to track whether a slot span is "idle", which may be used to decommit
> part of the freelist if possible.
>
> Bug: 998048
> Change-Id: I06cb59e1dffdec23fc1869ee8bd8e6dba70c6bd6
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3259733
> Reviewed-by: Bartek Nowierski <bartekn@chromium.org>
> Commit-Queue: Benoit L <lizeb@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#938209}

Bug: 998048
Change-Id: I5f40cbaf3e1c496c301dfbb8fe54c53f540da6a4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3259542
Commit-Queue: Benoit L <lizeb@chromium.org>
Reviewed-by: Bartek Nowierski <bartekn@chromium.org>
Cr-Commit-Position: refs/heads/main@{#939306}

base/allocator/partition_allocator/partition_alloc_unittest.cc

@@ -283,7 +283,7 @@ class PartitionAllocTest : public testing::Test {
     EXPECT_EQ(num_slots,
               static_cast<size_t>(
                   bucket->active_slot_spans_head->num_allocated_slots));
-    EXPECT_EQ(nullptr, bucket->active_slot_spans_head->freelist_head);
+    EXPECT_EQ(nullptr, bucket->active_slot_spans_head->get_freelist_head());
     EXPECT_TRUE(bucket->is_valid());
     EXPECT_TRUE(bucket->active_slot_spans_head !=
                 SlotSpan::get_sentinel_slot_span());
@@ -1102,7 +1102,7 @@ TEST_F(PartitionAllocTest, Realloc) {
   void* ptr2 = allocator.root()->Realloc(ptr, 0, type_name);
   EXPECT_EQ(nullptr, ptr2);
   EXPECT_PEQ(allocator.root()->AdjustPointerForExtrasSubtract(ptr),
-             slot_span->freelist_head);
+             slot_span->get_freelist_head());
 
   // Test that growing an allocation with realloc() copies everything from the
   // old allocation.
@@ -1304,25 +1304,25 @@ TEST_F(PartitionAllocTest, PartialPageFreelists) {
   // The freelist should have one entry, because we were able to exactly fit
   // one object slot and one freelist pointer (the null that the head points
   // to) into a system page.
-  EXPECT_FALSE(slot_span->freelist_head);
+  EXPECT_FALSE(slot_span->get_freelist_head());
   EXPECT_EQ(1, slot_span->num_allocated_slots);
   EXPECT_EQ(3, slot_span->num_unprovisioned_slots);
 
   void* ptr2 = allocator.root()->Alloc(big_size, type_name);
   EXPECT_TRUE(ptr2);
-  EXPECT_FALSE(slot_span->freelist_head);
+  EXPECT_FALSE(slot_span->get_freelist_head());
   EXPECT_EQ(2, slot_span->num_allocated_slots);
   EXPECT_EQ(2, slot_span->num_unprovisioned_slots);
 
   void* ptr3 = allocator.root()->Alloc(big_size, type_name);
   EXPECT_TRUE(ptr3);
-  EXPECT_FALSE(slot_span->freelist_head);
+  EXPECT_FALSE(slot_span->get_freelist_head());
   EXPECT_EQ(3, slot_span->num_allocated_slots);
   EXPECT_EQ(1, slot_span->num_unprovisioned_slots);
 
   void* ptr4 = allocator.root()->Alloc(big_size, type_name);
   EXPECT_TRUE(ptr4);
-  EXPECT_FALSE(slot_span->freelist_head);
+  EXPECT_FALSE(slot_span->get_freelist_head());
   EXPECT_EQ(4, slot_span->num_allocated_slots);
   EXPECT_EQ(0, slot_span->num_unprovisioned_slots);
 
@@ -1346,7 +1346,7 @@ TEST_F(PartitionAllocTest, PartialPageFreelists) {
   allocator.root()->Free(ptr6);
   EXPECT_NE(-1, slot_span->empty_cache_index);
   EXPECT_NE(-1, slot_span2->empty_cache_index);
-  EXPECT_TRUE(slot_span2->freelist_head);
+  EXPECT_TRUE(slot_span2->get_freelist_head());
   EXPECT_EQ(0, slot_span2->num_allocated_slots);
 
   // Size that's just above half a page.
@@ -1365,13 +1365,13 @@ TEST_F(PartitionAllocTest, PartialPageFreelists) {
       bucket->slot_size;
   const size_t expected_slots = kNumBucketsPerOrderBits == 3 ? 16u : 24u;
   EXPECT_EQ(expected_slots, total_slots);
-  EXPECT_FALSE(slot_span->freelist_head);
+  EXPECT_FALSE(slot_span->get_freelist_head());
   EXPECT_EQ(1, slot_span->num_allocated_slots);
   EXPECT_EQ(expected_slots - 1, slot_span->num_unprovisioned_slots);
 
   ptr2 = allocator.root()->Alloc(non_dividing_size, type_name);
   EXPECT_TRUE(ptr2);
-  EXPECT_TRUE(slot_span->freelist_head);
+  EXPECT_TRUE(slot_span->get_freelist_head());
   EXPECT_EQ(2, slot_span->num_allocated_slots);
   // 2 slots got provisioned: the first one fills the rest of the first (already
   // provision page) and exceeds it by just a tad, thus leading to provisioning
@@ -1380,15 +1380,15 @@ TEST_F(PartitionAllocTest, PartialPageFreelists) {
 
   ptr3 = allocator.root()->Alloc(non_dividing_size, type_name);
   EXPECT_TRUE(ptr3);
-  EXPECT_FALSE(slot_span->freelist_head);
+  EXPECT_FALSE(slot_span->get_freelist_head());
   EXPECT_EQ(3, slot_span->num_allocated_slots);
   EXPECT_EQ(expected_slots - 3, 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_TRUE(slot_span2->get_freelist_head());
   EXPECT_EQ(0, slot_span2->num_allocated_slots);
 
   // And test a couple of sizes that do not cross SystemPageSize() with a
@@ -1432,7 +1432,7 @@ TEST_F(PartitionAllocTest, PartialPageFreelists) {
             slot_span->num_unprovisioned_slots);
 
   allocator.root()->Free(ptr);
-  EXPECT_TRUE(slot_span->freelist_head);
+  EXPECT_TRUE(slot_span->get_freelist_head());
   EXPECT_EQ(0, slot_span->num_allocated_slots);
 
   static_assert(kExtraAllocSize < 64, "");
@@ -1459,7 +1459,7 @@ TEST_F(PartitionAllocTest, PartialPageFreelists) {
             slot_span->num_unprovisioned_slots);
 
   allocator.root()->Free(ptr);
-  EXPECT_TRUE(slot_span->freelist_head);
+  EXPECT_TRUE(slot_span->get_freelist_head());
   EXPECT_EQ(0, slot_span->num_allocated_slots);
 
   // And try an allocation size (against the generic allocator) that is
@@ -1473,7 +1473,7 @@ TEST_F(PartitionAllocTest, PartialPageFreelists) {
   EXPECT_EQ(1, slot_span->num_allocated_slots);
   // 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);
+  EXPECT_TRUE(!slot_span->get_freelist_head());
   total_slots =
       (slot_span->bucket->num_system_pages_per_slot_span * SystemPageSize()) /
       (page_and_a_half_size + kExtraAllocSize);
@@ -1484,7 +1484,7 @@ TEST_F(PartitionAllocTest, PartialPageFreelists) {
       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_TRUE(!slot_span->get_freelist_head());
   EXPECT_EQ(total_slots - 2, slot_span->num_unprovisioned_slots);
   allocator.root()->Free(ptr);
   allocator.root()->Free(ptr2);
@@ -1496,7 +1496,7 @@ TEST_F(PartitionAllocTest, PartialPageFreelists) {
   slot_span = SlotSpan::FromSlotStartPtr(
       allocator.root()->AdjustPointerForExtrasSubtract(ptr));
   EXPECT_EQ(1, slot_span->num_allocated_slots);
-  EXPECT_TRUE(slot_span->freelist_head);
+  EXPECT_TRUE(slot_span->get_freelist_head());
   total_slots =
       (slot_span->bucket->num_system_pages_per_slot_span * SystemPageSize()) /
       (page_size + kExtraAllocSize);
@@ -1679,12 +1679,12 @@ TEST_F(PartitionAllocTest, FreeCache) {
   allocator.root()->Free(ptr);
   EXPECT_EQ(0, slot_span->num_allocated_slots);
   EXPECT_NE(-1, slot_span->empty_cache_index);
-  EXPECT_TRUE(slot_span->freelist_head);
+  EXPECT_TRUE(slot_span->get_freelist_head());
 
   CycleFreeCache(kTestAllocSize);
 
   // Flushing the cache should have really freed the unused slot spans.
-  EXPECT_FALSE(slot_span->freelist_head);
+  EXPECT_FALSE(slot_span->get_freelist_head());
   EXPECT_EQ(-1, slot_span->empty_cache_index);
   EXPECT_EQ(0, slot_span->num_allocated_slots);
   size_t num_system_pages_per_slot_span = allocator.root()
@@ -1707,9 +1707,9 @@ TEST_F(PartitionAllocTest, FreeCache) {
   // used does not get freed.
   for (size_t i = 0; i < kMaxFreeableSpans * 2; ++i) {
     ptr = allocator.root()->Alloc(big_size, type_name);
-    EXPECT_TRUE(slot_span->freelist_head);
+    EXPECT_TRUE(slot_span->get_freelist_head());
     allocator.root()->Free(ptr);
-    EXPECT_TRUE(slot_span->freelist_head);
+    EXPECT_TRUE(slot_span->get_freelist_head());
   }
   EXPECT_EQ(expected_committed_size,
             allocator.root()->get_total_size_of_committed_pages());
@@ -1743,13 +1743,13 @@ TEST_F(PartitionAllocTest, LostFreeSlotSpansBug) {
   EXPECT_TRUE(bucket->empty_slot_spans_head->next_slot_span);
   EXPECT_EQ(0, slot_span->num_allocated_slots);
   EXPECT_EQ(0, slot_span2->num_allocated_slots);
-  EXPECT_TRUE(slot_span->freelist_head);
-  EXPECT_TRUE(slot_span2->freelist_head);
+  EXPECT_TRUE(slot_span->get_freelist_head());
+  EXPECT_TRUE(slot_span2->get_freelist_head());
 
   CycleFreeCache(kTestAllocSize);
 
-  EXPECT_FALSE(slot_span->freelist_head);
-  EXPECT_FALSE(slot_span2->freelist_head);
+  EXPECT_FALSE(slot_span->get_freelist_head());
+  EXPECT_FALSE(slot_span2->get_freelist_head());
 
   EXPECT_TRUE(bucket->empty_slot_spans_head);
   EXPECT_TRUE(bucket->empty_slot_spans_head->next_slot_span);
@@ -2624,7 +2624,7 @@ TEST_F(PartitionAllocTest, PurgeDiscardableWithFreeListRewrite) {
   void* ptr2b =
       allocator.root()->Alloc(SystemPageSize() - kExtraAllocSize, type_name);
   EXPECT_PEQ(ptr2, ptr2b);
-  EXPECT_FALSE(slot_span->freelist_head);
+  EXPECT_FALSE(slot_span->get_freelist_head());
 
   allocator.root()->Free(ptr1);
   allocator.root()->Free(ptr2);
@@ -2679,7 +2679,7 @@ TEST_F(PartitionAllocTest, PurgeDiscardableDoubleTruncateFreeList) {
   CHECK_PAGE_IN_CORE(ptr1 - kPointerOffset + (SystemPageSize() * 2), false);
   CHECK_PAGE_IN_CORE(ptr1 - kPointerOffset + (SystemPageSize() * 3), false);
 
-  EXPECT_FALSE(slot_span->freelist_head);
+  EXPECT_FALSE(slot_span->get_freelist_head());
 
   allocator.root()->Free(ptr1);
   allocator.root()->Free(ptr2);
@@ -3800,8 +3800,20 @@ TEST_F(PartitionAllocTest, SortFreelist) {
   allocations.clear();
 
   allocator.root()->PurgeMemory(PartitionPurgeDiscardUnusedSystemPages);
-  for (size_t i = 0; i < count; ++i)
+
+  size_t bucket_index = SizeToIndex(allocation_size + kExtraAllocSize);
+  auto& bucket = allocator.root()->buckets[bucket_index];
+  EXPECT_TRUE(bucket.active_slot_spans_head->freelist_is_sorted);
+
+  // Can sort again.
+  allocator.root()->PurgeMemory(PartitionPurgeDiscardUnusedSystemPages);
+  EXPECT_TRUE(bucket.active_slot_spans_head->freelist_is_sorted);
+
+  for (size_t i = 0; i < count; ++i) {
     allocations.push_back(allocator.root()->Alloc(allocation_size, ""));
+    // Allocating keeps the freelist sorted.
+    EXPECT_TRUE(bucket.active_slot_spans_head->freelist_is_sorted);
+  }
 
   // Check that it is sorted.
   for (size_t i = 1; i < allocations.size(); i++) {
@@ -3810,8 +3822,15 @@ TEST_F(PartitionAllocTest, SortFreelist) {
         reinterpret_cast<uintptr_t>(memory::UnmaskPtr(allocations[i])));
   }
 
-  for (void* ptr : allocations)
+  for (void* ptr : allocations) {
     allocator.root()->Free(ptr);
+    // Free()-ing memory destroys order.  Not looking at the head of the active
+    // list, as it is not necessarily the one from which |ptr| came from.
+    auto* slot_span = SlotSpan::FromSlotStartPtr(
+        allocator.root()->AdjustPointerForExtrasSubtract(ptr));
+    EXPECT_FALSE(slot_span->freelist_is_sorted);
+  }
+
   allocator.root()->Free(first_ptr);
 }
 

base/allocator/partition_allocator/partition_bucket.cc

@@ -713,7 +713,7 @@ ALWAYS_INLINE char* PartitionBucket<thread_safe>::ProvisionMoreSlotsAndAllocOne(
   // should not get here.)
   PA_DCHECK(num_slots + slot_span->num_allocated_slots == get_slots_per_span());
   // Similarly, make explicitly sure that the freelist is empty.
-  PA_DCHECK(!slot_span->freelist_head);
+  PA_DCHECK(!slot_span->get_freelist_head());
   PA_DCHECK(slot_span->num_allocated_slots >= 0);
 
   size_t size = slot_size;
@@ -767,7 +767,7 @@ ALWAYS_INLINE char* PartitionBucket<thread_safe>::ProvisionMoreSlotsAndAllocOne(
     if (LIKELY(size <= kMaxMemoryTaggingSize)) {
       entry = memory::TagMemoryRangeRandomly(entry, size);
     }
-    if (!slot_span->freelist_head) {
+    if (!slot_span->get_freelist_head()) {
       PA_DCHECK(!prev_entry);
       PA_DCHECK(!free_list_entries_added);
       slot_span->SetFreelistHead(entry);
@@ -789,12 +789,15 @@ ALWAYS_INLINE char* PartitionBucket<thread_safe>::ProvisionMoreSlotsAndAllocOne(
   PA_DCHECK(slots_to_provision == free_list_entries_added + 1);
   // We didn't necessarily provision more than one slot (e.g. if |slot_size|
   // is large), meaning that |slot_span->freelist_head| can be nullptr.
-  if (slot_span->freelist_head) {
+  if (slot_span->get_freelist_head()) {
     PA_DCHECK(free_list_entries_added);
-    slot_span->freelist_head->CheckFreeList(slot_size);
+    slot_span->get_freelist_head()->CheckFreeList(slot_size);
   }
 #endif
 
+  // We had no free slots, and created some (potentially 0) in sorted order.
+  slot_span->freelist_is_sorted = true;
+
   return return_slot;
 }
 
@@ -851,7 +854,14 @@ template <bool thread_safe>
 void PartitionBucket<thread_safe>::SortSlotSpanFreelists() {
   for (auto* slot_span = active_slot_spans_head; slot_span;
        slot_span = slot_span->next_slot_span) {
-    if (slot_span->num_allocated_slots > 0)
+    // No need to sort the freelist if it's already sorted. Note that if the
+    // freelist is sorted, this means that it didn't change at all since the
+    // last call. This may be a good signal to shrink it if possible (if an
+    // entire OS page is free, we can decommit it).
+    //
+    // Besides saving CPU, this also avoid touching memory of fully idle slot
+    // spans, which may required paging.
+    if (slot_span->num_allocated_slots > 0 && !slot_span->freelist_is_sorted)
       slot_span->SortFreelist();
   }
 }
@@ -870,7 +880,7 @@ void* PartitionBucket<thread_safe>::SlowPathAlloc(
   // when a higher-order alignment is requested, in which case the freelist
   // logic is bypassed and we go directly for slot span allocation.
   bool allocate_aligned_slot_span = slot_span_alignment > PartitionPageSize();
-  PA_DCHECK(!active_slot_spans_head->freelist_head ||
+  PA_DCHECK(!active_slot_spans_head->get_freelist_head() ||
             allocate_aligned_slot_span);
 
   SlotSpanMetadata<thread_safe>* new_slot_span = nullptr;
@@ -921,7 +931,7 @@ void* PartitionBucket<thread_safe>::SlowPathAlloc(
       PA_DCHECK(new_slot_span->is_empty() || new_slot_span->is_decommitted());
       empty_slot_spans_head = new_slot_span->next_slot_span;
       // Accept the empty slot span unless it got decommitted.
-      if (new_slot_span->freelist_head) {
+      if (new_slot_span->get_freelist_head()) {
         new_slot_span->next_slot_span = nullptr;
         new_slot_span->ToSuperPageExtent()
             ->IncrementNumberOfNonemptySlotSpans();
@@ -1003,11 +1013,9 @@ void* PartitionBucket<thread_safe>::SlowPathAlloc(
 
   // If we found an active slot span with free slots, or an empty slot span, we
   // have a usable freelist head.
-  if (LIKELY(new_slot_span->freelist_head != nullptr)) {
-    PartitionFreelistEntry* entry = new_slot_span->freelist_head;
-    PartitionFreelistEntry* new_head = entry->GetNext(slot_size);
-    new_slot_span->SetFreelistHead(new_head);
-    new_slot_span->num_allocated_slots++;
+  if (LIKELY(new_slot_span->get_freelist_head() != nullptr)) {
+    PartitionFreelistEntry* entry =
+        new_slot_span->PopForAlloc(new_bucket->slot_size);
 
     // We likely set *is_already_zeroed to true above, make sure that the
     // freelist entry doesn't contain data.

base/allocator/partition_allocator/partition_page.cc

@@ -286,6 +286,7 @@ void SlotSpanMetadata<thread_safe>::SortFreelist() {
     }
   }
   SetFreelistHead(head);
+  freelist_is_sorted = true;
 }
 
 namespace {