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

[Michał Dominiak via Phabricator via libcxx-commits]

griwes accepted this revision.
griwes added a comment.

This all definitely feels extremely familiar - this is essentially the same code I've written for `thrust::mr`, minus the semantic extensions we've done and some code organization differences. I've left few minor comments below, but otherwise this looks good, with two additional top level comments that should be resolved before this is merged:

1. I have only skimmed over most of the code dealing with `polymorphic_allocator::construct`, so someone who is more familiar with all that machinery should review it in detail.
2. It looks like some files you're adding have an Apache w/ LLVM exceptions license header, some have a dual MIT/UIUC license header, and some have UIUC only license header.



================
Comment at: libcxx/include/memory_resource:249
+        size_t __allocation_size() {
+            return (reinterpret_cast<char*>(this) - __start_) + sizeof(*this);
+        }
----------------
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.)


================
Comment at: libcxx/include/memory_resource:292
+    _LIBCPP_INLINE_VISIBILITY
+    monotonic_buffer_resource(void *__buffer, size_t __buffer_size,
+                              memory_resource *__upstream)
----------------
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.


================
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();
+        }
----------------
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.


================
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) {
----------------
Maybe `shared_mutex` here? (Though if a program has a point of heavy contention here it's probably a problem in the program ;))


================
Comment at: libcxx/src/memory_resource.cpp:173
+    size_t __allocation_size() {
+        return (reinterpret_cast<char*>(this) - __start_) + sizeof(*this);
+    }
----------------
Same "header" naming comment here as earlier.


================
Comment at: libcxx/src/memory_resource.cpp:238
+        size_t __allocation_size() {
+            return (reinterpret_cast<char*>(this) - __start_) + sizeof(*this);
+        }
----------------
Same header naming comment here.


================
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),
----------------
This variable doesn't seem very useful - assign directly to `chunk_size_in_blocks`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89057