Add the C++17 <memory_resource> header (mono-patch)

[Quuxplusone]

Summary

This patch is the rebase and squash of three earlier patches. It supersedes all three of them.

• D47111: experimental monotonic_buffer_resource.
• D47358: experimental pool resources.
• D47360: Copy std::experimental::pmr to std::pmr.

The significant difference between this patch and the-sum-of-those-three is that this patch does not add std::experimental::pmr::monotonic_buffer_resource and so on. This patch simply adds the C++17 standard facilities, and leaves the std::experimental namespace entirely alone.

[Quuxplusone]

"UNSUPPORTED: no-exceptions" on three tests.
Use modern style ("override", no "virtual", no "protected") on three other tests which were otherwise copied from existing experimental/ tests.

[ldionne]

You seem to be missing several // UNSUPPORTED: c++11, c++14 annotations from your test. This is a C++17 feature, let's just provide it in C++17. This will also make the test suite more portable (other implementations might not ship pmr before C++17).

libcxx/include/__memory_resource_base

15

This is a C++17 addition, so I would use #if _LIBCPP_STD_VER > 14.

libcxx/include/memory_resource

15

This is a C++17 addition, so I would use #if _LIBCPP_STD_VER > 14.

[Quuxplusone]

Update language-version guards per Louis's review comments.
Update UNSUPPORTED guards on tests.
Add header_*_synop2.pass.cpp tests for every container type.

[Quuxplusone]

libcxx/include/__memory_resource_base

15

Done. However, I had copied this #ifndef _LIBCPP_CXX03_LANG from libc++'s C++17 <filesystem>; you might want to make a similar change to <filesystem>.

libc++'s <string_view> is not gated on language version at all.

[mclow.lists]

libcxx/include/__memory_resource_base

15

libc++'s <string_view> is not gated on language version at all.

That was a deliberate choice.

[ldionne]

@wash mentioned @griwes might be able to look at this, since he's done some work with memory_resource before.

[griwes]

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 (including tests) 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.

libcxx/include/memory_resource

250

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.)

293

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.

libcxx/src/memory_resource.cpp

51–55

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.

127

Maybe shared_mutex here? (Though if a program has a point of heavy contention here it's probably a problem in the program ;))

174

Same "header" naming comment here as earlier.

239

Same header naming comment here.

441

This variable doesn't seem very useful - assign directly to chunk_size_in_blocks?

[Quuxplusone]

libcxx/include/memory_resource

250

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.

293

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.

libcxx/src/memory_resource.cpp

51–55

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.

127

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. :)

441

Perhaps. Now I'm wondering why I used names like chunk_size_in_blocks instead of num_blocks_in_chunk.

[Quuxplusone]

Fix copyright headers. Maybe fix the failing monotonic-buffer-resource test, finally!
Still to do: Change "header" to "footer" throughout.
Still to do: Maybe rename e.g. "chunk_size_in_bytes => num_bytes_in_chunk".

[Quuxplusone]

Renamed chunk_header to chunk_footer and the corresponding header_{size,align} to footer_{size,align}. This actually makes the math in aligned_capacity easier to understand, IMO! I left the loop variable h alone, rather than rename it to f, because f seems uglier to me.

I left vacancy_header alone, because it is actually stored in the initial bytes of the chunk. (Right?)

So far, I've left initial_header alone, even though technically it is neither a header nor a footer, because it is stored outside of the initial buffer. Perhaps it could be renamed something like initial_info? Any thoughts on terminology there, @griwes?

Also in this update I've merged in a couple of threading-related fixes that have been made to <experimental/memory_resource> in the past year.

[Quuxplusone]

No change since last time. Rebase on D93137, and poke buildbots.

[griwes]

So far, I've left initial_header alone, even though technically it is neither a header nor a footer, because it is stored outside of the initial buffer. Perhaps it could be renamed something like initial_info? Any thoughts on terminology there, @griwes?

I think the name I've been using for things like that is "descriptor".

[Quuxplusone]

Rename "initial_header" to "initial_descriptor" (since technically it's neither a header nor a footer).
Rebase on main.