[libcxx-commits] [PATCH] D122877: [libc++] Implement P0401R6 (allocate_at_least)
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Apr 6 12:37:01 PDT 2022
ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.
LGTM but I'd like to see the updated tests. Thanks!
================
Comment at: libcxx/include/__memory/allocate_at_least.h:47
+struct __allocation_result {
+ // both count and ptr are already allowed because of `std::count()` and `std::from_chars_result` respectively
+ _Pointer ptr;
----------------
I don't think this comment is necessary -- IMO it creates confusion where there should be none. Someone looking at this code and going "Hmm, I don't think those identifiers are valid" would easily convince themselves that the world is broken if we can't write that pre-C++23.
================
Comment at: libcxx/include/__memory/allocator.h:112
+ allocation_result<_Tp*> allocate_at_least(size_t __n) {
+ return {allocate(__n), __n};
+ }
----------------
var-const wrote:
> 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?
My understanding of the paper's intent is that we add this interface so that user-defined allocators can take advantage of it, *and* system allocators too when possible. However, I agree with @philnik that the C library simply doesn't give us a standard way to take advantage of this at the moment, so I think our implementation is optimal (with one caveat, see below). Because of this, I feel that we *are* implementing the paper faithfully, and there is nothing left to do, so I wouldn't leave any comment. I hope this makes sense.
Caveat: On systems that support it, we could perhaps use `malloc_usable_size` (https://man7.org/linux/man-pages/man3/malloc_usable_size.3.html) to query how much memory `malloc` gave us in reality. Doing that in `std::allocator` is going to create interesting challenges, though, since we use `operator new` from `std::allocator`, not `std::malloc` directly. If it's even possible to do it, I think it will mandate being done in a separate patch as it will certainly be somewhat involved.
In my opinion, where P0401r6 perhaps didn't go far enough is by not adding some sort of `operator new` where you get a size feedback (oh wait, this is https://wg21.link/p0901r5)! If we had that, then we could use that from `std::allocator` instead. And from the dylib (where we'd implement that novel `operator new`), we'd be in a position to call `malloc` and use `malloc_usable_size()` on the returned pointer (whenever that's supported).
So I guess I can summarize my opinion by saying that `std::allocator` is basically a veneer on top of `operator new`, and so any optimization needs to be made in `operator new` itself, not in `std::allocator`.
================
Comment at: libcxx/include/memory:101-109
+template<class Pointer>
+struct allocation_result {
+ Pointer ptr;
+ size_t count;
+};
+
+template<class Allocator>
----------------
I think we need some `// since C++20`s?
================
Comment at: libcxx/include/string:1862
{
- size_type __cap = __recommend(__reserve);
- __p = __alloc_traits::allocate(__alloc(), __cap+1);
+ auto __allocation = std::__allocate_at_least(__alloc(), __recommend(__reserve) + 1);
+ __p = __allocation.ptr;
----------------
After we ship this, I would like us to revisit the patch where you modified `__recommend`'s return value (I can't find it ATM). See if it makes that patch any easier.
================
Comment at: libcxx/include/vector:683
+ // Precondition: __n > 0
+ // Postcondition: capacity() == __n
+ // Postcondition: size() == 0
----------------
================
Comment at: libcxx/include/vector:685
+ // Postcondition: size() == 0
+ void __vallocate(size_type __n) {
+ if (__n > max_size())
----------------
`_LIBCPP_HIDE_FROM_ABI`?
================
Comment at: libcxx/include/vector:2337
+ // Precondition: __n > 0
+ // Postcondition: capacity() == __n
+ // Postcondition: size() == 0
----------------
================
Comment at: libcxx/include/vector:2340
_LIBCPP_INLINE_VISIBILITY void __invalidate_all_iterators();
- void __vallocate(size_type __n);
+ void __vallocate(size_type __n) {
+ if (__n > max_size())
----------------
`_LIBCPP_HIDE_FROM_ABI`
================
Comment at: libcxx/test/libcxx/diagnostics/detail.headers/memory/allocate_at_least.module.verify.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Please rebase onto `main`.
================
Comment at: libcxx/test/std/utilities/memory/allocator.traits/allocate_at_least.pass.cpp:14
+// template<class Allocator>
+// [[nodiscard]] constexpr allocation_result<typename allocator_traits<Allocator>::pointer>
+// allocate_at_least(Allocator& a, size_t n);
----------------
Can you also add a simple test like
```
using AllocationResult = std::allocation_result<int*>;
```
Just to check the box of testing specifically for that type's existence.
================
Comment at: libcxx/test/std/utilities/memory/allocator.traits/allocate_at_least.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
var-const wrote:
> 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.
I agree -- we definitely need to treat `std::allocate_at_least` and `std::allocator<T>::allocate_at_least` as if they were different things, since they are different functions in the API. This test seems to be fine for `std::allocate_at_least`, but I'd like a separate test for `std::allocator::allocate_at_least`. You probably want to take some inspiration from `libcxx/test/std/utilities/memory/default.allocator/allocator.members/allocate.pass.cpp` & friends for that one (e.g. testing overaligned types, etc.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122877/new/
https://reviews.llvm.org/D122877
More information about the libcxx-commits
mailing list