Lower std::string's alignment requirement from 16 to 8.

[EricWF]

This allows smaller allocations to occur, closer to the actual std::string's required size. This is particularly effective in decreasing the allocation size upon initial construction (where __recommend is called to determine the size).

Although the memory savings per-string are never more than 8 bytes per string initially, this quickly adds up. And has lead to not insigficant memory savings at Google.

Unfortunately, this change is ABI breaking because it changes the value returned by max_size. So it has to be guarded.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Eric (EricWF)

Changes

This allows smaller allocations to occur, closer to the actual std::string's required size. This is particularly effective in decreasing the allocation size upon initial construction (where __recommend is called to determine the size).

Although the memory savings per-string are never more than 8 bytes per string initially, this quickly adds up. And has lead to not insigficant memory savings at Google.

Unfortunately, this change is ABI breaking because it changes the value returned by max_size. So it has to be guarded.

---

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

4 Files Affected:

• (modified) libcxx/include/__config (+5)
• (modified) libcxx/include/string (+8-1)
• (added) libcxx/test/libcxx/strings/basic.string/string.capacity/allocation_size.pass.cpp (+43)
• (modified) libcxx/test/libcxx/strings/basic.string/string.capacity/max_size.pass.cpp (+7-1)

diff --git a/libcxx/include/__config b/libcxx/include/__config
index 55d9f1c737652e6..01b1aa74163e4b1 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -167,6 +167,11 @@
 // The implementation moved to the header, but we still export the symbols from
 // the dylib for backwards compatibility.
 #    define _LIBCPP_ABI_DO_NOT_EXPORT_TO_CHARS_BASE_10
+// Same memory by providing the allocator more freedom to allocate the most
+// efficient size class by dropping the alignment requirements for std::string's
+// pointer from 16 to 8. This changes the output of std::string::max_size,
+// which makes it ABI breaking
+#   define _LIBCPP_ABI_STRING_8_BYTE_ALIGNMENT
 #  elif _LIBCPP_ABI_VERSION == 1
 #    if !(defined(_LIBCPP_OBJECT_FORMAT_COFF) || defined(_LIBCPP_OBJECT_FORMAT_XCOFF))
 // Enable compiling copies of now inline methods into the dylib to support
diff --git a/libcxx/include/string b/libcxx/include/string
index 33e87406a1156a6..3078715e02b358a 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -1851,7 +1851,14 @@ private:
         _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20
         size_type __align_it(size_type __s) _NOEXCEPT
             {return (__s + (__a-1)) & ~(__a-1);}
-    enum {__alignment = 16};
+    enum {
+      __alignment =
+#ifdef _LIBCPP_ABI_STRING_8_BYTE_ALIGNMENT
+      8
+#else
+      16
+#endif
+    };
     static _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20
     size_type __recommend(size_type __s) _NOEXCEPT
     {
diff --git a/libcxx/test/libcxx/strings/basic.string/string.capacity/allocation_size.pass.cpp b/libcxx/test/libcxx/strings/basic.string/string.capacity/allocation_size.pass.cpp
new file mode 100644
index 000000000000000..6260cbd92e17b11
--- /dev/null
+++ b/libcxx/test/libcxx/strings/basic.string/string.capacity/allocation_size.pass.cpp
@@ -0,0 +1,43 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// <string>
+
+// This test demonstrates the smaller allocation sizes when the alignment
+// requirements of std::string are dropped from 16 to 8.
+#include <algorithm>
+#include <cassert>
+#include <cstddef>
+#include <string>
+
+#include "test_macros.h"
+
+// alignment of the string heap buffer is hardcoded to 16
+
+constexpr std::size_t alignment =
+#ifdef _LIBCPP_ABI_STRING_8_BYTE_ALIGNMENT
+    8;
+#else
+    16;
+#endif
+
+int main(int, char**) {
+  std::string input_string;
+  input_string.resize(64, 'a');
+
+  // Call a constructor which selects its size using __recommend.
+  std::string test_string(input_string.data());
+  constexpr std::size_t expected_align8_size = 71;
+
+  // Demonstrate the lesser capacity/allocation size when the alignment requirement is 8.
+  if (alignment == 8) {
+    assert(test_string.capacity() == expected_align8_size);
+  } else {
+    assert(test_string.capacity() == expected_align8_size + 8);
+  }
+}
diff --git a/libcxx/test/libcxx/strings/basic.string/string.capacity/max_size.pass.cpp b/libcxx/test/libcxx/strings/basic.string/string.capacity/max_size.pass.cpp
index 5af9cab0be4e80a..a3cb79522f2e1eb 100644
--- a/libcxx/test/libcxx/strings/basic.string/string.capacity/max_size.pass.cpp
+++ b/libcxx/test/libcxx/strings/basic.string/string.capacity/max_size.pass.cpp
@@ -18,7 +18,13 @@
 #include "test_macros.h"
 
 // alignment of the string heap buffer is hardcoded to 16
-static const std::size_t alignment = 16;
+
+static const std::size_t alignment =
+#ifdef _LIBCPP_ABI_STRING_8_BYTE_ALIGNMENT
+    8;
+#else
+    16;
+#endif
 
 template <class = int>
 TEST_CONSTEXPR_CXX20 void full_size() {

[EricWF]

This is a duplicate of #68749 which was closed by accident.

[mordante]

In general no objection.
We should add this to the release notes too.

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.

[EricWF]

OK, I have no idea how to rebase diffs apparently. Working on fixing this.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[EricWF]

In general no objection. We should add this to the release notes too.

Sorry, I wasn't ignoring this. But the release note should be there now.

[ldionne]

Thanks, this LGTM with a few nitpicks. The C++03 build is failing but I think this is easy to address (probably your usage of constexpr in the test).

[mordante]

In general no objection. We should add this to the release notes too.

Sorry, I wasn't ignoring this. But the release note should be there now.

No problem.

[mordante]

sorry wrong button, still getting used to GitHub reviews

[mordante]

LGTM modulo some nits.