Re: [boost] [histogram] review part 2 (implementation)

Subject: Re: [boost] [histogram] review part 2 (implementation)
From: Hans Dembinski (hans.dembinski_at_[hidden])
Date: 2018-09-25 21:44:06

Hi Steven,

> On 25. Sep 2018, at 22:45, Steven Watanabe via Boost <boost_at_[hidden]> wrote:
> 
> AMDG
> 
> On 09/25/2018 01:34 PM, Hans Dembinski wrote:
>> 
>>> 128: if (Archive::is_loading::value) { this->~variable(); }
>>> Don't call the destructor like this.
>> 
>> How should I call it then?
>> 
> 
>  Don't.  You should only call the destructor
> explicitly if you intend to end the object's
> lifetime.  In particular, it should only be
> used for objects that were constructed with
> placement new.  The code has undefined behavior,
> because you are manipulating an object that
> does not exist (because it has been destroyed).
> Even if you ignore that problem, it's not exception-safe.

I googled a bit but couldn't find that there is anything special about calling a destructor compared to calling other methods.
https://stackoverflow.com/questions/14187006/is-calling-destructor-manually-always-a-sign-of-bad-design

I want to deallocate the memory of axis::variable here, so I call the destructor manually, which does that. The alternative is to either repeat the deallocation code in the serialize function (error prone), or move the deallocation code into another method which is then called by the destructor and the serialize function. I don't see the gain unless something special happens when you call destructors.

>> Yes, sizeof(detail::buffer<T>) < sizeof(std::vector<T>) and axis types should be as small as possible. We don't need growth, so there no need to store a capacity in addition to a size.
>> 
> 
> Why do the axis types need to be small?  Even if you
> waste a bit of memory, it should be insignificant
> compared to the actual data, right?  Also, if you're
> that concerned about memory usage, the label costs
> at least as much.

The histogram iterators over axis types a lot. If they are small they are more likely to fit onto a cache line. When you use a dynamic histogram, the size of axis::any<…> is size of largest bounded axis type plus some bytes for the type index. The largest bounded axis type determines the size of axis::any, so I was generally careful in making the larger axis types not larger than necessary.

True, labels have a size cost of several pointers, but having labels is more important than reducing the memory footprint, since they are very useful in any non-trivial program with more than one kind of histogram.

I had a special tiny_string implementation at some point with the size of one pointer. It even had small-string-optimisation, you can find it here:
https://github.com/HDembinski/stateful_pointer

Then I decided that adding a custom string type is overkill and used boost::container::string instead.

It is not overkill to use detail::buffer here instead of std::vector, since detail::buffer is anyway used by adaptive_storage. Replacing it here with std::vector does not make detail::buffer obsolete.

>>> detail/axes.hpp:
>>> 
>>> 521: optional_index call_impl(Tag, const std::tuple<T>& axes,
>>> It seems that this specialization prevents using a single
>>> element container with a single element static histogram,
>>> which seems inconsistent with the documentation and the
>>> behavior for dynamic histograms.
>> 
>> Correct, because this case is ambiguous. Let's say you have a 1d-histogram h1 and 2d-histogram h2. Both are dynamic histograms and you fill them.
>> 
>> h2(1, 2); // ok
>> h2(std::make_tuple(1, 2)); // ok, one argument must represent two values
>> h1(1); // ok
>> h1(std::make_tuple(1)); // ambiguous!
>> 
>> h1 cannot know at compile-time whether it should unpack the tuple or pass it to the axis unchanged. Perhaps you have a weird axis that accepts tuples as value_type.
>> 
>> I had to make a decision here and the most reasonable one is to pass single arguments of 1d histograms unchanged to the axis.
>> 
> 
> Shouldn't the same reasoning apply to dynamic histograms
> as well?   This definitely needs to be documented.  The
> documentation says that you can pass a container, without
> saying that one argument is an exception.

static and dynamic histograms have the exact same behaviour in this regard.
Agreed, this has to be documented.