[libcxx-commits] [PATCH] D109510: [libc++] string: Allocate exactly when growing from short to long string.
Clement Courbet via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
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
More information about the libcxx-commits
mailing list