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

EricWF · 2023-10-10T22:14:40Z

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 · 2023-10-10T22:15:17Z

@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/68749.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..97714a11835902f
--- /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..70aa8f7bda65efe 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() {

github-actions · 2023-10-10T22:25:22Z

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

ldionne · 2023-10-10T23:03:48Z

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


Suggested change

// Same memory by providing the allocator more freedom to allocate the most

// Save memory by providing the allocator more freedom to allocate the most

ldionne · 2023-10-10T23:06:51Z

libcxx/include/__config

+// pointer from 16 to 8. This changes the output of std::string::max_size,
+// which makes it ABI breaking


I think the main source of the ABI break here is that a std::string's data() could suddenly be aligned at a 8-byte aligned address and passed to a function that was compiled assuming a 16-bytes alignment? Or am I mistaken?

If it's really just about the value of max_size(), honestly I think this is something we could probably do unconditionally. I think the likelihood of that actually breaking the ABI for anyone is quite small. Didn't we return an incorrect value from that function for the longest time and it never affected anyone?

It is unlikely to "really" break users, in a previous change we saw automated tests failing because 'dynamic libraries'. Theoretically it could cause ODR if you have (inlined / weak) code with different constexpr max_size() compiled in?

But yeah, I think "it should be fine", but I am always a bit.... uncomfortable with these of language lawyer questions :)

Ah, I just saw this is actually https://reviews.llvm.org/D155486 IIUC. This provides more background, I see now. Can we abandon that change to clear up our Phabricator queue, or is there stuff left in that patch that we still need to migrate over to Github (or land).

Coming back to the change itself, FWIW I think I would be happy with going for the "ABI break". Yes this could technically result in ODR violations, but in practice I can hardly imagine someone depending on the exact value of max_size(), whose documentation is:

Returns the maximum number of elements the string is able to hold due to system or library implementation limitations, i.e. std::distance(begin(), end()) for the largest string.

If it's really just about the value of max_size(), honestly I think this is something we could probably do unconditionally. I think the likelihood of that actually breaking the ABI for anyone is quite small. Didn't we return an incorrect value from that function for the longest time and it never affected anyone?

There are tests is our test suite that fail when compiled against the old headers and tested against the new ones.
Specifically, tests that check the type of exception thrown from large string allocations. We either throw length_error or bad_alloc. By increasing the max size, you change the exception that's thrown.

If we didn't have the versioned function symbols, I think the ODR violations could potentially be worse, but thankfully we're protected against that.

That said, I agree this case is likely esoteric, and not likely to bother real world users, but I'm not certain.

Honestly, I think this looks like a corner case and I think it is unlikely to bite users. Like I said, our value of max_size() was broken for the longest time (until 2db67e9 in 2021).

I would be tempted to make this change unconditionally and give everyone the memory savings. I believe that's the right tradeoff between being paranoid about ABI and actually delivering value to our users.

ldionne · 2023-10-10T23:08:44Z

libcxx/include/string

+#ifdef _LIBCPP_ABI_STRING_8_BYTE_ALIGNMENT
+      8
+#else
+      16


Do you know why the library originally used 16?

My suspicion is that someone figured 'malloc() always returns 16 byte aligned data' and likewise, "it is likely then also aligned in multiples of 16", which doesn't hold true. (in practice glib malloc is n * 16 + 8 for small allocations as it uses 8 bytes overhead).

That's just my guess, there is no need for any 16 byte alignment obviously in string, so it must have been some attempt to align better to the (default) allocator. (obviously this is different with tcmalloc, jemalloc, etc)

My conversations with howard agree with Martijn.

martijnvels · 2023-10-10T23:21:11Z

libcxx/include/__config

+// 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


Should we make the define instead be the direct value?

I.e.:

# define _LIBCPP_ABI_STRING_BYTE_ALIGNMENT 8 # define _LIBCPP_ABI_STRING_BYTE_ALIGNMENT 16

?

I would rather leave the ABI macro in the idiomatic form, despite it causing uglier code.

EricWF · 2023-10-11T14:37:30Z

Urg, I'm sorry. I fudged up the git history trying to reformat the change. I'll have to submit a new PR.

EricWF · 2023-10-11T14:43:47Z

New PR is #68807

ldionne · 2023-10-11T14:52:12Z

Urg, I'm sorry. I fudged up the git history trying to reformat the change. I'll have to submit a new PR.

Just FYI I believe it is possible to re-open a PR.

EricWF · 2023-10-11T16:06:29Z

In this case it was not. It says "There are no new commits on the efcs:align-8 branch". This is likely because I had to force-update the remote branch it used as a base.

EricWF added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Oct 10, 2023

EricWF assigned martijnvels Oct 10, 2023

EricWF requested a review from cjdb October 10, 2023 22:15

ldionne reviewed Oct 10, 2023

View reviewed changes

martijnvels reviewed Oct 10, 2023

View reviewed changes

martijnvels approved these changes Oct 10, 2023

View reviewed changes

EricWF closed this Oct 11, 2023

EricWF force-pushed the align-8 branch from bbad197 to 02678bc Compare October 11, 2023 14:34

EricWF mentioned this pull request Oct 11, 2023

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

EricWF commented Oct 10, 2023 •

edited

Loading

llvmbot commented Oct 10, 2023

github-actions bot commented Oct 10, 2023 •

edited

Loading

ldionne Oct 10, 2023

EricWF Oct 11, 2023

ldionne Oct 10, 2023

martijnvels Oct 10, 2023

ldionne Oct 10, 2023 •

edited

Loading

EricWF Oct 11, 2023

ldionne Oct 11, 2023

ldionne Oct 10, 2023

martijnvels Oct 10, 2023

EricWF Oct 11, 2023

martijnvels Oct 10, 2023 •

edited

Loading

EricWF Oct 11, 2023

EricWF commented Oct 11, 2023

EricWF commented Oct 11, 2023

ldionne commented Oct 11, 2023

EricWF commented Oct 11, 2023

	// Same memory by providing the allocator more freedom to allocate the most
	// Save memory by providing the allocator more freedom to allocate the most

		// pointer from 16 to 8. This changes the output of std::string::max_size,
		// which makes it ABI breaking

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

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

Conversation

EricWF commented Oct 10, 2023 • edited Loading

llvmbot commented Oct 10, 2023

github-actions bot commented Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldionne Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvels Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricWF commented Oct 11, 2023

EricWF commented Oct 11, 2023

ldionne commented Oct 11, 2023

EricWF commented Oct 11, 2023

EricWF commented Oct 10, 2023 •

edited

Loading

github-actions bot commented Oct 10, 2023 •

edited

Loading

ldionne Oct 10, 2023 •

edited

Loading

martijnvels Oct 10, 2023 •

edited

Loading