[libcxx-commits] [libcxx] [libc++][test] Refactor increasing_allocator (PR #115671)

[Peng Liu via libcxx-commits]

================
@@ -0,0 +1,61 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef TEST_SUPPORT_INCREASING_ALLOCATOR_H
+#define TEST_SUPPORT_INCREASING_ALLOCATOR_H
+
+#include <cstddef>
+#include <memory>
+
+#include "test_macros.h"
+
+// The increasing_allocator is a custom allocator that enforces an increasing minimum allocation size,
+// ensuring that it allocates an increasing amount of memory, possibly exceeding the requested amount.
+// This unique behavior is particularly useful for testing the shrink_to_fit functionality in std::vector,
+// vector<bool>, and std::basic_string, ensuring that shrink_to_fit does not increase the capacity of
+// the allocated memory.
+
+template <typename T>
+struct increasing_allocator {
+  using value_type         = T;
+  std::size_t min_elements = 1000;
+  increasing_allocator()   = default;
+
+  template <typename U>
+  TEST_CONSTEXPR_CXX20 increasing_allocator(const increasing_allocator<U>& other) TEST_NOEXCEPT
+      : min_elements(other.min_elements) {}
+
+#if TEST_STD_VER >= 23
+  TEST_CONSTEXPR_CXX23 std::allocation_result<T*> allocate_at_least(std::size_t n) {
+    if (n < min_elements)
+      n = min_elements;
+    min_elements += 1000;
+    return std::allocator<T>{}.allocate_at_least(n);
+  }
+#endif // TEST_STD_VER >= 23
+
+  TEST_CONSTEXPR_CXX20 T* allocate(std::size_t n) {
+#if TEST_STD_VER >= 23
+    return allocate_at_least(n).ptr;
+#else
----------------
winner245 wrote:

Thank you for your detailed feedback!

> Why don't we unconditionally call `std::allocator<T>().allocate(n)`?

I agree that it's better to unconditionally call `std::allocator<T>().allocate(n)` within `increasing_allocator::allocate(n)`. I also think that we should allow only the `allocate_at_least(n)` function of `increasing_allocator` to allocate more memory than the requested amount `n`, i.e., it actually allocates `max(n, min_elements)`. However, I think it makes more sense for `increasing_allocator::allocate(n)` to allocate effectively the exact amount `n`, otherwise we will see a mismatch between the reported and the actual allocation counts, as shown in this [example](https://godbolt.org/z/efGzqKb93). Therefore, `allocate_at_least(n)` should allocate at least `n`, while `allocate(n)` should allocate exactly `n`. 

> Also, we still run into the issue that we had before, which is that we're calling std::allocator<T>().allocate(n) but calling std::allocator<T>().deallocate(ptr, m) with a smaller value than we allocated. That violates [allocator.members](http://eel.is/c++draft/allocator.members#10.1).
> 
> We can either do what @frederick-vs-ja suggested [here](https://github.com/llvm/llvm-project/pull/115671/files#r1843551032) (which TBH I don't fully understand why that makes the allocator ever-increasing), or we could try something like this: [godbolt.org/z/P8PK6hvEj](https://godbolt.org/z/P8PK6hvEj)

I agree that the allocation/deallocation-size mismatch between `allocate(n)/allocate_at_least(n)` and `deallocate(p, m)` always exists because it completely depends on the usages of the functions. Users may provide mismatched `m/n` values, regardless of our implementation. I think @frederick-vs-ja's idea provides a partial solution or it alleviates this issue by quantizing the allocation/deallocation sizes into discrete power-of-2 values. This allows for mismatches between `n` and `m`, as long as they are quantized to the same power of 2. However, completely irrelevant `m, n` values yielding different equitizations may still cause mismatches. 

To fully resolve the allocation/deallocation-size mismatch, we might need to record the actual allocation size during each allocation, similar to how you store the allocation size for each allocated memory `p`. This way, regardless of what `m` was provided during `deallocate(p, m)`, we can always determine the actual allocation size from `p`. Your code provides a neat implementation of this approach.

> This is a lot more complicated but I think it works. If you agree with that direction, we could land this PR as-is and then tackle this additional change as a follow-up, since it can be done independently from this refactoring.

I agree it's getting a bit complicated but it seems worth proceeding. I agree with your direction. I'll keep the current refactoring in this PR as I really want the `increasing_allocator` utility in a single location, so that other work (including my own ongoing work) can use this class without duplicating the code. Follow-up work to further improve this class can be done independently after this PR. 

https://github.com/llvm/llvm-project/pull/115671