[PATCH] D47111: <experimental/memory_resource>: Implement monotonic_buffer_resource.

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

Quuxplusone marked 11 inline comments as done.
Quuxplusone added inline comments.


================
Comment at: include/experimental/memory_resource:427
+    static _LIBCPP_CONSTEXPR const size_t __default_buffer_capacity = 1024;
+    static _LIBCPP_CONSTEXPR const size_t __default_buffer_alignment = 16;
+
----------------
strager wrote:
> Nit: Why isn't `__default_buffer_alignment` set to `alignof(max_align_t)` (which I think is 16)? I think `alignof(max_align_t)` is a good, portable default.
Will users be more upset if `__default_buffer_alignment` is 16-instead-of-`???` on exotic platform `???`; or more upset if `__default_buffer_alignment` is //usually// 16 but then sometimes is `???`-instead-of-16 on exotic platforms? I tend to think that "usually 16, sometimes something else" is worse behavior software-engineering-wise than "always 16, even when something else might arguably be better on exotic platform `???`."


================
Comment at: include/experimental/memory_resource:431-432
+        __chunk_header *__next_;
+        char *__start_;
+        char *__cur_;
+        size_t __align_;
----------------
strager wrote:
> Nit: Would it make sense to use `std::byte` instead of `char`?
IMHO "strong no"; `char` is the standard core-language type that takes up one byte. `std::byte` is an enum type that we'd have to drag in from a header (cost), and otherwise behaves just like `char` (no benefit). But it's certainly a non-functional nit and I would not put up a fight if libc++ wanted it that way. :)


================
Comment at: include/experimental/memory_resource:433
+        char *__cur_;
+        size_t __align_;
+        size_t __allocation_size() {
----------------
strager wrote:
> > Eric suggests replacing size_t __align_ with uint8_t __log2_align_. I'm amenable to this idea, but I'd want to know what's the best way to compute the log2 of a user-supplied number.
> 
> Perhaps `std::log2p1` could help: https://en.cppreference.com/w/cpp/numeric/log2p1
Yes, but that's C++2a; I imagine some fiddliness would be needed to use it in C++17 library code (which in turn is actually compiled as C++11 or '14, I forget).


================
Comment at: test/libcxx/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_in_geometric_progression.pass.cpp:32
+        assert(globalMemCounter.checkOutstandingNewEq(0));
+    }
+
----------------
strager wrote:
> Nit: This test is named allocate_in_geometric_progression. I don't see any multiplication or anything happening in this test case. Did you mean to call this test something else, like release_deletes_upstream_memory?
Prior to the resolution of [LWG 3120](https://cplusplus.github.io/LWG/issue3120), this test had tested something else. I guess now it //should// be more like "release_resets_initial_buffer.pass.cpp".


================
Comment at: test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_deallocate.pass.cpp:28
+        std::experimental::pmr::monotonic_buffer_resource mono1(std::experimental::pmr::new_delete_resource());
+        std::experimental::pmr::memory_resource & r1 = mono1;
+
----------------
strager wrote:
> Nit: What is the purpose of the `r1` variable? Why not use `mono1` everywhere instead (and rename `mono1` to `r1` if you like `r1` better than `mono1` as a name)?
`r1` has a different static type. For example, here it's testing that `r1.allocate(50)` actually makes a virtual call to `monotonic_buffer_resource::do_allocate`. But I guess it doesn't really matter; I could use `mono1` for everything just as well.


================
Comment at: test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_exception_safety.pass.cpp:48-49
+    void *res = r1.allocate(64, 16);
+    assert(res == buffer);
+    assert(globalMemCounter.checkNewCalledEq(0));
+
----------------
strager wrote:
> Nit: These assertions look unrelated to exception safety to me. (The test is named allocate_exception_safety.) I'd delete these assertions.
Depends on your test-overfitting philosophy. These assertions are still expected to pass 100% of the time — if they fail, then something is definitely wrong. And they're cheap to execute. So IMHO they might as well be there, as "self-documenting code."


================
Comment at: test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_exception_safety.pass.cpp:67-68
+
+    upstream.which = std::experimental::pmr::new_delete_resource();
+    res = r1.allocate(last_new_size, 16);
+    assert(res != buffer);
----------------
strager wrote:
> Question about the intended behavior of `monotonic_buffer_resource`:
> 
> If line 68 instead allocated 1 byte, and the byte could fit in the original heap allocation (line 51), should `monotonic_buffer_resource` return a byte from that original resource? In other words, should the following test pass?
> 
> ```
> repointable_resource upstream(std::experimental::pmr::new_delete_resource());
> std::experimental::pmr::monotonic_buffer_resource mono1(&upstream);
> 
> globalMemCounter.reset();
> mono1.allocate(1, 1);
> const size_t first_new_size = globalMemCounter.last_new_size;
> bool mono1_has_spare_capacity = first_new_size == 1;
> 
> upstream.which = std::experimental::pmr::null_memory_resource();
> try {
>     mono1.allocate(first_new_size, 1); // Force allocation from upstream.
>     assert(false);
> } catch (const std::bad_alloc&) {
>     // we expect this
> }
> 
> globalMemCounter.reset();
> mono1.allocate(1, 1);
> if (mono1_has_spare_capacity) {
>   assert(globalMemCounter.checkNewCalledEq(0));
> } else {
>   assert(globalMemCounter.checkNewCalledEq(1));
> }
> ```
> 
> Similarly, if `monotonic_buffer_resource` was given a buffer, and that buffer still has spare capacity, should that capacity be reused? In other words, should the following test pass?
> 
> ```
> int first_allocation_size = 20;
> int second_allocation_size = 30;
> 
> auto *upstream = std::experimental::pmr::null_memory_resource();
> char buffer[64];
> assert(sizeof buffer >= first_allocation_size + second_allocation_size);
> std::experimental::pmr::monotonic_buffer_resource mono1(buffer, sizeof buffer, upstream);
> 
> globalMemCounter.reset();
> void *first_allocation = mono1.allocate(first_allocation_size, 1);
> assert(first_allocation == &buffer[0]);
> 
> try {
>     mono1.allocate(sizeof buffer, 1); // Force allocation from upstream.
>     assert(false);
> } catch (const std::bad_alloc&) {
>     // we expect this
> }
> 
> void *second_allocation = mono1.allocate(second_allocation_size, 1);
> assert(second_allocation == &buffer[second_allocation_size]);
> ```
> 
> I think these scenarios would be useful to test, in addition to the scenario in your test here.
> If [the request] could fit in the original heap allocation, should `monotonic_buffer_resource` return a byte from that original allocation?
> Similarly, if `monotonic_buffer_resource` was given a buffer, and that buffer still has spare capacity, should that capacity be reused?

In both cases, the Standard says "absolutely not": [mem.res.monotonic.buffer.mem#5](http://eel.is/c++draft/mem.res.monotonic.buffer.mem#5). I can add these tests.



Repository:
  rCXX libc++

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

https://reviews.llvm.org/D47111