[libcxx-commits] [PATCH] D122877: [libc++] Implement P0401R6 (allocate_at_least)

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__memory/allocator.h:112
+    allocation_result<_Tp*> allocate_at_least(size_t __n) {
+        return {allocate(__n), __n};
+    }
----------------
philnik wrote:
> var-const wrote:
> > philnik wrote:
> > > var-const wrote:
> > > > Hmm, if I'm reading this correctly, this is exactly equivalent to the old behavior -- even though we implement an interface that would allow an allocator to overallocate, the actual standard allocators never overallocate. Does this require a TODO? Or is this feature only for user-provided allocators?
> > > I don't think there is anything preventing an implementation from over-allocating, but I don't know of a standard C API for over-allocating. We can of course special-case this for malloc implementations that have an API for over-allocating.
> > Maybe I'm misreading the paper (I wasn't aware of it before seeing this patch), but it seemed like the whole purpose of `allocate_at_least` is to avoid wasting capacity in case an allocator overallocates. I'm not clear, however, if this only applies to user-provided allocators, or if it's meant specifically for `std::allocator`, but given the wording changes for `std::allocator`, I presume the latter.
> > 
> > I don't know if the underlying allocator machinery we're currently using is overallocating and if there's a way to query the actual allocated size. However, I think that:
> > - if our underlying allocations never overallocate, we should add a comment to that effect to `std::allocator<T>::allocate_at_least`;
> > - otherwise, we should add a TODO.
> > 
> > In short, if I'm reading the paper correctly, it's meant to bring a behavioral change, whereas with the current implementation in this patch, the behavior will be exactly equivalent. Once again, I might well be missing or misreading something -- please point it out if I do.
> I think you are right that the implementation is allowed to return more memory than was requested. The problem is that we have no way to tell if the underlying allocator ever over-allocates. We don't have an API that can tell us that. There is no C-version of `allocate_at_least`. The only way to support this currently is through extensions. But I don't think we should add a TODO there. This TODO would never be removed, and what's the point of a TODO then? We also don't have a `// TODO: optimize sort`.
I feel that we would have to use compiler-internal functions, so it's okay that there is no C-version. I wouldn't call it an extension, though -- some library functionality can only be implemented using magical compiler intrinsics.

Regarding the TODO, I think the full intention of the paper won't be realized until this is implemented, so the TODO is necessary. The `optimize sort` case is different -- there is always room for optional optimization, but in this case I feel this is almost a mandatory optimization. To continue with the sort analogy, if we had a `sort` implementation that had a `O(N^2)` worst-case complexity, a TODO would be in order.

Having said that, I don't know if the actual low-level allocation functions we use support or ever plan to support over-allocating. @ldionne what are your thoughts on this?


================
Comment at: libcxx/test/std/utilities/memory/allocator.traits/allocate_at_least.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
var-const wrote:
> I think there also needs to be a test for `std::allocator<T>::allocate_at_least`. This test essentially checks that `std::allocate_at_least` is correctly forwarded to the allocator's `allocate_at_least`.
Thanks for expanding the test! What I meant is that since we're adding two new functions to the API (`std::allocate_at_least` and `std::allocator<T>::allocate_at_least`), we should have separate test for both of them -- i.e., we can't rely on `std::allocate_at_least` tests to test `std::allocator<T>::allocate_at_least`. I know that, at least in the current state, `std::allocator<T>::allocate_at_least` is equivalent to `std::allocator<T>::allocate` and there isn't much to test, but I still feel that at least a rudimentary test needs to be added. But please check with @ldionne -- perhaps what I'm suggesting is overkill. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122877