[libc++] Update the status for lwg-3143 #116971

love1angel · 2024-11-20T13:37:05Z

Current implementation use the larger one either requested bytes or growth factor multiply previous buffer size.

This patch update the lwg 3143 issue latest status.

github-actions · 2024-11-20T13:37:23Z

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 · 2024-11-20T13:37:55Z

@llvm/pr-subscribers-libcxx

Author: helianthus (love1angel)

Changes

Current implementation use the larger one either requested bytes or growth factor multiply previous buffer size.

This patch update the lwg 3143 issue latest status.

Close #104258

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

4 Files Affected:

(modified) libcxx/docs/Status/Cxx23Issues.csv (+1-1)
(modified) libcxx/include/__memory_resource/monotonic_buffer_resource.h (+2-2)
(modified) libcxx/src/memory_resource.cpp (+1-1)
(renamed) libcxx/test/std/utilities/utility/mem.res/mem.res.monotonic.buffer/mem.res.monotonic.buffer.mem/allocate_growth_capacity.cpp (+7-7)

diff --git a/libcxx/docs/Status/Cxx23Issues.csv b/libcxx/docs/Status/Cxx23Issues.csv
index 1215f21985eb94..f6aae79c00b884 100644
--- a/libcxx/docs/Status/Cxx23Issues.csv
+++ b/libcxx/docs/Status/Cxx23Issues.csv
@@ -1,7 +1,7 @@
 "Issue #","Issue Name","Meeting","Status","First released version","Notes"
 "`LWG2839 <https://wg21.link/LWG2839>`__","Self-move-assignment of library types, again","2020-11 (Virtual)","|Nothing To Do|","",""
 "`LWG3117 <https://wg21.link/LWG3117>`__","Missing ``packaged_task`` deduction guides","2020-11 (Virtual)","|Complete|","16",""
-"`LWG3143 <https://wg21.link/LWG3143>`__","``monotonic_buffer_resource`` growth policy is unclear","2020-11 (Virtual)","","",""
+"`LWG3143 <https://wg21.link/LWG3143>`__","``monotonic_buffer_resource`` growth policy is unclear","2020-11 (Virtual)","|Complete|","16",""
 "`LWG3195 <https://wg21.link/LWG3195>`__","What is the stored pointer value of an empty ``weak_ptr``?","2020-11 (Virtual)","|Nothing To Do|","",""
 "`LWG3211 <https://wg21.link/LWG3211>`__","``std::tuple<>`` should be trivially constructible","2020-11 (Virtual)","|Complete|","9",""
 "`LWG3236 <https://wg21.link/LWG3236>`__","Random access iterator requirements lack limiting relational operators domain to comparing those from the same range","2020-11 (Virtual)","|Nothing To Do|","",""
diff --git a/libcxx/include/__memory_resource/monotonic_buffer_resource.h b/libcxx/include/__memory_resource/monotonic_buffer_resource.h
index c5a2b556707f6a..08c0745c8d084e 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_;
diff --git a/libcxx/src/memory_resource.cpp b/libcxx/src/memory_resource.cpp
index 0cd575e995c0ff..a9207a7d814822 100644
--- a/libcxx/src/memory_resource.cpp
+++ b/libcxx/src/memory_resource.cpp
@@ -478,7 +478,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;
   }
 
diff --git a/libcxx/test/std/utilities/utility/mem.res/mem.res.monotonic.buffer/mem.res.monotonic.buffer.mem/allocate_in_geometric_progression.pass.cpp b/libcxx/test/std/utilities/utility/mem.res/mem.res.monotonic.buffer/mem.res.monotonic.buffer.mem/allocate_growth_capacity.cpp
similarity index 80%
rename from libcxx/test/std/utilities/utility/mem.res/mem.res.monotonic.buffer/mem.res.monotonic.buffer.mem/allocate_in_geometric_progression.pass.cpp
rename to libcxx/test/std/utilities/utility/mem.res/mem.res.monotonic.buffer/mem.res.monotonic.buffer.mem/allocate_growth_capacity.cpp
index 7892b182297ea0..024e8f676864e4 100644
--- a/libcxx/test/std/utilities/utility/mem.res/mem.res.monotonic.buffer/mem.res.monotonic.buffer.mem/allocate_in_geometric_progression.pass.cpp
+++ b/libcxx/test/std/utilities/utility/mem.res/mem.res.monotonic.buffer/mem.res.monotonic.buffer.mem/allocate_growth_capacity.cpp
@@ -20,10 +20,10 @@
 #include "count_new.h"
 #include "test_macros.h"
 
-void test_geometric_progression() {
+// LWG 3143: https://cplusplus.github.io/LWG/issue3143
+void test_growth_capacity() {
   // mem.res.monotonic.buffer 1.3
-  // Each additional buffer is larger than the previous one, following a
-  // geometric progression.
+  // Each additional buffer is larger than the previous one
 
   globalMemCounter.reset();
   std::pmr::monotonic_buffer_resource mono1(100, std::pmr::new_delete_resource());
@@ -35,15 +35,15 @@ void test_geometric_progression() {
   assert(ret != nullptr);
   assert(globalMemCounter.checkNewCalledEq(1));
   assert(globalMemCounter.last_new_size >= next_buffer_size);
-  next_buffer_size = globalMemCounter.last_new_size + 1;
+  next_buffer_size = globalMemCounter.last_new_size;
 
   int new_called = 1;
   while (new_called < 5) {
     ret = r1.allocate(10, 1);
     if (globalMemCounter.new_called > new_called) {
       assert(globalMemCounter.new_called == new_called + 1);
-      assert(globalMemCounter.last_new_size >= next_buffer_size);
-      next_buffer_size = globalMemCounter.last_new_size + 1;
+      next_buffer_size = next_buffer_size * 2 - 4 * sizeof(void*);
+      assert(globalMemCounter.last_new_size == next_buffer_size);
       new_called += 1;
     }
   }
@@ -51,7 +51,7 @@ void test_geometric_progression() {
 
 int main(int, char**) {
 #if TEST_SUPPORTS_LIBRARY_INTERNAL_ALLOCATIONS && !defined(DISABLE_NEW_COUNT)
-  test_geometric_progression();
+  test_growth_capacity();
 #endif
 
   return 0;

frederick-vs-ja

I think the (NFC) changes in monotonic_buffer_resource.h and memory_resource.cpp should be split to another PR.

Current implementation use the larger one either requested bytes or growth factor multiply previous buffer size. This patch update the lwg 3143 issue latest status.

love1angel · 2024-11-20T23:30:39Z

seems i couldn't produce the CI problem, i test successfully in my local environment.

frederick-vs-ja · 2024-11-20T23:39:48Z

seems i couldn't produce the CI problem, i test successfully in my local environment.

The failure was because of that MSan makes the test extremely slow. Let's wait for landing of #116933.

ldionne · 2024-11-22T08:08:12Z

...y/mem.res/mem.res.monotonic.buffer/mem.res.monotonic.buffer.mem/allocate_growth_capacity.cpp

  // mem.res.monotonic.buffer 1.3
-  // Each additional buffer is larger than the previous one, following a
-  // geometric progression.
+  // Each additional buffer is larger than the previous one


Would it make sense to still test that libc++ does geometric progression? Perhaps as a libc++ specific test?

thanks for review. Yes, and here i want to remove "geometric progression" related naming for not causing ambiguous as the lwg issue saied( when request allocation size larger than growth factor * previous allocation size). this test want to test when initial buffer size is specified the growth behaviour.

maybe i should remove the 3143 link here?

IMO this test is OK as it is now, but we're missing an additional test for the libc++ specific behavior with the geometric progression -- unless I misunderstood what this LWG issue is about.

I will add more test to test the behavior

Gentle ping on this comment.

sry, I was busy with my work these day. I will finish my job ASAP. thanks

also, Could i also implement this task P0843R14. could you assign it to me? My job is related to this.

also, Could i also implement this task P0843R14. could you assign it to me? My job is related to this.

There's already a PR #105981 for this, although it seems stuck.

ldionne · 2025-01-10T17:29:37Z

...y/mem.res/mem.res.monotonic.buffer/mem.res.monotonic.buffer.mem/allocate_growth_capacity.cpp

  // mem.res.monotonic.buffer 1.3
-  // Each additional buffer is larger than the previous one, following a
-  // geometric progression.
+  // Each additional buffer is larger than the previous one


Gentle ping on this comment.

ldionne · 2025-05-12T20:43:44Z

Gentle ping @love1angel , it looks like this is almost ready to go.

love1angel requested a review from a team as a code owner November 20, 2024 13:37

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 20, 2024

frederick-vs-ja requested changes Nov 20, 2024

View reviewed changes

[libc++] Update the status for lwg-3143

90e2125

Current implementation use the larger one either requested bytes or growth factor multiply previous buffer size. This patch update the lwg 3143 issue latest status.

love1angel force-pushed the peng/fix_lwg_3143 branch from 5091283 to 90e2125 Compare November 20, 2024 15:35

love1angel requested a review from frederick-vs-ja November 20, 2024 15:38

frederick-vs-ja approved these changes Nov 20, 2024

View reviewed changes

ldionne reviewed Nov 22, 2024

View reviewed changes

Merge branch 'main' into peng/fix_lwg_3143

fdf638b

ldionne requested changes Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[libc++] Update the status for lwg-3143 #116971

[libc++] Update the status for lwg-3143 #116971

Uh oh!

love1angel commented Nov 20, 2024

Uh oh!

github-actions bot commented Nov 20, 2024

Uh oh!

llvmbot commented Nov 20, 2024

Uh oh!

frederick-vs-ja left a comment

Uh oh!

love1angel commented Nov 20, 2024

Uh oh!

frederick-vs-ja commented Nov 20, 2024 •

edited

Loading

Uh oh!

ldionne Nov 22, 2024

Uh oh!

love1angel Nov 22, 2024

Uh oh!

ldionne Nov 22, 2024

Uh oh!

love1angel Nov 25, 2024

Uh oh!

ldionne Jan 10, 2025

Uh oh!

love1angel Jan 13, 2025

Uh oh!

love1angel Jan 13, 2025

Uh oh!

frederick-vs-ja Jan 16, 2025

Uh oh!

ldionne Jan 10, 2025

Uh oh!

ldionne commented May 12, 2025

Uh oh!

Uh oh!

[libc++] Update the status for lwg-3143 #116971

Are you sure you want to change the base?

[libc++] Update the status for lwg-3143 #116971

Uh oh!

Conversation

love1angel commented Nov 20, 2024

Uh oh!

github-actions bot commented Nov 20, 2024

Uh oh!

llvmbot commented Nov 20, 2024

Uh oh!

frederick-vs-ja left a comment

Choose a reason for hiding this comment

Uh oh!

love1angel commented Nov 20, 2024

Uh oh!

frederick-vs-ja commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldionne commented May 12, 2025

Uh oh!

Uh oh!

frederick-vs-ja commented Nov 20, 2024 •

edited

Loading