This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Fix stack_allocator
ClosedPublic

Authored by EricWF on Oct 1 2016, 12:06 AM.

Details

Summary

To quote STL the problems with stack allocator are"

"stack_allocator<T, N> is seriously nonconformant to N4582 17.6.3.5 [allocator.requirements].
First, it lacks a rebinding constructor. (The nested "struct rebind" isn't sufficient.)
Second, it lacks templated equality/inequality.
Third, it completely ignores alignment.
Finally, and most severely, the Standard forbids its existence. Allocators are forbidden from returning memory "inside themselves". This requirement is implied by the Standard's requirements for rebinding and equality. It's permitted to return memory from a separate buffer object on the stack, though."

This patch attempts to address all of those issues.

First, instead of storing the buffer inside the allocator I've change stack_allocator to accept the buffer as an argument.

Second, in order to fix rebinding I changed the parameter list from <class T, size_t NumElements> to <class T, size_t NumBytes>. This allows allocator rebinding
between types that have different sizes.

Third, I added copy and rebinding constructors and assignment operators.

And finally I fixed the allocation logic to always return properly aligned storage.

Diff Detail

Event Timeline

EricWF updated this revision to Diff 73188.Oct 1 2016, 12:06 AM
EricWF retitled this revision from to [libc++] Fix stack_allocator.
EricWF updated this object.
EricWF added a subscriber: cfe-commits.
STL_MSFT edited edge metadata.Oct 3 2016, 2:59 PM

stack_allocator.h is inconsistent about saying plain size_t or std::size_t.

stack_allocator.h should include type_traits for integral_constant.

stack_allocator.h refers to stack_allocator as both a class and a struct. It should be consistent (not just because MSVC complains).

stack_allocator provides copy assign and convert copy assign. This is unnecessary because it doesn't set any of POCCA/POCMA/POCS. It should probably be non-assignable.

For the same reason, stack_allocator probably shouldn't have a do-nothing swap() overload. It won't be used.

In C++11 land, stack_allocator::allocate() doesn't need "const void* = 0" (because allocator_traits will fill it in). Don't know if you need this for C++03 mode.

stack_allocator equality/inequality probably shouldn't be templated on differing TN/UN, as they aren't rebindable that way.

Why does test_stack_allocator.pass.cpp line 11 have a commented-out path?

There is occasional whitespace damage caused by search-and-replace, e.g. "std::list<int, AllocT >".

Why is test/std/containers/sequences/deque/deque.cons/size_value_alloc.pass.cpp 84-85 gaining totally empty braces?

EricWF added a comment.Oct 7 2016, 3:11 PM

Why does test_stack_allocator.pass.cpp line 11 have a commented-out path?

Typically tests name the header they test in a comment.

Why is test/std/containers/sequences/deque/deque.cons/size_value_alloc.pass.cpp 84-85 gaining totally empty braces?

Mass-translation fatigue. I've removed the empty braces.

All of the comments have been addressed.

EricWF updated this revision to Diff 73995.Oct 7 2016, 3:12 PM
EricWF edited edge metadata.

Address @STL_MSFT's comments.

STL_MSFT accepted this revision.Oct 7 2016, 3:16 PM
STL_MSFT edited edge metadata.

Seems plausible to me. I haven't tested this against MSVC yet, but at most I expect to have minor issues with space consumption (as we allocate helper objects and sentinel nodes, so any exactly-sized buffers may run out of room). In any event the existing stuff is totally broken, so I have no objections to you checking this in. Thanks!

This revision is now accepted and ready to land.Oct 7 2016, 3:16 PM
EricWF added a comment.Oct 7 2016, 3:18 PM

Seems plausible to me. I haven't tested this against MSVC yet, but at most I expect to have minor issues with space consumption (as we allocate helper objects and sentinel nodes, so any exactly-sized buffers may run out of room). In any event the existing stuff is totally broken, so I have no objections to you checking this in. Thanks!

Please let me know if any of your tests remain broken, so I can fix this further.

  • The attached patch on top of your patch fixes two simple MSVC warnings - unreferenced formal parameter and class/struct mismatch.
  • Three tests are emitting stack consumption warnings (since MSVC /analyze tries to understand what's going on in the stack):

test\std\containers\sequences\deque\deque.cons\iter_iter.pass.cpp
iter_iter.pass.cpp(50) : warning C6262: Function uses '32868' bytes of stack: exceeds /analyze:stacksize '16384'. Consider moving some data to heap.

test\std\containers\sequences\deque\deque.cons\size.pass.cpp
size.pass.cpp(85) : warning C6262: Function uses '800032' bytes of stack: exceeds /analyze:stacksize '16384'. Consider moving some data to heap.

test\std\containers\sequences\deque\deque.cons\size_value_alloc.pass.cpp
size_value_alloc.pass.cpp(34) : warning C6262: Function uses '80044' bytes of stack: exceeds /analyze:stacksize '16384'. Consider moving some data to heap.

The 800KB consumption seems especially egregious. Changing these tests to dynamically allocate these huge objects will silence the warning.

  • Finally, the following tests are asserting:

test\std\containers\sequences\vector\vector.capacity\reserve.pass.cpp
test\std\containers\sequences\vector\vector.capacity\resize_size.pass.cpp
test\std\containers\sequences\vector\vector.capacity\resize_size_value.pass.cpp
test\std\containers\sequences\vector\vector.cons\construct_iter_iter_alloc.pass.cpp
test\std\containers\sequences\vector\vector.cons\construct_size_value_alloc.pass.cpp
test\std\containers\sequences\vector\vector.modifiers\push_back.pass.cpp
test\std\containers\sequences\vector\vector.modifiers\push_back_rvalue.pass.cpp

This is because of the exact-space-consumption issue I feared/predicted - you're constructing stack_allocators with a small fixed amount of space, but our debug object occupies space, and that makes the allocator run out of memory.

A possible fix would be to increase the amount of space (two pointers should be sufficient).

EricWF updated this revision to Diff 74006.Oct 7 2016, 6:04 PM
EricWF edited edge metadata.

Rewrite the patch entirely. Instead of using an actual "stack buffer" I've transformed stack_allocator into limited_allocator<T, N> which allows at most N elements to be allocated from it. This resolves the issues about stack usage but it doesn't help with the MSVC debug extra allocations.

EricWF closed this revision.Oct 7 2016, 6:05 PM
test/std/containers/sequences/vector/vector.modifiers/push_back_rvalue.pass.cpp