Page MenuHomePhabricator

<experimental/memory_resource>: Implement {un,}synchronized_pool_resource.
Needs ReviewPublic

Authored by Quuxplusone on May 24 2018, 10:33 PM.

Details

Summary

Diff Detail

Repository
rCXX libc++

Event Timeline

Quuxplusone created this revision.May 24 2018, 10:33 PM

Rebase and update the diff.

Quuxplusone added inline comments.May 29 2018, 5:17 PM
test/std/experimental/memory/memory.resource.pool/pool_options.pass.cpp
12

This test comes from Eric's D27402. The default-initialization of a const pool_options object actually depends on a DR, so I needed to add UNSUPPORTED: apple-clang-7. I don't know how to determine the list of all unsupporty targets.

Quuxplusone updated this revision to Diff 149611.EditedJun 2 2018, 8:51 AM
  • Split up the unit tests.
  • Refactor to shrink the memory layout of the resource object itself. Before this patch sizeof(unsynchronized_pool_resource)==56. After this patch sizeof(unsynchronized_pool_resource) == 40.
Quuxplusone planned changes to this revision.Jun 18 2018, 7:02 PM
Quuxplusone added inline comments.
src/experimental/memory_resource.cpp
232

I now see how to avoid doing this: instead of keeping a separate free-list per each chunk, I should keep one big free-list per pool, where blocks from all chunks in the same pool are mixed together. This is okay because we never deallocate one chunk unless we're deallocating *all* chunks.
I'll rework the patch to eliminate this undefined behavior and keep a free-list per pool.

Bugfix and shrink {un,}synchronized_pool_resource.

The old implementation was severely broken in two ways:

  • It accidentally sometimes trusted the user's bytes and align arguments to do_deallocate, which didn't match the bytes and alignment stored in the actual ad-hoc chunk header. The new test case catches this bug.
  • It relied on pointer comparison (undefined behavior) to test whether the user-supplied pointer belonged to each chunk in turn. Fortunately this is unnecessary. The new code follows Boost.Container by keeping just one list of "vacancies" per pool (not one per chunk, as I'd been doing). The new code is faster (deallocation is now O(1) not O(chunks-in-this-pool)), smaller (there are O(pools) free-list pointers instead of O(chunks)), and avoids undefined behavior.
Quuxplusone marked an inline comment as done.Jun 19 2018, 3:18 PM

Implement similar cosmetic cleanup to D47111, but for the pool resources this time.
I think the argument for keeping do_allocate and do_deallocate in the .cpp file is stronger for these guys than for monotonic_buffer_resource (where Eric convinced me to move them into the .h file for the benefit of the inliner). However, I'm willing to be persuaded (as I was in that case).

Rebased on master. @EricWF @ldionne ping please?

Change 1 << to size_t(1) << in one last place. @EricWF ping.

Quuxplusone added a subscriber: AlisdairM.

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

Rebased. Added _NOEXCEPT to upstream_resource() and options() (this is OK per [res.on.exception.handling]/5).

Rebased on D47111.
Replaced all instances of __release with __release_ptr, to match what @EricWF did in https://github.com/llvm-mirror/libcxx/commit/8e365750a0299ee0cbef70a3f3492c7716f81d5b under the commit message "Avoid name conflict with kernel headers."

Rebased. Added _NOEXCEPT to upstream_resource() and options() (this is OK per [res.on.exception.handling]/5).

That's fine, but then we should have a test for that.
We have the macro LIBCPP_ASSERT_NOEXCEPT specifically for this purpose (libc++ - specific noexcept markings).

Add tests that LIBCPP_ASSERT_NOEXCEPT the options and upstream_resource accessors.