Page MenuHomePhabricator

<experimental/memory_resource>: Implement monotonic_buffer_resource.
Needs ReviewPublic

Authored by Quuxplusone on May 19 2018, 3:47 PM.

Details

Summary

(D47090 included this in <memory_resource>; at Eric's request, I've split this out into its own patch applied to the existing <experimental/memory_resource> instead.)

Depends on D46806 D47109 D47344

Diff Detail

Repository
rCXX libc++

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Quuxplusone retitled this revision from Implement monotonic_buffer_resource in <experimental/memory_resource> to <experimental/memory_resource>: Implement monotonic_buffer_resource..
Quuxplusone added 1 blocking reviewer(s): EricWF.

Fix one visibility macro.

FYI I have a full implementation of this laying around as D27402 (https://reviews.llvm.org/D27402). But I have never taken the time to resolve merge conflicts.
Feel free to steal any of the tests if they're still relevant.

include/experimental/memory_resource
428

I can't imagine we'll need more than 1 byte to represent the alignment.

473

Lets add override to these functions.

src/experimental/memory_resource.cpp
212

Drop the braces for conditionals and loops with single statements.

Quuxplusone added inline comments.May 28 2018, 7:05 PM
include/experimental/memory_resource
428

Even assuming for the sake of argument that that's right, it wouldn't buy us anything here because of padding, would it?

At the moment, __alignment_ needs to have enough range to store the align parameter passed to monotonic_buffer_resource::do_allocate. In an SSE world we should not blithely assume that align < 256. We could store lg(align) in a single byte since align is always a power of two and less than 2^64 — but then we're back to the first argument, which is that it'll be padded to 8 bytes anyway so what do we gain.

473

I grepped for "override" in include/ and saw exactly one (accidental?) use in experimental/filesystem, so I thought probably libc++ had a policy not to use it for portability reasons. I'll add it throughout, but would like someone to explicitly confirm that

(A) it's okay to use in this header and wouldn't need to be guarded with a _LIBCPP_OVERRIDE macro or anything

(B) Arthur should not bother trying to add it to any other headers, not even "for consistency"

src/experimental/memory_resource.cpp
212

*shiver* but okay.

232

For reference, here is where we ask the upstream for a block aligned according to the user's align.
It occurs to me that the upstream might not be able to satisfy such a request (actually, new_delete_resource works that way for me because libc++ doesn't support aligned new and delete on OSX), which would make the upstream throw bad_alloc. We handle this by passing along the exception. We could conceivably handle it by retrying with

aligned_capacity += align;
__res_->allocate(aligned_capacity, alignof(max_align_t));
header->__alignment_ = alignof(max_align_t);

but I'm not sure that that's any of monotonic_buffer_resource's business. Happy to make the patch if you think it ought to be.

EricWF added inline comments.May 28 2018, 7:46 PM
include/experimental/memory_resource
427

Can't we determine the capacity in most cases simply using static_cast<char*>(this) - static_cast<char*>(__start_)?

428

Even assuming for the sake of argument that that's right, it wouldn't buy us anything here because of padding, would it?

It could. (A) we don't actually need to include the types trailing padding when tail allocating it as a part of a buffer.
and less importantly (B) I'm not sure all ABI's require the trailing padding of a type be included when that type is a data member of another type (I might just be wrong on this).

I'm also looking into other ways of improving the way your implementation packs data, which may cause this to be beneficial.

473

The header already assumes a full C++11 implementation (it uses std::tuple), the override keyword will bi available. No need for a special macro.

test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic_buffer.pass.cpp
1 ↗(On Diff #148542)

Instead of testing the whole of monotonic_buffer in a single file, this test should be broken up into separate files. Roughly one per function, or when it makes sense, one per subsection for the lowest level of heading (For example a single test for all constructors under memory.buffer.ctor ).

EricWF added inline comments.May 28 2018, 7:48 PM
src/experimental/memory_resource.cpp
232

I was initially thinking of storing lg(align).

Add override; disintegrate the unit test; adopt some tests from Eric's D27402.
Also fix one QOI issue discovered by Eric's tests: if the user passes an initial_size to the constructor, then they are probably intending that our first upstream-allocation be big enough to serve at least initial_size 1-byte allocations (or one initial_size-byte allocation with a suitably small alignment). This is explicitly not mandated by the Standard (it merely requires that our next upstream-allocation be of size at least initial_size), but it's probably a healthy choice.

Refactor to remove unused fields from the header structs.

Before this change, sizeof(monotonic_buffer_resource) == 56 and the overhead per allocated chunk was 40 bytes.
After this change, sizeof(monotonic_buffer_resource) == 48 and the overhead per allocated chunk is 24 bytes.

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.

Quuxplusone marked 6 inline comments as done.May 28 2018, 10:36 PM
Quuxplusone added inline comments.Jun 10 2018, 7:25 PM
include/experimental/memory_resource
488

I've discovered that Boost.Container does not bother to preserve this state across calls to release(). If that's legal, then we can save 8 bytes here. I've asked for an LWG issue to be opened on the subject of "what the heck is release() supposed to do anyway."

  • Shrink monotonic_buffer_resource, and make release() not "leak" allocation size.

    Repeatedly calling allocate() and release() in a loop should not blow up. I originally thought this was required by the Standard, and maybe it is, but that was before I discovered this terrible effect, and before Pablo Halpern told me that it was not his actual intent.

    Since the only purpose of storing __next_buffer_size_ in the object was to implement the "looping blows up" misfeature, we can now get rid of that field, which shrinks monotonic_buffer_resource from 56 bytes down to 48 bytes, matching Boost.Container's implementation.
  • Add REQUIRES: c++experimental to all tests (oops).
Quuxplusone added inline comments.Jun 13 2018, 1:43 AM
include/experimental/memory_resource
488

On discussion with Ion Gaztañaga, Pablo Halpern, and Casey Carter, I've come to the conclusion that we *must not* preserve this state across calls to release, no matter what the Standard currently says, because that leads to insanely high memory usage when calling mr.release() in a loop. (Casey implies that MSVC's implementation has this issue; Pablo confirms that he never intended it.)

Fixed my implementation and added a regression test in test/libcxx/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_in_geometric_progression.pass.cpp.

Minor cosmetic changes.

Quuxplusone marked 7 inline comments as done.Jun 21 2018, 11:14 PM
EricWF added a comment.Jul 3 2018, 8:21 PM

Refactor to remove unused fields from the header structs.

Before this change, sizeof(monotonic_buffer_resource) == 56 and the overhead per allocated chunk was 40 bytes.
After this change, sizeof(monotonic_buffer_resource) == 48 and the overhead per allocated chunk is 24 bytes.

Woo! This is looking great!

I'm done looking at the implementation, I'll take a pass at the tests tomorrow. Some pointers in general:

  1. Tests should be named after the component they test, not how they're testing it.
  2. All the tests for a single component should be in the same file.
  3. You can use LIBCPP_ASSERT to check specific implementation details, like exactly how much our allocation sizes grow.

(1) and (2) are important for maintaining the tests. (3) will probably be useful for actually writing meaningful tests despite
most observable behavior being implementation specific.

include/experimental/memory_resource
424

Can these classes be members of monotonic_buffer_resource?

450

Lets keep these constructors inline.

466

Need _LIBCPP_FUNC_VIS

470

I think it would be nice for release to be inline. What do you think?

488

__previous_allocation_size isn't used in the header, so it shouldn't be declared there. We don't want to expose more symbols than we need to.

src/experimental/memory_resource.cpp
207

In general, please wrap to 80 characters.

test/std/experimental/memory/memory.resource.monotonic.buffer/nothing_to_do.pass.cpp
1 ↗(On Diff #153881)

We don't actually need this file, though older libc++ tests will often include it.

It's only needed to keep empty directories around, but this one has children.

Quuxplusone marked 7 inline comments as done.Jul 3 2018, 10:02 PM

I'll take a pass at the tests tomorrow. Some pointers in general:

  • Tests should be named after the component they test, not how they're testing it.
  • All the tests for a single component should be in the same file.

I'm certain I'm doing it fairly wrong and you'll have to give me specific handholds like "merge these two tests into one file", "rename this file to x.cpp", etc.
The current large number of files is only because *last* iteration, you told me to split up the tests from "one big monotonic_buffer.pass.cpp" into one file per test!
The problem with "one file per component" is that I don't understand what a "component" is in this context. From last iteration, I know that "all of monotonic_buffer_resource" is too big of a component! I also know that it's impossible to test deallocate without also calling allocate, but the reverse is not true.

include/experimental/memory_resource
470

Good point. It does pull a ton of stuff into the public header, but it's probably worth it.
I'm also moving the virtual destructor into the public header, so it can be inlined. (do_allocate becomes the new key function.)

test/std/experimental/memory/memory.resource.monotonic.buffer/nothing_to_do.pass.cpp
1 ↗(On Diff #153881)

I was starting to wonder about this! Removed.

Quuxplusone marked 2 inline comments as done.

Massive changes based on Eric's feedback. Moved constructors and destructors of monotonic_buffer_resource into the header so they can be completely inlined. Renamed struct __monotonic_buffer_foo_header to nested types monotonic_buffer_resource::__foo_header. Refactored for 80-column lines.

Quuxplusone marked an inline comment as done.Jul 3 2018, 10:23 PM

I'll take a pass at the tests tomorrow. Some pointers in general:

  • Tests should be named after the component they test, not how they're testing it.
  • All the tests for a single component should be in the same file.

I'm certain I'm doing it fairly wrong and you'll have to give me specific handholds like "merge these two tests into one file", "rename this file to x.cpp", etc.
The current large number of files is only because *last* iteration, you told me to split up the tests from "one big monotonic_buffer.pass.cpp" into one file per test!

I'm sorry, I don't mean to jerk you around. I'm still learning to be a reviewer. I'll provide some specific examples to avoid the ambiguity from before.

The problem with "one file per component" is that I don't understand what a "component" is in this context. From last iteration, I know that "all of monotonic_buffer_resource" is too big of a component! I also know that it's impossible to test deallocate without also calling allocate, but the reverse is not true.

Again, my apologies.

Oops, my previous diff had a stray ) in it, somehow.

Long-delayed ping!

Rebased on master. @EricWF @ldionne ping please?

EricWF accepted this revision.Nov 26 2018, 9:32 PM

LGTM. The tests might need some XFAIL'ing, but otherwise I think this looks good.

Sorry for the wait.

This revision is now accepted and ready to land.Nov 26 2018, 9:32 PM

@EricWF ping? This still needs someone to land it; I don't have commit privs. Thanks!

EricWF requested changes to this revision.Dec 9 2018, 6:00 PM

Some of the tests aren't passing with Trunk clang. Please fix these.

include/experimental/memory_resource
427

constexpr these constants if they're const?

test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_overaligned_request.pass.cpp
33

This check isn't passing with ToT clang.

44

Either remove this or uncomment them. Preferably the latter. More test coverage is better.

This revision now requires changes to proceed.Dec 9 2018, 6:00 PM
Quuxplusone marked 5 inline comments as done.

@EricWF ping!

Updated and addressed review comments.
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_overaligned_request.pass.cpp is now XFAILed on OSX platforms, using code copied-and-pasted from test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp.

@EricWF ping!

@EricWF ping!
and oops, I never submitted these comments, I guess.

include/experimental/memory_resource
427

Compare <ios>, which has "constexpr" in the synopsis comment but "static const" in the code.
However, I updated the diff to follow <random>'s example and use static _LIBCPP_CONSTEXPR const everywhere.

test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_overaligned_request.pass.cpp
33

Agreed, it won't pass unless the underlying new_delete_resource supports overaligned alignments, which isn't true unless the underlying runtime supports aligned new. Which isn't true on my MacBook.

I think the fix would be for me to copy all the XFAIL lines from test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp, is that right?

Quuxplusone added a subscriber: AlisdairM.

Rebased on master. @EricWF (cc @AlisdairM) ping!

Rebased on master. Added _NOEXCEPT to the resource() method (this is OK per [res.on.exception.handling]/5).

@ckennelly ping!