[libcxx-commits] [PATCH] D102727: Use precise growth rather than amortized growth in std::string::resize/__resize_default_init.

Wed Jul 21 11:41:45 PDT 2021

ezbr marked 3 inline comments as done.
ezbr added a comment.

> Thanks for your replies. I'd like to understand: how did you measure the memory improvements you found? Ideally, it would be nice to have something that we can reproduce on a large open-source code base (e.g. Chrome) so that the experiment can be verified. Is that possible? Otherwise, you have to understand I'm blindly basing the decision on "an internal Google experiment", which while I do trust, is not the best from a due diligence perspective. I'm being very conservative because silently introducing quadratic behavior in user code is pretty close to the scariest thing a maintainer can think about -- very bad but also very difficult to detect.

We used Google Wide Profiling to detect the memory improvements. It's described in this paper <https://research.google/pubs/pub36575/>, but I don't know of a similar system in place externally, unfortunately. We can see if there's a way to get similar data for Chrome.

> Also, if this came with some sort of Clang warning or clang-tidy check that flagged using resize() in a loop (because the standard doesn't guarantee amortized O(1) for that but people might be expecting it), I might ease up a bit. It still wouldn't fix the case where resize() is called in a loop lower in the call stack, but it would be good if we could detect obvious misuses automatically.

The clang-tidy check seems reasonable. I'll try to find someone who could write one.

> What was the quadratic case you encountered and fixed? Is that something you can share?

I can't share the code exactly, but I can say that it was using __resize_default_init to repeatedly extend a string and copy into the new space at the end of the string. I changed it to just use push_back/append. In that case, the problematic resize was in a different function from the loop so it wouldn't be caught by a simple clang-tidy.

Re: we shouldn't use ABI flag unless it's an ABI break. I can see @mvels's point that when users update to a new ABI version and/or opt-in to the unstable ABI they may be more aware that significant changes can occur and would potentially be less likely to have a bad experience with this change. Martijn had a related idea (discussing outside this review), which is: we could make resize() inline, and delegate to a new __resize_grow_exact() for n > size(). This would break the ABI to (a) improve performance, and (b) allow for a potentially more user-friendly transition to precise resize (guarded by unstable/version 2 ABI). @ldionne, what do you think of this idea?

================
Comment at: libcxx/test/libcxx/strings/basic.string/string.capacity/resize.exact.cpp:13
+
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ABI_EXACT_STRING_RESIZE
+
----------------
ldionne wrote:
> Enabling tests for the unstable ABI like this is not necessarily going to work. It may happen to work, but it could just as well not work, because the shared library will not have been built with the same ABI settings.
> 
> In other words, the ABI in use is not a property that can be changed by users with a mere `#define`, it's a configuration-wide setting that should be decided by vendors when they ship the library. I think what we need to do here instead is:
> 
> 1. add a CI configuration that builds libc++ with the unstable ABI and runs the tests under that configuration
> 2. move the existing tests that do something like this to using `// REQUIRES: libcpp-abi-unstable` (we have a couple of tricky ones that define things like `_LIBCPP_ABI_ENABLE_UNIQUE_PTR_TRIVIAL_ABI`)
> 
Is there documentation about how to add a CI configuration? Note that once I renamed the test to end in "pass.cpp", it was failing and seems to be using exponential growth so I think this is necessary.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102727