[libcxx-commits] [PATCH] D89057: Add the C++17 <memory_resource> header (mono-patch)

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone marked an inline comment as done.
Quuxplusone added inline comments.


================
Comment at: libcxx/include/memory_resource:249
+        size_t __allocation_size() {
+            return (reinterpret_cast<char*>(this) - __start_) + sizeof(*this);
+        }
----------------
griwes wrote:
> Reading in order, so this may be answered later, but `this - start` suggests that this is not a _header_, and rather it's a _footer_; is that right? Can we tweak the naming here so that we don't call a footer a header? (If this sounds too pedantic to you, feel free to ignore - but it gave me a long pause on the `this - start` expression on the first read.)
Yes, this "chunk header" appears //after// the allocation; I don't object to calling it a "footer," if that terminology is less likely to confuse. I dimly recall that my first draft probably had this as a real "header," which of course wastes a ton of space on overaligned allocations, so then it moved to the end without changing its name.

I'll s/header/footer/ in the next revision, hopefully next week, unless I hear objections from anyone on the name change.


================
Comment at: libcxx/include/memory_resource:292
+    _LIBCPP_INLINE_VISIBILITY
+    monotonic_buffer_resource(void *__buffer, size_t __buffer_size,
+                              memory_resource *__upstream)
----------------
griwes wrote:
> What's the rationale you've followed for deciding what goes fully into the header vs what goes into the dylib? I'm curious about the difference between this constructor and the constructor for the unsynchronized pool.
As a rule they're out-of-line, except for where @ericwf advised me to keep them inline. (See https://reviews.llvm.org/D47111#inline-429047 ). I think the rationale here is that `monotonic_buffer_resource` is likelier to be amenable to optimization if we let the inliner see everything that's happening, whereas `*_pool_resource` is so complicated that the optimizer won't be able to do much with it.


================
Comment at: libcxx/src/memory_resource.cpp:50-54
+        void *result = _VSTD::__libcpp_allocate(bytes, align);
+        if (!is_aligned_to(result, align)) {
+            _VSTD::__libcpp_deallocate(result, bytes, align);
+            __throw_bad_alloc();
+        }
----------------
griwes wrote:
> Whether this is what happens or not seems unspecified in the standard; is this implementation aligned with any other implementations of PMR that are out there in the wild? For Thrust I've chosen the other option and just overallocate to ensure the alignment, but if this is the common behavior across other implementations, I'm fine with it.
Notice that this ugly codepath happens only if `_LIBCPP_HAS_NO_ALIGNED_ALLOCATION`. When aligned allocation is known to be supported, then we trust `__libcpp_allocate(bytes, align)` to return a correctly aligned pointer.

Libstdc++ always trusts `::operator new(bytes, align)` to return a correctly aligned pointer. They do not have a "no-aligned-allocation" version. https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/src/c%2B%2B17/memory_resource.cc#L50

MSVC has two implementations, like me. The "normal" version trusts the result of `::operator new(bytes, align)` for overaligned requests, but will prefer to call `::operator new(bytes)` for small alignments. I don't know their rationale for this.
MSVC's "no-aligned-allocation" version (when `/Zc:alignedNew-`) will throw `bad_alloc` in response to any overaligned request, without even trying to allocate.
https://github.com/microsoft/STL/blob/ea156e730cf3370d0551a0532669b7de90c78915/stl/inc/memory_resource#L57

So this "no-aligned-allocation" version is more "helpful" than MSVC's, because it will sometimes succeed. If it fails anyway, then it throws `bad_alloc` just like MSVC. But, yes, what I'm doing here //is// novel.

I do wish that I better understood the rationale for having `__libcpp_allocate(bytes, align)` in the first place.


================
Comment at: libcxx/src/memory_resource.cpp:126
+    _LIBCPP_SAFE_STATIC static memory_resource * res = &res_init.resources.new_delete_res;
+    static mutex res_lock;
+    if (set) {
----------------
griwes wrote:
> Maybe `shared_mutex` here? (Though if a program has a point of heavy contention here it's probably a problem in the program ;))
This codepath is hit only if the platform lacks `<atomic>`. In this decade, I have no idea what wacky platforms might be using this codepath. This is all copied from old code in experimental/; I did not write it. :)


================
Comment at: libcxx/src/memory_resource.cpp:440
+            if (prev_chunk_size_in_blocks == 0) {
+                size_t min_blocks_per_chunk = max(
+                    __min_bytes_per_chunk >> __log2_pool_block_size(i),
----------------
griwes wrote:
> This variable doesn't seem very useful - assign directly to `chunk_size_in_blocks`?
Perhaps. Now I'm wondering why I used names like `chunk_size_in_blocks` instead of `num_blocks_in_chunk`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89057