This is an archive of the discontinued LLVM Phabricator instance.

Optimize basic_string's memory usage
AbandonedPublic

Authored by ldionne on Jul 17 2023, 9:39 AM.

Details

Reviewers
EricWF
Mordante
mvels
Group Reviewers
Restricted Project
Summary
  • change __alignment from 16 to 8

The 16 value is likely inspired by the default glibc malloc() function returning 16 byte aligned memory. While this is true, this is not the underlying allocated capacity which for smaller allocs follows 8 + n * 16 which can be seen here: https://gcc.godbolt.org/z/WxeqWxqxY. Likewise, google's tcmalloc has finer grained allocations for all sizes from 64 - 128 bytes, meaning we also wast 8 bytes of allocation on every other allocation inside tcmalloc.

  • change 'grow_by' to use precise size for the first SSO --> long allocation

The logic currently always at least doubles the capacity on growth. However, this causes any short or empty initialized string exceeding 22 bytes to be allocated to at least 48 bytes. (22 * 2 rounded up to 48). This is wasteful as there are likely plenty of strings that could be allocated into 40 (glibc) or 32 (tcmalloc) sized allocs.

Diff Detail

Event Timeline

mvels created this revision.Jul 17 2023, 9:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2023, 9:39 AM
mvels requested review of this revision.Jul 17 2023, 9:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2023, 9:39 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
mvels updated this revision to Diff 541102.Jul 17 2023, 9:49 AM

Add grow_by without replace

mvels updated this revision to Diff 541104.Jul 17 2023, 9:51 AM

Upload grow_by amended changes

mvels updated this revision to Diff 541106.Jul 17 2023, 9:53 AM

Upload correct version this time

mvels updated this revision to Diff 541109.Jul 17 2023, 9:55 AM

Fix headers removed by clang-format mess

Thanks for working on this!

libcxx/include/string
2330

Can you add a bit more comment here why this is important. The text in the commit message contains a lot more information. When I read this message I was quite puzzled, after reading the commit message I understood what the reason for these changes are.

mvels updated this revision to Diff 543978.Jul 25 2023, 7:51 AM

Added extra comments on string growth

mvels marked an inline comment as done.Jul 25 2023, 7:51 AM
mvels edited reviewers, added: Mordante; removed: ldionne.Jul 25 2023, 7:57 AM
mvels edited subscribers, added: ldionne; removed: Mordante.
mvels added a comment.Jul 25 2023, 8:51 AM

BTW: I did not understand the Apple max_size.pass failure, I adjusted the test and they all pass (check-cxx) on my local git repo on x86.

mvels updated this revision to Diff 544357.Jul 26 2023, 7:15 AM

Fix what I am sure is a bug in checking for max_size() in grow_by_....

mvels updated this revision to Diff 544361.Jul 26 2023, 7:18 AM

Remove parenthesis from debugging

BTW: I did not understand the Apple max_size.pass failure, I adjusted the test and they all pass (check-cxx) on my local git repo on x86.

On the backdeployment targets the new code uses the old version of the dylib. My suspicion is there is an ABI incompatibility. @philnik do you see an issue in this patch?

BTW: I did not understand the Apple max_size.pass failure, I adjusted the test and they all pass (check-cxx) on my local git repo on x86.

On the backdeployment targets the new code uses the old version of the dylib. My suspicion is there is an ABI incompatibility. @philnik do you see an issue in this patch?

This is an ABI break, but I don't think that's the problem. max_size isn't instantiated. This look to me like there is an off-by-one error somewhere.

Could we split this into two patches? This is really seems like it has high potential for having subtile bugs.

libcxx/include/string
2328

Why do we want to change the growth specifically for the case when we go from short to long?

mvels added a comment.Aug 4 2023, 3:42 AM

BTW: I did not understand the Apple max_size.pass failure, I adjusted the test and they all pass (check-cxx) on my local git repo on x86.

On the backdeployment targets the new code uses the old version of the dylib. My suspicion is there is an ABI incompatibility. @philnik do you see an issue in this patch?

This is an ABI break, but I don't think that's the problem. max_size isn't instantiated. This look to me like there is an off-by-one error somewhere.

Could we split this into two patches? This is really seems like it has high potential for having subtile bugs.

Let me split the __alignment and growth changes in two separate patches

libcxx/include/string
2328

This is where most of the waste comes from, and we can also argue that the most common use case isn't actually 'Growing' the string, i.e., the doubling is to keep amortized growth on continued growth.

I.e., a common pattern is:

std::string s;
s = SomeValue();

In the above case assigning a value to s is not really "growing" the string, the fact that the size changes from 0 to n is a consequence of string not having a notion of "unassigned" (which is generally good).

But if you consider the above code, and SomeValue() returns a string of say, length 24, then the above algorithm would simply say we "grow from 0 to 24" and applies max(24, __min_cap * 2) = 44 bytes as minimum allocation size, which is wasteful.

Assigning values once to empty strings is a very common occurrence in practice, and there is no good reason to apply amortized growth to cases where a string goes from SSO to non SSO, and not doing so (precise sizing) saves memory,

philnik added inline comments.Aug 4 2023, 9:48 AM
libcxx/include/string
2328

My question was more meant as "why don't we just change the growth rate for everything?". That would save us a branch here, and AFAIK there is nothing preventing us from going that route.

ldionne commandeered this revision.Nov 3 2023, 8:33 AM
ldionne added a reviewer: mvels.

[Github PR transition] Commandeering to abandon since this landed in https://github.com/llvm/llvm-project/pull/68807 and another one I can't seem to find.

ldionne abandoned this revision.Nov 3 2023, 8:33 AM