[poly_collection] Andrzej's review

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[poly_collection] Andrzej's review

Boost - Dev mailing list
Hi Everyone,

Some comments from me about PolyCollection library, First, Joaquín, thank
you for writing and sharing this library.

I am not an an expert in the domain. In my programs I never needed this
optimization. But I have seen it described in a number of publications, and
I am convinced it deserves a place in Boost libraries, so that people are
not forced to reinvent it.

[1]

Now, because I have no practical experience with the problem, what I say in
this paragraph might be incorrect. But I have a problem here. You will use
this library for improved performance, maybe for a game implementation, yet
because a game needs super-performance, can a programmer afford to use
dynamic dispatch, or RTTI or exceptions? I always imagined taht such
library, rather than resembling `ptr_vector<I>` would resemble
`vector<variant<A, B, C>>`, that is: I decide and fix the container on a
set of types I will be storing inside. This set of tyes is embedded in the
type, so I can detect many mis-usages at compile time. The implementation
can be faster, because the number of segments is fixed (no segment
management), and no registration checks need to be performed. Then, iterate
over such collection with a static_visitor (from Boost.Variant). Of course,
this would be constraining: I need to know all the types I will be using
ahead of time. But maybe in practice this is often the case?

If you disagree, maybe it is wort discussing it in the rationale.

[2]

I am not comfortable with per-segment functions having the same name as
container-wide functions, and being only overloaded based on function or
template parameters, like `cc.size()` and `cc.size(index)`. These two
functions do different things, so they deserve different names. Maybe
`cc.segment_size(index)`, or `cc.serment(index).size()`?

[3]

I downloaded it and tried toy examples with GCC 6.3 on Windows with
-std=c++11, and clang 3.8.1 on Fedora with -std=c++11. It compiIes fine. I
observed that the following small program crashes (assertion fails):

```
#include <boost/poly_collection/base_collection.hpp>

struct Iface
{
    Iface() = default;
    virtual ~Iface() = 0;
};

inline Iface::~Iface() {}

struct Type1 : Iface
{
    Type1() = default;
    Type1(Type1&&) = delete;
};

struct Type2 : Iface
{
    Type2() = default;
    Type2(Type2&&) {} // throwing move
    // no move assignment
};

int main ()
try {
    boost::base_collection<Iface> c;
    c.insert(Type1{}); // fires an assert
    c.insert(Type2{}); // fires an assert
}
catch (std::exception const& e)
{
}
```

Admittedly, `Type1` and `Type2` are not "acceptable", but according to the
documentation this should throw an exception upon insertion rather than
firing assertions.

[4]

The documentation is good. As to implementation, I did not look at it in
detail.

Regards,
&rzej;

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [poly_collection] Andrzej's review

Boost - Dev mailing list
El 12/05/2017 a las 1:07, Andrzej Krzemienski via Boost escribió:
> Hi Everyone,
>
> Some comments from me about PolyCollection library, First, Joaquín, thank
> you for writing and sharing this library.

Thank you for your review!

> [...]
>
> [1]
>
> Now, because I have no practical experience with the problem, what I say in
> this paragraph might be incorrect. But I have a problem here. You will use
> this library for improved performance, maybe for a game implementation, yet
> because a game needs super-performance, can a programmer afford to use
> dynamic dispatch, or RTTI or exceptions? I always imagined taht such
> library, rather than resembling `ptr_vector<I>` would resemble
> `vector<variant<A, B, C>>`, that is: I decide and fix the container on a
> set of types I will be storing inside. This set of tyes is embedded in the
> type, so I can detect many mis-usages at compile time. The implementation
> can be faster, because the number of segments is fixed (no segment
> management), and no registration checks need to be performed. Then, iterate
> over such collection with a static_visitor (from Boost.Variant). Of course,
> this would be constraining: I need to know all the types I will be using
> ahead of time. But maybe in practice this is often the case?

I agree with you vector<variant<...>> should be faster than
ptr_vector<I>, but this
does not invalidate the lib rationale that replacing ptr_vector<I> with
base_collection<I>
will get you a speedup; of course the programmer has to decide on the
various alternatives
at their disposal, with different pros and cons. I don't know either if
vector<variant<...>>
is the "obvious" improvement over ptr_vector>I> to the extent that it
deserves
discussing/mentioning explicitly.

On a related note, some have asked for a variant_collection to be part of
Boost.PolyCollection roadmap.

> [...]
>
> [2]
>
> I am not comfortable with per-segment functions having the same name as
> container-wide functions, and being only overloaded based on function or
> template parameters, like `cc.size()` and `cc.size(index)`. These two
> functions do different things, so they deserve different names. Maybe
> `cc.segment_size(index)`, or `cc.serment(index).size()`?

On the contrary, I like name overloading better, because it looks terser
yet sufficiently
expressive:

   cc.size<warrior>(); // what else but the numer of warriors in the
collection?

Furthermore, there are member functions such as

   cc.erase(begin<warrior>());

that, according to your suggestion, should be renamed as (IMHO very ugly)

   cc.segment_erase(begin<warrior>());

and if we decide that this latter example should be exempt from the segment_
prefix, then what's the rationale for what gets segment_ and what does not?

> [3]
>
> I downloaded it and tried toy examples with GCC 6.3 on Windows with
> -std=c++11, and clang 3.8.1 on Fedora with -std=c++11. It compiIes fine. I
> observed that the following small program crashes (assertion fails):
>
> ```
> #include <boost/poly_collection/base_collection.hpp>
>
> struct Iface
> {
>      Iface() = default;
>      virtual ~Iface() = 0;
> };
>
> inline Iface::~Iface() {}
>
> struct Type1 : Iface
> {
>      Type1() = default;
>      Type1(Type1&&) = delete;
> };
>
> struct Type2 : Iface
> {
>      Type2() = default;
>      Type2(Type2&&) {} // throwing move
>      // no move assignment
> };
>
> int main ()
> try {
>      boost::base_collection<Iface> c;
>      c.insert(Type1{}); // fires an assert
>      c.insert(Type2{}); // fires an assert
> }
> catch (std::exception const& e)
> {
> }
> ```
>
> Admittedly, `Type1` and `Type2` are not "acceptable", but according to the
> documentation this should throw an exception upon insertion rather than
> firing assertions.

Umm... Yes, you're right, I think the assertion in

https://github.com/joaquintides/poly_collection/blob/master/include/boost/poly_collection/detail/poly_collection.hpp#L945

should be removed. Let me study it carefully. Thanks for spotting this.

Joaquín M López Muñoz

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [poly_collection] Andrzej's review

Boost - Dev mailing list
2017-05-12 8:13 GMT+02:00 Joaquin M López Muñoz via Boost <
[hidden email]>:

> El 12/05/2017 a las 1:07, Andrzej Krzemienski via Boost escribió:
>
>> Hi Everyone,
>>
>> Some comments from me about PolyCollection library, First, Joaquín, thank
>> you for writing and sharing this library.
>>
>
> Thank you for your review!
>
> [...]
>>
>> [1]
>>
>> Now, because I have no practical experience with the problem, what I say
>> in
>> this paragraph might be incorrect. But I have a problem here. You will use
>> this library for improved performance, maybe for a game implementation,
>> yet
>> because a game needs super-performance, can a programmer afford to use
>> dynamic dispatch, or RTTI or exceptions? I always imagined taht such
>> library, rather than resembling `ptr_vector<I>` would resemble
>> `vector<variant<A, B, C>>`, that is: I decide and fix the container on a
>> set of types I will be storing inside. This set of tyes is embedded in the
>> type, so I can detect many mis-usages at compile time. The implementation
>> can be faster, because the number of segments is fixed (no segment
>> management), and no registration checks need to be performed. Then,
>> iterate
>> over such collection with a static_visitor (from Boost.Variant). Of
>> course,
>> this would be constraining: I need to know all the types I will be using
>> ahead of time. But maybe in practice this is often the case?
>>
>
> I agree with you vector<variant<...>> should be faster than ptr_vector<I>,
> but this
> does not invalidate the lib rationale that replacing ptr_vector<I> with
> base_collection<I>
> will get you a speedup; of course the programmer has to decide on the
> various alternatives
> at their disposal, with different pros and cons. I don't know either if
> vector<variant<...>>
> is the "obvious" improvement over ptr_vector>I> to the extent that it
> deserves
> discussing/mentioning explicitly.
>

Just to make my remark clear, I am not saying one should use
vector<variant<A, B, C>> instead of ptr_vector<> for performance. Based on
my experience OO-inheritance solutions are faster that using `variant`. As
you say below, I was suggesting the alternative design:
`variant_collection`.


>
> On a related note, some have asked for a variant_collection to be part of
> Boost.PolyCollection roadmap.
>
> [...]
>>
>> [2]
>>
>> I am not comfortable with per-segment functions having the same name as
>> container-wide functions, and being only overloaded based on function or
>> template parameters, like `cc.size()` and `cc.size(index)`. These two
>> functions do different things, so they deserve different names. Maybe
>> `cc.segment_size(index)`, or `cc.serment(index).size()`?
>>
>
> On the contrary, I like name overloading better, because it looks terser
> yet sufficiently
> expressive:
>
>   cc.size<warrior>(); // what else but the numer of warriors in the
> collection?
>
> Furthermore, there are member functions such as
>
>   cc.erase(begin<warrior>());
>
> that, according to your suggestion, should be renamed as (IMHO very ugly)
>
>   cc.segment_erase(begin<warrior>());
>

or:

cc.segment<warrior>().clear();

Assuming that I understand what the former notation does.


> and if we decide that this latter example should be exempt from the
> segment_
> prefix, then what's the rationale for what gets segment_ and what does not?


I can see the problem you are describing. Also I agree with you that in the
examples above, using the overload is not ambiguous to the users.

On the other hand, I was encountering problems when trying to pass member
function names as parameters to to other functions. When function `size()`
is overloaded, I cannot easily pass its address to another function:

some_for_each(&T:: size); // ambiguous: which overload

But maybe it is just a bikeshed discussion. The library is good with either
choice of names.

Regards,
&rzej;

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost