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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a subscriber: ckennelly.
ldionne added a comment.
This revision now requires changes to proceed.

I like the way this change is done better now, but I'm still not sure we want/can do this -- I have a few questions I need the answer to before this LGTM. Sorry if that sounds like I'm trying to poke holes in your optimization, but doing my due diligence requires me to try for a type as important as `std::string` and a method as widely used as `resize()`. We really don't want to get this wrong.

1. Does this change how iterators are invalidated?

2. http://eel.is/c++draft/string.capacity#5.2 says: "If `n > size()`, appends `n - size()` copies of `c`". Now, `std::string::append` has the strong exception guarantee when an exception is thrown, which means that `std::string::resize` also does when `n > size()`. Does your change modify that (please explain)?

3. Can you explain what are the cases where this change introduced quadratic CPU usage? I assume this is whenever someone was calling `resize()` in a loop with a gradually larger size: previously, the code would have reserved exponentially so we would have reallocated very rarely, but with your change, we will reallocate every time.

@ckennelly I know you did a lot of work on `std::string`, can you take a look at this change?



================
Comment at: libcxx/include/string:3271
     }
     __invalidate_iterators_past(__pos);
 }
----------------
Can you add a libc++ specific test (under `libcxx/test/libcxx`) to check this new behavior?


================
Comment at: libcxx/include/string:3280
+    if (__n > __sz) {
+        if (__n > capacity()) __shrink_or_extend(__recommend(__n));
         append(__n - __sz, __c);
----------------
Please put the `__shrink_or_extend` call on its own line (same below).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102727