Re: [boost] [auto_buffer] Interest check: container with preallocated buffer

Subject: Re: [boost] [auto_buffer] Interest check: container with preallocated buffer
From: Stewart, Robert (Robert.Stewart_at_[hidden])
Date: 2009-05-19 17:00:32

Thorsten Ottosen wrote
On Tuesday, May 19, 2009 4:41 PM
> Stewart, Robert skrev:
> > Thorsten Ottosen wrote
> > On Tuesday, May 19, 2009 2:17 PM
> >> Stewart, Robert skrev:
>
> > Is the signature actually sufficient?  What if the
> > requested size exceeds the adjustment new_capacity() would make?
>
> >IOW, what if the requested size is greater than four times
> > the old capacity?  Would this be better?
> >
> >    template <class Size>
> >    static Size
> >    grow(Size _old, Size _size);
> >
> > This means supplying the old capacity and desired size such
> > that the result should be at least _size.
>
> In that case the exact requested size is allocated. The interface you
> show provides some extra flexibility, but I don't know if it
> is useful.
> Perhaps.

I haven't looked at your implementation of reserve(), but I was expecting that it would simply ask the policy for the new size and allocate it.  Apparently, you're making decisions in reserve() over and above those made by new_capacity().  I think that will be surprising to those writing the policy.

> >>> The "GrowPolicy" template parameter should be named
> "GrowthPolicy."
> >> Really?
> >
> > Yes, "growth" is a noun.
>
> How about ResizingPolicy? The policy is also used in shrink_to_fit().

Good

> >>> The auto_buffer nested value "N" is confusing as it will
> >>> change based upon the SizingPolicy,
> >>> but won't match that policy's "value" if using
> >>> boost::store_n_bytes, and it isn't the number of elements in,
> >>> or capacity of, the auto_buffer, particularly when memory is
> >>> allocated from the allocator.
> >>> Renaming it to "stack_capacity" should work, though.
> >> I'll think about it. I need a short name for the docs, and N
> >> seems both
> >> short and good to me. N is also documented.
> >
> > "N" suggests number of elements to me.
>
> Right. And that is what it is.

Isn't it just the number of elements that will fit within the stack allocation?  When the buffer switches to allocator-supplied memory, N is no longer likely to be the number of elements -- which is why you provide size().

> > Why would anyone bother to create one that won't fit on the
> > stack?  Why not just use vector?
>
> Because auto_buffer provides additional functionality not found in
> std::vector.
>
> > Are you thinking of generic code that sometimes will succeed
> > in using the stack and other times not?
>
> I don't know what you mean by this, but I'm happy if there is yet
> another reason to use the class.

I meant a function template that creates an auto_buffer<T,store_n_bytes<N>> where sizeof(T) may be greater than N, depending upon T.

> >>> How about that store_n_bytes<N>::value >= sizeof(T)?  Those
> >>> will help to diagnose misuse.
> >> This might be ok to have if  0 < N < sizeof(T).
> >
> > I suppose this can occur for the same reason as above but
> > it should be documented.  Otherwise, there's no sense in allowing it.
>
> OTOH, I would hate to put a static assertion in there if it sometimes
> complicates the use when the user actually don't mind this.

It may be worth mentioning the relationship between N and sizeof(T) when using store_n_bytes as it can lead to not using the stack space.

> >>> For small types, optimized_const_reference isn't a reference
> >>> at all, so the name is misleading.
> >>> Better to name it for the usage rather than the type.
> >> But then for large types, "by_value_type" is not a value
> >> type, but a
> >> reference. What about parameter_type or param_type?
> >
> > Yeah, it doesn't work well for the other case.  "Parameter"
> > suggests only the input side of things.  boost::call_traits
> > does something similar, but that only has to do with calling
> > functions.  It would be odd to return a call_traits type.
> > How about "fast_value_type?"
>
> It's a candidate, but again, sometimes it is a reference :-)

It is being used as a value.  You're returning a T, but you're using a reference to const to avoid copying.  "fast_value_type" refers to the usage rather than the type.

> >>> Rather than reserve_precisely(), which should be named
> >>> "reserve_exactly" if retained,
> >>> why not just document reserve() as allocating what
> >>> GrowthPolicy::reserve() returns and leave the behavior to the
> >>> policy?
> >>> That leaves room for reserve() allocating exactly what's
> >>> requested, or rounding up according to some computation, as
> >>> the policy dictates.
> >> reserve() is used several places when the buffer needs to
> >> grow. If this
> >> was to allocate precisely what was requested, then we would e.g.
> >> allocate just one extra element.
> >>
> >> But I guess I could factor the code so it was as you
> >> suggested. I will
> >> make this a review question. This will require a small change in
> >> GrowPolicy so you can see what is requested.
> >
> > I think the "see what is requested" is the change I suggested above.
>
> Not quite. reserve_precisely() allows you to do extractly that, not
> matter what GrowPolicy you have installed. The user knows that the
> new buffer should be exactly what he asks for, no matter what the
> policy thinks.

OK, but if you remove reserve_precisely(), as I suggested, then you're left with reserve() and it will behave as dictated by the policy.  Whenever I use reserve(), I only want that many elements, not at least that many, but I understand that when auto_buffer grows implicitly, it wants to allocate something extra to get amortized constant time for the increase.  Thus, in my mind, reserve() does exactly what the caller asks, but auto_buffer itself uses a private member function that calls new_capacity() and moves the data to a new allocation.

> >>> The unchecked_*() functions deserve remarks that explain
> >>> the danger in their use and that the precondition is not checked.
> >
> > You didn't comment on the addition of remarks for these
> functions, so I'm re-raising this point.
>
> The precondition is checked with BOOST_ASSERT(). As for
> remarks, yeah,
> but how many other functions in Boost have a remark saying that you
> should not violate the preconditions?

OK, but you can still document the danger.

> >>> shrink_to_stack() might be a useful addition.
> >> shrink_to_fit() can already place the content on the stack.
> >
> > OK.  I assume that shrink_to_fit() takes an argument, so
> that shrink_to_stack() would be shorthand for
> a.shrink_to_fit(a.stack_capacity).
>
> No, shrink_to_fit() never erases elements from the container
> (logically). resize() does, but this is on my todo list.
> resize() would
> probably not move elements to the stack.

Then shrink_to_stack() would be a nice addition for which shrink_to_fit() is not a replacement.

_____
Rob Stewart                           robert.stewart_at_[hidden]
Software Engineer, Core Software      using std::disclaimer;
Susquehanna International Group, LLP  http://www.sig.com

IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.