[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 added a comment.

In D102727#2894118 <https://reviews.llvm.org/D102727#2894118>, @ezbr wrote:

> [...]
>
> 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?

As I said, it's really not about whether to break the ABI. It's about whether this change is good in the general case or not, and I'm not so sure about that. For example, I just came across code in `locale.cpp` that calls `install()` successively, and `install()` precisely calls `resize(id + 1)`, growing the vector by one each time (I think): https://github.com/llvm/llvm-project/blob/main/libcxx/src/locale.cpp#L485 and https://github.com/llvm/llvm-project/blob/main/libcxx/src/locale.cpp#L199. And it's spread across multiple functions, so even a clang-tidy check would not catch it. It would not occur to me to use this pattern, but if even libc++ is doing it, I can imagine others are doing it too despite the lack of formal complexity guarantees on `resize()`.

I also just did an internal code search and in just 5 minutes, I can find at least a couple places that appear to be resizing in a loop with an increasing size like that.

Instead of changing `resize()` like this, have you considered using `shrink_to_fit()` more aggressively in places that have a noticeable memory impact?

I would be curious to write a paper proposing for `resize()` to be spec'd to have amortized O(1) complexity (which would effectively make this change non-conforming), as a way of seeing what other implementers think, and whether there has been other experience with this. If other people say "it's a useful implementation to shrink-to-fit on resize()", that would be valuable information to have. If everybody says "hmm, the intent always have been that `resize()` would behave consistently with `push_back()` w.r.t. amortized complexity", then that would be valuable input.

I'm sorry, but I can't approve something with potentially such a sharp edge without more guidance. I realize you're just trying to upstream a change that is useful on your end and I appreciate that, but I'm a bit stuck because I have to try to represent everybody's interests, and I'm really lacking data to make anything but the conservative "let's keep the status quo" decision.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102727