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
235

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