Re: [boost] [Review] Shmem

From: Caleb Epstein (caleb.epstein_at_[hidden])
Date: 2006-02-08 12:48:15

On 2/6/06, Fred Bertsch <fred.bertsch_at_[hidden]> wrote:

Please always state in your review, whether you think the library should be
> accepted as a Boost library!

An enthusiastic YES.

Additionally please consider giving feedback on the following general
> topics:
>
> - What is your evaluation of the design?

The design is very good.

> - What is your evaluation of the implementation?

I agree with others that there should be RAII versions of the constructors
of various classes like shared_memory and named_shared_object that throw if
they fail.

Another concern, addressed clearly in the documentation, is the necessity of
reproducing the synchronization primitives of Boost.Thread and the xtime
class in Shmem.  This is yet another case where a library that tries to be
thread-aware, without necessarily being multithreaded itself, has had to do
this.  From what I can gather from some grepping,
Boost.Pool(pool/detail/mutex.hpp) and
Boost.Regex (regex/pending/static_mutex.hpp) contains their own mutex
implementation as well.  Also, the proposed Logging library (which was
pulled before the review was complete) and ASIO both contain some elemental
synchronization classes that could be a more fundamental part of Boost.

I think it is high time for the separation of the synchronization primitives
of Boost.Thread into a separate library before they are duplicated again.
This duplication has the follow-on issues of reduced portability for the
library that reproduces them and the possibility of bugs being introduced.

I'm also intrigued by the reproduction of many Standard Library containers
due to implementation restrictions (under Current Limitations, "Problems
with most STL implementations").  Ion, can you include a listing of
platforms for which the Shmem versions of containers must be used and ones
where the Standard Library containers can be used with the Shmem allocator?
Are there any of the latter?  A table would be quite useful here.  For
compilers with a reasonable hope of getting changes fed into the Standard
Library implementation (e.g. gcc) it might be worthwhile to contact the
maintainers about these limitations to see if they can be removed.

- What is your evaluation of the documentation?

Very good and quite complete.  Some minor questions/quibbles:

>From Current Limitations / "Be careful with static class members":

"Static members are not dangerous if they are just constant variables
initialized when the process starts, but they don't change at all (for
example, when used like enums)."

I'm not sure I understand the second half of this sentence.  What does it
mean to use a static variable like an enum?  Maybe I'm just being
thick-headed.

The Introduction states:

"Shmem also offers the basic_string pseudo-container to use full-powered C++
strings in shared memory."

But this class isn't documented.  I see it in the header
<boost/shmem/containers/string.hpp>.  Actually it appears that none of the
headers in the shmem/containers dir are mentioned explicitly in the
documentation.  They are touched on briefly in "Shmem and containers in
shared memory", but this documentation might benefit from some expansion.

- What is your evaluation of the potential usefulness of the library?

I think it has great potential, especially if the memory segments can be
made to grow (fingers crossed!).

- Did you try to use the library? With what compiler? Did you have any
> problems?

I didn't actually use the library other than to compile and run the tests
w/gcc 3.3.4 on Linux.  I did run into one error trying to compile the tests
with a CVS version of Boost from this morning:

    "g++"   -c  -DBOOST_ALL_NO_LIB=1  -g -O0 -fno-inline -pthread -Wall
-ftemplate-depth-255  -I"../../../bin/boost/libs/shmem/test"   -I
"/home/nbde52d/src/boost-regression/boost"  -o
"../../../bin/boost/libs/shmem/test/private_node_allocator_test.test/gcc/debug/threading-multi/private_node_allocator_test.o"
"private_node_allocator_test.cpp"
    "/usr/bin/objcopy" --set-section-flags .debug_str=contents,debug
"../../../bin/boost/libs/shmem/test/private_node_allocator_test.test/gcc/debug/threading-multi/private_node_allocator_test.o"

/home/nbde52d/src/boost-regression/boost/boost/shmem/allocators/private_node_allocator.hpp:
In
   function `void
boost::shmem::swap(boost::shmem::private_node_allocator<boost::shmem::detail::shmem_list_node<priv_node_allocator_t>,
   64, boost::shmem::detail::segment_manager<wchar_t,
   boost::shmem::simple_seq_fit<boost::shmem::shared_mutex_family,
   boost::shmem::offset_ptr<void, boost::shmem::offset_1_null_ptr> >,
   boost::shmem::flat_map_index> >&,

boost::shmem::private_node_allocator<boost::shmem::detail::shmem_list_node<priv_node_allocator_t>,
   64, boost::shmem::detail::segment_manager<wchar_t,
   boost::shmem::simple_seq_fit<boost::shmem::shared_mutex_family,
   boost::shmem::offset_ptr<void, boost::shmem::offset_1_null_ptr> >,
   boost::shmem::flat_map_index> >&)':
/home/nbde52d/src/boost-regression/boost/boost/shmem/detail/utilities.hpp:45:
instantiated from `void boost::shmem::detail::swap(T&, T&) [with T =
boost::shmem::private_node_allocator<boost::shmem::detail::shmem_list_node<priv_node_allocator_t>,
64, boost::shmem::detail::segment_manager<wchar_t,
boost::shmem::simple_seq_fit<boost::shmem::shared_mutex_family,
boost::shmem::offset_ptr<void, boost::shmem::offset_1_null_ptr> >,
boost::shmem::flat_map_index> >]'
/home/nbde52d/src/boost-regression/boost/boost/shmem/containers/list.hpp:317:
instantiated from `void boost::shmem::detail::shmem_list_alloc<T, A,
true>::swap(boost::shmem::detail::shmem_list_alloc<T, A, true>&) [with T =
int, A = priv_node_allocator_t]'
/home/nbde52d/src/boost-regression/boost/boost/shmem/containers/list.hpp:598:
instantiated from `void boost::shmem::list<T, A>::swap(boost::shmem::list<T,
A>&) [with T = int, A = priv_node_allocator_t]'
/home/nbde52d/src/boost-regression/boost/boost/shmem/containers/list.hpp:792:
instantiated from `void boost::shmem::list<T, A>::sort(StrictWeakOrdering)
[with StrictWeakOrdering = boost::shmem::list<int,
priv_node_allocator_t>::value_less, T = int, A = priv_node_allocator_t]'
/home/nbde52d/src/boost-regression/boost/boost/shmem/containers/list.hpp:774:
instantiated from `void boost::shmem::list<T, A>::sort() [with T = int, A =
priv_node_allocator_t]'
private_node_allocator_test.cpp:95:   instantiated from here
/home/nbde52d/src/boost-regression/boost/boost/shmem/allocators/private_node_allocator.hpp:198:
error: call
   of overloaded `swap(
   boost::shmem::offset_ptr<boost::shmem::detail::segment_manager<wchar_t,
   boost::shmem::simple_seq_fit<boost::shmem::shared_mutex_family,
   boost::shmem::offset_ptr<void, boost::shmem::offset_1_null_ptr> >,
   boost::shmem::flat_map_index>, boost::shmem::offset_1_null_ptr>&,
   boost::shmem::offset_ptr<boost::shmem::detail::segment_manager<wchar_t,
   boost::shmem::simple_seq_fit<boost::shmem::shared_mutex_family,
   boost::shmem::offset_ptr<void, boost::shmem::offset_1_null_ptr> >,
   boost::shmem::flat_map_index>, boost::shmem::offset_1_null_ptr>&)' is
   ambiguous
/home/ietdev/tools/linux-i686/gcc-3.3.4/include/c++/3.3.4/bits/stl_algobase.h:121:
error: candidates
   are: void std::swap(_Tp&, _Tp&) [with _Tp =
   boost::shmem::offset_ptr<boost::shmem::detail::segment_manager<wchar_t,
   boost::shmem::simple_seq_fit<boost::shmem::shared_mutex_family,
   boost::shmem::offset_ptr<void, boost::shmem::offset_1_null_ptr> >,
   boost::shmem::flat_map_index>, boost::shmem::offset_1_null_ptr>]
/home/nbde52d/src/boost-regression/boost/boost/shmem/detail/utilities.hpp:43:
error:
                  void boost::shmem::detail::swap(T&, T&) [with T =
   boost::shmem::offset_ptr<boost::shmem::detail::segment_manager<wchar_t,
   boost::shmem::simple_seq_fit<boost::shmem::shared_mutex_family,
   boost::shmem::offset_ptr<void, boost::shmem::offset_1_null_ptr> >,
   boost::shmem::flat_map_index>, boost::shmem::offset_1_null_ptr>]

- How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?

I've read the documentation going back to the Ion's version 0.3 and re-read
it from the Review Materials.  Overall, a few hours of reading and thinking.

- Are you knowledgeable about the problem domain?

A little bit.

--
Caleb Epstein
caleb dot epstein at gmail dot com