[libc++] Refactor some code in monotonic_buffer_resource

[love1angel]

1. remove unused __default_buffer_alignment
2. two __try_allocate_from_chunk are same, put it together

This patch refactor some code in monotonic_buffer_resource.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Peng Xie (love1angel)

Changes

1. remove unused __default_buffer_alignment
2. two __try_allocate_from_chunk are same, put it together

This patch refactor some code in monotonic_buffer_resource.

---

Full diff: https://github.com/llvm/llvm-project/pull/117271.diff

2 Files Affected:

• (modified) libcxx/include/__memory_resource/monotonic_buffer_resource.h (+5-4)
• (modified) libcxx/src/memory_resource.cpp (+14-19)

diff --git a/libcxx/include/__memory_resource/monotonic_buffer_resource.h b/libcxx/include/__memory_resource/monotonic_buffer_resource.h
index c5a2b556707f6a..d972f4e4d24efd 100644
--- a/libcxx/include/__memory_resource/monotonic_buffer_resource.h
+++ b/libcxx/include/__memory_resource/monotonic_buffer_resource.h
@@ -27,8 +27,8 @@ namespace pmr {
 // [mem.res.monotonic.buffer]
 
 class _LIBCPP_AVAILABILITY_PMR _LIBCPP_EXPORTED_FROM_ABI monotonic_buffer_resource : public memory_resource {
-  static const size_t __default_buffer_capacity  = 1024;
-  static const size_t __default_buffer_alignment = 16;
+  static constexpr size_t __default_buffer_capacity = 1024;
+  static constexpr size_t __default_growth_factor   = 2;
 
   struct __chunk_footer {
     __chunk_footer* __next_;
@@ -38,7 +38,6 @@ class _LIBCPP_AVAILABILITY_PMR _LIBCPP_EXPORTED_FROM_ABI monotonic_buffer_resour
     _LIBCPP_HIDE_FROM_ABI size_t __allocation_size() {
       return (reinterpret_cast<char*>(this) - __start_) + sizeof(*this);
     }
-    void* __try_allocate_from_chunk(size_t, size_t);
   };
 
   struct __initial_descriptor {
@@ -48,9 +47,11 @@ class _LIBCPP_AVAILABILITY_PMR _LIBCPP_EXPORTED_FROM_ABI monotonic_buffer_resour
       char* __end_;
       size_t __size_;
     };
-    void* __try_allocate_from_chunk(size_t, size_t);
   };
 
+  template <typename Chunk>
+  _LIBCPP_HIDE_FROM_ABI void* __try_allocate_from_chunk(Chunk& self, size_t bytes, size_t align);
+
 public:
   _LIBCPP_HIDE_FROM_ABI monotonic_buffer_resource()
       : monotonic_buffer_resource(nullptr, __default_buffer_capacity, get_default_resource()) {}
diff --git a/libcxx/src/memory_resource.cpp b/libcxx/src/memory_resource.cpp
index 0cd575e995c0ff..5500c5c8ebbbd1 100644
--- a/libcxx/src/memory_resource.cpp
+++ b/libcxx/src/memory_resource.cpp
@@ -9,6 +9,7 @@
 #include <cstddef>
 #include <memory>
 #include <memory_resource>
+#include <type_traits>
 
 #if _LIBCPP_HAS_ATOMIC_HEADER
 #  include <atomic>
@@ -429,23 +430,17 @@ static void* align_down(size_t align, size_t size, void*& ptr, size_t& space) {
   return ptr;
 }
 
-void* monotonic_buffer_resource::__initial_descriptor::__try_allocate_from_chunk(size_t bytes, size_t align) {
-  if (!__cur_)
-    return nullptr;
-  void* new_ptr       = static_cast<void*>(__cur_);
-  size_t new_capacity = (__cur_ - __start_);
-  void* aligned_ptr   = align_down(align, bytes, new_ptr, new_capacity);
-  if (aligned_ptr != nullptr)
-    __cur_ = static_cast<char*>(new_ptr);
-  return aligned_ptr;
-}
-
-void* monotonic_buffer_resource::__chunk_footer::__try_allocate_from_chunk(size_t bytes, size_t align) {
-  void* new_ptr       = static_cast<void*>(__cur_);
-  size_t new_capacity = (__cur_ - __start_);
+template <typename Chunk>
+void* monotonic_buffer_resource::__try_allocate_from_chunk(Chunk& self, size_t bytes, size_t align) {
+  if constexpr (std::is_same_v<Chunk, monotonic_buffer_resource::__initial_descriptor>) {
+    if (self.__cur_)
+      return nullptr;
+  }
+  void* new_ptr       = static_cast<void*>(self.__cur_);
+  size_t new_capacity = (self.__cur_ - self.__start_);
   void* aligned_ptr   = align_down(align, bytes, new_ptr, new_capacity);
   if (aligned_ptr != nullptr)
-    __cur_ = static_cast<char*>(new_ptr);
+    self.__cur_ = static_cast<char*>(new_ptr);
   return aligned_ptr;
 }
 
@@ -462,10 +457,10 @@ void* monotonic_buffer_resource::do_allocate(size_t bytes, size_t align) {
     return roundup(newsize, footer_align) + footer_size;
   };
 
-  if (void* result = __initial_.__try_allocate_from_chunk(bytes, align))
+  if (void* result = this->__try_allocate_from_chunk(__initial_, bytes, align))
     return result;
   if (__chunks_ != nullptr) {
-    if (void* result = __chunks_->__try_allocate_from_chunk(bytes, align))
+    if (void* result = this->__try_allocate_from_chunk(*__chunks_, bytes, align))
       return result;
   }
 
@@ -478,7 +473,7 @@ void* monotonic_buffer_resource::do_allocate(size_t bytes, size_t align) {
   size_t previous_capacity = previous_allocation_size();
 
   if (aligned_capacity <= previous_capacity) {
-    size_t newsize   = 2 * (previous_capacity - footer_size);
+    size_t newsize   = __default_growth_factor * (previous_capacity - footer_size);
     aligned_capacity = roundup(newsize, footer_align) + footer_size;
   }
 
@@ -491,7 +486,7 @@ void* monotonic_buffer_resource::do_allocate(size_t bytes, size_t align) {
   footer->__align_       = align;
   __chunks_              = footer;
 
-  return __chunks_->__try_allocate_from_chunk(bytes, align);
+  return __try_allocate_from_chunk(*__chunks_, bytes, align);
 }
 
 } // namespace pmr

[frederick-vs-ja]

__default_growth_factor is not used in headers. I guess it would be better to move it to memory_resource.cpp, but I'd ask maintainers first.

[frederick-vs-ja]

Oops, both __try_allocate_from_chunk functions are ABI-critical, which is probably indicated by lack of _LIBCPP_HIDE_FROM_ABI. They must still exist, although I'm not sure whether it's possible to change their behavior (if so, they can just return nullptr).

[ldionne]

Those functions are on the ABI boundary, we can't remove them or change their behavior in a backwards incompatible way because pre-existing programs compiled against older versions of the headers will contain calls to this external function.

Unless nothing in the headers was ever calling into this function, in which case we might be able to remove it from the dylib. Is that the situation?

Edit: Doing a bit of searching, it seems like nobody's depending on that symbol, which would support that it was actually never needed as part of the ABI. I think it would be OK to remove it. You will need to update the libcxx/lib/abi/*.abilist files and libcxx/lib/abi/CHANGELOG.TXT.

[love1angel]

thanks, i will change

[ldionne]

Was __default_buffer_alignment unused?

[love1angel]

i don't found any used...

[ldionne]

Those functions are on the ABI boundary, we can't remove them or change their behavior in a backwards incompatible way because pre-existing programs compiled against older versions of the headers will contain calls to this external function.

Unless nothing in the headers was ever calling into this function, in which case we might be able to remove it from the dylib. Is that the situation?

Edit: Doing a bit of searching, it seems like nobody's depending on that symbol, which would support that it was actually never needed as part of the ABI. I think it would be OK to remove it. You will need to update the libcxx/lib/abi/*.abilist files and libcxx/lib/abi/CHANGELOG.TXT.

[ldionne]

Can this be moved to a non-member function defined only in memory_resource.cpp?

[love1angel]

Can this be moved to a non-member function defined only in memory_resource.cpp?

since struct __chunk_footer is private inside class monotonic_buffer_resource. then i couldn't get scope struct __chunk_footer __initial_descriptor.

but this can be easy archieved by add a branch judge cost. otherwise i can distinguish them by struct size, but this is ugly.