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

[Nikolas Klauser via libcxx-commits]

philnik777 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 a negative 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 completely disagree. This change is switching to a fundamentally more difficult scenario to handle efficiently. Just because we have to use this pattern in some places doesn't mean we shouldn't acknowledge that we should avoid it if possible.

> > 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 * Internal lib implementation = 2 * new internal lib implementation**. As I already explained, the proposed change dose 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.

Swapping two vectors is a constant time operation if they have the same allocators, so I'm not sure how you get to twice the number of operations.

Closing, since I don't think we want to pursue this further and the author hasn't been active. If anyone wants to continue discussion, feel free to re-open.


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