[libcxx-commits] [PATCH] D109510: [libc++] string: Allocate exactly when growing from short to long string.

Fri Sep 10 07:30:54 PDT 2021

courbet added a comment.

In D109510#2992763 <https://reviews.llvm.org/D109510#2992763>, @Quuxplusone wrote:

>> We have measured this overhead to be significant on our (Google) internal workloads, as 64% of global string allocations (and 26% of allocated bytes) are 48 bytes.
>
> And after this patch, what are the numbers? Does it become, for example, "30% of global string allocations are 48 bytes and 34% are 32 bytes"?
>
> Seems plausibly like a good idea to me, but I think your rationale is conspicuously incomplete without those "after" numbers.

Unfortunately I can't give the "after" number for now, because this analysis is for the whole fleet, which includes an extremely large number of binaries which I cannot all run with my patch.

One easy example for which I can give number is the protobuf descriptors I mentioned in the description (0.17% of the fleet memory) given how ubiquitous protobuf usage is at google. The exact numbers depend on what protos are linked into a given binary, but I looked at a few of our largest jobs and here's the percentage of allocations that are switched from 48 bytes to 32 bytes:

|          | % in [22;31] |
| server 1 | 48.1         |
| batch 1  | 42.1         |
| batch 2  | 42.9         |
| batch 3  | 48.9         |
|

================
Comment at: libcxx/include/string:2316
     pointer __old_p = __get_pointer();
+    const bool __was_short = __old_cap + 1 == __min_cap;
     size_type __cap = __old_cap < __ms / 2 - __alignment ?
----------------
Quuxplusone wrote:
> FWIW, I'd rather see `bool __was_short = __old_cap < __min_cap;` — no const (for consistency with every other variable in this function even though, sure, we're not planning to modify any of them), and shorter/simpler condition. Assuming the condition `__old_cap < __min_cap` remains correct, that is.
I had originally kept the old condition, but I agree that this one is clearer. Both should be equivalent as `__invariants` checks that `__cap < __min_cap` => `__cap + 1 == __min_cap`. Done.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109510/new/

https://reviews.llvm.org/D109510