Re: [boost] [review] The review of Boost.DoubleEnded starts today: September 21 - September 30

Subject: Re: [boost] [review] The review of Boost.DoubleEnded starts today: September 21 - September 30
From: Thorsten Ottosen (tottosen_at_[hidden])
Date: 2017-09-26 17:14:10

Hi Tim,

> Just some comments on devector for now, based on a quick read of the
> documentation and implementation.
> 
> Design:
> 
> I don't find the unsafe functions sufficiently motivated. Introducing
> footguns should require a compelling performance justification, but
> there are no actual benchmarks. The unsafe_push_* functions encourage
> heavy uses of reserve(), which, if done incorrectly, can easily
> prevent the growth policy from working.  Moreover, the design is
> inconsistent. Why unsafe_push_* but not unsafe_emplace_*?

The missing _emplace functions is surely an oversight.

I the thread with Zach's review I tried to view this from a more 
philosophical viewpoint. What is safe and unsafe is highly subjective 
(and as I wrote in the other thread, it is a mistake to use the word 
"unsafe" in the function name").
But C++ is not memory safe like Java or C#, and there are tons of 
preconditions in C++ code that are only checked with debug assertions. 
Yet I don't think in general that C++ programs have more bugs than Java 
or C# programs. Take a look at various Boost libraries and they offer 
plenty of opportunity to shoot yourself in the foot. But we use them 
anyway because they offer functionality and performance that cannot be 
found in any other way. Said differently: all the alternatives are 
worse. As just one example: I can use Boost.Intrusive and it is surely 
harder to use than normal containers, but the performance is awesome and 
compared with all the hand-rolled, ad hoc alternatives, it's vastly 
superior.

So it's easy to say something is hard and unsafe to use. But what about 
the alternative?

I have personally been optimizing stuff that fell back on vector's 
push_back was not inlined. So I had to rewrite the code, effectively 
making an ad hoc container just so I could add elements to it 
efficiently in the loop that controlled the general performance of the 
program.

> What's the motivating use case for the unsafe_uninitialized functions
> that can't handled by a non-invariant-violating design like
> default_init? The sole example in the documentation is a
> devector<char> whose content will be filled by recv later, which can
> be handled equally well. 

Can you elaborate on what this design is exactly? Thanks.

> Also, assuming that this container meets the
> allocator-aware container requirements, the user would have to
> construct and destroy the elements via
> allocator_traits<>::construct/destroy, not just placement new.
> Otherwise, your container's destructor would be asking its allocator
> to destroy something it didn't construct.

Indeed. If the documentation doesn't say so, it needs to be updated.

> I'm somewhat surprised that dropping the move_if_noexcept/strong
> exception safety dance doesn't seem to have been considered for a
> container that's supposed to be performance-oriented.

For which container operation are you thinking about?

> The default growth policy is:
> 
>> static size_type new_capacity(size_type capacity);
>> Returns: 4 times the old capacity or 16 if it's 0.
> 
> Why those numbers? Were they arbitrarily chosen or based on some sort
> of benchmark? If so, what?

In this connection,

https://github.com/facebook/folly/blob/master/folly/docs/FBVector.md

may be relevant. It argues for a 1.5 growth factor.

kind regards

Thorsten