libc++: First cut at <dynarray>

[Richard Smith]

On Mon, Sep 9, 2013 at 4:48 PM, Howard Hinnant <howard.hinnant at gmail.com>wrote:

> Thanks Marshall.  I've read Richard's excellent comments, and these are in
> addition to his.
>
> ...
>
> Marshall and I have already discussed this, but for the benefit of others:
>  I don't think dynarray should ever use the heap, and I don't want to
> commit even temporarily an implementation that does.


I don't think that's possible (and Nick has argued that it's not desirable,
because it will probably be a pessimization; I don't have data there).

Consider:

void g(std::dynarray<int> &);

void f() {
  std::dynarray<int> arr(10);
  g(arr);
  return arr[3];
}

// Some other TU:
void g(std::dynarray<int> &arr) {
  arr.~std::dynarray<int>();
  new (&arr) std::dynarray<int>(30);
}

I believe this code is valid, and we can't promote the allocation in 'f' to
the stack, nor the allocation in 'g'.

Also, if the dynarray object itself is on the heap, or in a global or
thread-local variable, we can't promote its allocation to the stack. And if
it's in a class type that's on the stack, we can only promote that if we
manage to inline the constructor and destructor (and we see that the
allocation doesn't escape).

You should think of putting the allocation on the stack as, at best, an
optimization, and should expect the allocation to go on the heap if we
cannot perform the optimization. FWIW, I'm not really sure why we have
std::dynarray, rather than an allocator for std::vector that encourages a
heap -> stack optimization.


> We need the stack support from the compiler prior to committing.  And that
> subsequently will probably require a __has_feature for the stack allocation
> support.
>
> On Sep 9, 2013, at 12:06 PM, Marshall Clow <mclow.lists at gmail.com> wrote:
>
> > <dynarray> See
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3662.html
> >
> > Open Issues:
> > 1) This includes <tuple> because that's where __uses_alloc_ctor lives.
> I'm thinking that __uses_alloc_ctor should be hoisted into a common header.
> Maybe __functional_base.
>
> I think <__functional_base> is a good suggestion.  However I started
> investigating this, and found it unnecessary.  <dynarray> already must
> include <algorithm> which includes <memory>, which includes <tuple>.  I
> think it is good that <dynarray> includes <tuple> to make it explicit.  But
> we aren't going to save anything by migrating __uses_alloc_ctor.
>
> That being said, see my comments below about Marshall's new
> __user_alloc_construct facility, which ought to be moved out of <dynarray>
> and packaged with __uses_alloc_ctor.  I contradict what I'm saying here,
> there. :-)
>
> > 2) This includes a general implementation of user-allocator construction
> (named __user_alloc_construct). This should probably be hoisted as well;
> and possibly the code in scoped_allocator can be reworked to use it (if it
> simplifies things).
>
> I like this.  Thanks for factoring it out.  Add "::" to the use of new:
>
>     ::new (__storage) _Tp (_VSTD::forward<_Args>(__args)...);
>
> Add #include <new>, though it is already included implicitly anyway.
>
> Hmm... logically this ought to go with __uses_alloc_ctor.  But it requires
> <new> and <__functional_base> does not have it.  I would rather not add
> <new> to <__functional_base>.  I would rather migrate
> __user_alloc_construct/__uses_alloc_ctor down the header dependency until
> <new> is available.
>
> However when I just now tried to do so, I created circular header
> dependencies.  After investigating further, adding <new> to
> <__functional_base> isn't so bad.  Everything <new> includes is already
> included by <__functional_base>.  So <__functional_base> only swells by the
> immediate contents of <new> which is pretty small.
>
> In summary, my suggestion is:
>
> *  Add #include <new> to <__functional_base>
>
> *  Move allocator_arg_t, __uses_alloc_ctor, __user_alloc_construct to
> <__functional_base>.
>
>
> >
> > 3) There's no support for stack allocations at present; that requires
> compiler support. I'm working with some people on getting that into clang.
>
> I'm guessing those people are cc'd here. :-)
>
> >
> > 4) There's an awful lot of duplication in the (many, many) constructors.
>
> Why not make base_ a _Tp* instead of a void*?  I think this will safely
> avoid some casting around.
>
> size_ and base_ need macro protection (leading "__").
>
> Need to add "inline" to each external definition marked with
> _LIBCPP_INLINE_VISIBILITY.  The client is allowed to #define
> _LIBCPP_INLINE_VISIBILITY, and we should go ahead and mark those functions
> with inline for those cases we think they are small enough to be inlined.
>  For those cases (and there may be some) where they are too big to be
> inlined, mark with neither inline nor _LIBCPP_INLINE_VISIBILITY.
>
> The use of uninitialized_fill_n and uninitialized_copy_n are too
> heavy-weight here.  We don't need or want to introduce the try/catch
> clauses associated with these functions.  Just copy the ::new-loops out of
> these functions and be sure to increment size_ on each iteration (as you've
> already correctly done elsewhere).
>
> Need a :: on the new in dynarray(size_type __c).
>
> __user_alloc_construct should use template argument deduction instead of
> explicit template arguments.
>
> __user_alloc_construct and __user_alloc_construct_impl needs decoration
> with inline _LIBCPP_INLINE_VISIBILITY.
>
> Case 0 of __user_alloc_construct_impl, remove unused argument name "__a".
>
> Need _LIBCPP_INLINE_VISIBILITY on the private default ctor, and on the
> private __allocate function.  Use of inline on the private __allocate is
> superfluous.
>
> I /think/ _LIBCPP_INLINE_VISIBILITY is unnecessary on the deleted copy
> assignment.
>
> The lack of a deallocation in ~dynarray() is giving me the heebie jeebies.
> ;-)  Hopefully this gets resolved when we move to stack allocation.
>
> You can use noexcept instead of _NOEXCEPT here as long as it stays under
> #if _LIBCPP_STD_VER > 11.  But I think this is nothing but an aesthetic
> comment.
>
> Also aesthetic:  Prefer use of value_type over T or _Tp when possible.
>
> Looking really good!  Looking forward to seeing the stack allocator.
>
> Thanks,
> Howard
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130909/3b1eb973/attachment.html>