[libcxx-commits] [libcxx] [libc++][safety] Enhance exception safety for vector assignments in reallocation scenarios (PR #117516)

[Peng Liu via libcxx-commits]

winner245 wrote:

> In the scenario I'm describing it's not about the increased memory usage, but rather that the allocator may have to take a much slower path now, potentially even back to the kernel, instead of giving back the just deallocated block.

I am not convinced that my specific usage of the allocator could cause an impact "**even back to the kernel**." In this PR, the deallocation of the old vector is postponed until the successful construction of the new vector. We are merely delaying the deallocation slightly. This usage pattern is quite common in practice and is employed in the same ways inside `vector` and other sequence containers. For example, `emplace_back`, `push_back`, `reserve`, and `shrink_to_fit` all postpone the deallocation of the old vector until the new vector is successfully constructed, mirroring the same approach in this PR. Do they impact the allocator's kernel? No. Generally, it is doubtful that a user of the allocator should be concerned about their correct usage pattern negatively impacting the allocator. Whether the allocator can immediately reuse newly deallocated memory is highly indeterminate and depends on the underlying implementation details of the allocator. As users, we should not worry about the hidden intricacies of the allocator in general. Concerns about allocator performance are valid but fall under the responsibilities of the allocator's author.


> I don't think we save anything for a user here. If a user cares about exception safety they'll just implement exactly what we'd do in the allocating case after this patch. The branches you're describing would be pretty much identical, so there doesn't seem to be much benefit in conditionally using assign instead of constructing a new vector.

If a user implements this approach themselves, on top of the current internal library implementation, they would essentially double the total operations compared to the proposed change that replaces the internal implementation inside the library. Informally, this means that **Internal lib implementation + user implementation = 2 * new internal lib implementation**. As I already explained, the proposed change dese not add additional operations, compared to the current library implementation. 

The internal implementation already performs one round of reallocation and construction, which may fail at any time. Let's quantify this effort as `T(N-1)`, indicating a failure during the construction of the last element. To ensure strong exception safety, the user would have to copy the vector beforehand, requiring a `T(N)` operation. Upon detecting a failure after `T(N-1)` operations by the internal implementation, the user must perform an additional `T(1)` swap, totaling `T(2N)`. 

In contrast, the proposed implementation constructs the assigned elements in a new `__split_buffer`, which, if it fails during the final construction, consumes `T(N-1)` time. The original vector remains intact, requiring no recovery efforts. Thus, the proposed implementation reduces the total time from `T(2N)` to `T(N-1)`, not only saving user effort but also improving performance.

https://github.com/llvm/llvm-project/pull/117516