[boost] [review] shmem

From: Kim Barrett (kab_at_[hidden])
Date: 2006-02-15 20:37:10

Review of proposed Boost.Shmem library
Kim Barrett <kab_at_[hidden]>

[Not as thorough as I'd hoped, running out of time for now, maybe more later.]

==============================================================================

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

Yes.

==============================================================================

Additionally please consider giving feedback on the following
general topics:

------------------------------------------------------------------------------
- What is your evaluation of the design?

Generally good. The biggest problem is name choices, which are
quite confusing in some cases. This needs some explicit discussion
or alternative suggestions to help the author in this area.

------------------------------------------------------------------------------
- What is your evaluation of the implementation?

Generally seems quite good. I've reported a couple of bugs against
previous versions, which I expect have been fixed. In my use of
this library it has pretty much just worked. Given the nasty kinds
of bugs one can run into in this area, that's really appreciated.

------------------------------------------------------------------------------
- What is your evaluation of the documentation?

This is the biggest weakness of this library. Some of the material
is very dense, and could probably use some expansion. But more
importantly, there are a number of things for which the
documentation is missing. In order to use this library I had to
look at the source code in many cases. At least some of this may be
just documentation generation problems, but I'm not sure that's all
of it. I really wish those kinds of issues had been ironed out
prior to the official review, so that we could be reviewing the
documentation the author would actually like us to have. Also note
that I've only been looking at the html documentation; I don't know
if the other documentation format is any different.

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

Highly useful for some problems. The existence of this library has
saved me weeks of work on a very limited subset of the provided
functionality. And that's before the effort of porting to other
platforms!

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

I am actively using this library as part of my current project at
work. Target platforms include

- gcc3.3 on linux-x86, where the library has worked fine

- gcc3.2 on linux-ppc, where we ran into some minor problems
(described in detail below) due to some "advanced" C++ idioms with
which this compiler version couldn't cope. We were able to work
around these fairly easily by making minor patches to the library
source. Hopefully we will soon be upgrading to a more recent
compiler version for this target platform, and we won't care about
this problem any more, though others might.

- gcc4.0 on MacOSX, where we ran into problems because the xtime
support copied from boost.threads is depending on the optional
POSIX clock_gettime interface, which MacOSX doesn't support. Since
boost.threads seems to work on MacOSX, perhaps the fork used by
this library is not up to date?

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

In-depth study of both the documentation and parts of the
implementation.

------------------------------------------------------------------------------
- Are you knowledgeable about the problem domain?

Yes and no. Before I started working with this library I was aware
of the existence of the allocator part of the standard library, but
had never actually looked at it. Understanding allocators is
important for significant use of this library, so I was starting
pretty much cold there, though I'm very familiar with memory
management in general. I've done a substantial amount of
programming that used posix shared memory, so was already familiar
with the concepts, and especially the restrictions involved.

==============================================================================

Specific comments on the library. Note that I've had private
discussions with the library author about some of these prior to
the boost the review.

------------------------------------------------------------------------------

Section "Introduction", bullet "Named allocation in shared memory"
contains several grammar issues. It could also be read to imply
that only named objects can be created, which is not true. Suggest
replacing with something like

   Shmem supports creating objects in shared memory. It further
   supports associating a C-string with such an object at creation
   time, permitting the object to be looked up by name by other
   processes.

------------------------------------------------------------------------------

Section "Using shared memory as a pool of unnamed memory blocks"

There is race condition between the two example programs, in that
the first creates the shared memory segment, while the second uses
open, expecting it to have already been created. Addressing this
would significantly complicate the example, probably to no good
purpose, but it should perhaps be noted that this problem exists.

This same problem exists in lots of other examples.

------------------------------------------------------------------------------

Section "Creating named shared memory objects", between the two
example programs, says "In other process, while the first process
is alive, ...".

That bit about the first process being alive is unexplained and
confusing here, though necessary with the present design. But see
below for some comments on that part of the design.

------------------------------------------------------------------------------

Section "Using an offset smart pointer for shared memory"

At the end of the first sentence, replace "pointed object" with
"object pointed to", thereby being consistent with boost.smart_ptr
and I think reading a bit more clearly.

In the second sentence, in "it can point safely objects stored",
missing "to" between "safely" and "objects".

In the second sentence, in "mapped in different base addresses", I
think "mapped to" rather than "mapped in".

------------------------------------------------------------------------------

Section "Using STL containers and mapping the memory at a fixed address"

As written, this section could be read to imply that if one doesn't
want to worry about offset_ptr stuff, wants to use standard
containers, or just wants better performance, one should just go
ahead and use fixed address mapping. I don't think that is an
accurate assessment. Rather, I think it should be mentioned here
that fixed address mapping is highly platform-dependent. For
example, the address specified must be a multiple of the system's
page size and use of this option is explicitly discouraged,
according to mmap man pages on Linux and MacOSX. POSIX also says
the MAP_FIXED option is optional.

Also, at least for POSIX (I don't know about Windows), there are
two options where the base address might be specified, one
requiring it to be honored and one where it is advisory. The
existing shmem interface doesn't provide access to the latter
behavior.

Also, the example code here *really* ought to be checking for
failure of the segment create / open calls. Of course, that becomes
moot if two-phase construction is eliminated.

------------------------------------------------------------------------------

Section "Memory growth Limitation"

First paragraph, suggest changing

   ", although it provides a way to map the shared memory in the
   same base address."

to something like

   ". Although it provides support for attempting to map a shared
   memory segment to a specific base address, the mechanism for
   accomplishing this may sometimes fail, and may be unsupported on
   some platforms."

Second paragraph, suggest changing

   "If there is no more memory, an exception will be thrown."

to something like

   "Allocation from a segment may fail, throwing an exception, if
   there isn't sufficient space available in the segment."

------------------------------------------------------------------------------

Section "Problems with most STL implementations"

Grammar improvement, suggest changing

   "Many STL container implementations, suppose that
   allocator::pointer type ..."

to

   "Many STL container implementations presume that the
   allocator::pointer type ..."

"libstdc++" is the GNU libstdc++?
I think "STLPort" should be "STLport".

Is the statement

   "This is likely to change soon, ..."

actually true? Do you (or anyone else) actually know of any work
being done to address this problem? And have you reported this as a
bug against those implementations you've checked?

And this problem is in fact a bug in all of these implementations,
and not leeway permitted by the standard? I note that later, in the
rationale for process-shared containers, it says "The standard ...
allows implementations to ignore this and suppose
allocator::pointer is equal to T*." And later that section talks
about how the standard says nothing about how members of the
container should relate to the allocator's typedefs. In fact, I
think that is correct behavior, because the allocator present in
the container concept is related to elements in the container, and
is completely unrelated to whatever allocator was used to allocate
the container itself.

If my reasoning is correct, then there isn't any hope of a fully
general solution here. Some container types might happen to work
because their only members with pointer type are related to the
elements and so ought to be using the associated (per the container
concept) allocator's pointer typedef. But if the container class
happens to have other members with pointer type which aren't
related to elements, those aren't (and shouldn't be) controlled by
the container concept's allocator. Which I guess is just a
restatement of the last paragraph of your "Rationale for
process-shared STL compatible containers".

------------------------------------------------------------------------------

Section "Limitations"

"Problems with most STL implementations" later in this section,
since the problem described here is actually a consequence of some
of the others, in concert with bugs in the STL implementations and
their (improper non-)use of the allocator interface.

------------------------------------------------------------------------------

Section "Rationale for process-shared mutexes and condition variables"

I think it was unnecessary and a really bad idea to fork some parts
of boost.thread, such as the time utilities. It would have been
much better to file bug reports and supply patches to allow them
to, where appropriate, be used in a single-threaded configuration.
There is no good reason why boost.thread's xtime facility should
require a multi-threaded build, and that could be fixed.

One result of this appears to be that this fork does not contain
code that is present in boost.thread in order to support MacOSX,
because boost.thread works there, but shmem's forked xtime doesn't,
due to a dependence on clock_gettime, which isn't supported by
MacOSX.

I suspect but have not verified that boost.thread's lock class
templates are similar, in that they could be made to work in a
single-threaded build. They are not themselves thread aware, and
simply have some requirements for the mutex argument, which shared
mutexes probably already satisfy.

It might require a little bit of work to disentangle the
multi-threaded build requirement from boost.thread's xtime and lock
utilities, but that would be far preferable to forking them.

------------------------------------------------------------------------------

Section "Concepts and Definitions"

This is really "Terminology"; the term "Concept" has a fairly
specific technical meaning in C++ documentation, and this isn't
that.

In the description of the term "allocator", suggest changing

   "In Shmem, an allocator can be placed ..."

to something like

   "The allocators provided by Shmem may be placed ..."

Without something like the above change, it isn't clear that there
may be additional requirements on an allocator being in shared
memory beyond just having a segment manager. For example, the
reference from the allocator to the segment manager needs to be via
an offset_ptr (or similar mechanism) in order to support such
placement.

The term "segment manager" is defined here, but there doesn't seem
to be documentation for anything that fits this description. There
are several places where it appears, but no actual documentation.
At present it ends up being used as an opaque type that is returned
by various operations and can be passed to other operations. That
probably isn't sufficient for anyone who wants to write a new type
of allocator.

The term "front end" seems particularly poor. They are described as
proxy's for segment managers, so calling them something more
evocative of that would be preferable. It also seems like this kind
of object is almost an artifact of two-phase construction, and that
a set of static factory function could (almost?) just return the
segment manager object and clients could then use that directly (if
only it was documented). I haven't looked closely enough to really
convince myself that there isn't some problem with that idea, but
it should be examined.

Also, though not stated here, all (I think) of the "front end"
classes are defined as noncopyable. If they are just proxies, why
can't they be copied? Perhaps it is so that there is a consistent
opened/closed state, but that could be dealt with by adding another
level of indirection in the exposed proxy objects.

------------------------------------------------------------------------------

Section "Shared memory"

It isn't obvious whether the code for the shared_memory class
included here is the same as what is described in the reference
section, and in fact they do differ in some subtle ways (reference
doesn't mention the nested segment_info_t struct, where one might
think it was defined at namespace scope since the mentions of it in
the functor descriptions are unqualified). There might be other
differences I didn't notice. I'm not sure this code is actually
useful or appropriate here, and I think better might be a brief
summary and a link to the reference documentation. Certainly having
source for the class (stripped of inline definitions) in the higher
level introductory part of the documentation seems odd; if I wanted
to read the source code I'd go do that. Same comment for other
classes.

It is unclear whether the size argument for shared_memory.create
includes or excludes the overhead for the header created by the
shmem library (which get_base() points past). (Similarly for
related functions here and for other "front-end" objects.) I
guessed that the size argument was just the "user" space and
exclusive of that header (in part because nowhere could I find any
indication of what that header size might be, and in fact it seems
likely that can vary depending on the kind of shared memory object,
the allocator, &etc), and then verified that guess by looking at
the source code. But obviously it would be better if I could have
figured this out just from the documentation.

Similar issues with the get_size() member function, here and
elsewhere.

The member functions open_or_create and open_or_create_with_func
take a size argument. There is no discussion of what happens if the
shared memory already exists (so that this is an open rather than
create), and the actual size differs from the requested size in the
call. It is probably ok if the actual size is larger, but that is
less clear if it is smaller, and should perhaps be cause for
failure. Similar issues for other "front end" classes.

[In email prior to the review, the author indicated that something
like this might not be possible under windows, without the use of
mapped files. He recommended that I mention this anyway, and I
don't pretend to have any understanding of windows...]
A reference count of create + open - close is maintained for shared
memory objects. If this count reaches zero, the shared memory
object is unlinked (at least in the posix version). The lack of any
documented unlink mechanism might lead one to guess that something
like this is going on, and is documented by the last paragraph of
this section ("When all processes ... close ..., the shared memory
segment is destroyed"). A bit more emphasis might be useful here.
On the other hand, I'm actually not convinced this is a good idea.
It is certainly fragile, in that a program which crashes (for
whatever reason) won't close any shared memory objects it has open,
resulting in the reference tracking getting messed up. As a result
of that, one can't design a system assuming that shared memory
objects will always get cleaned up appropriately, and must be able
to deal with stale OS objects being left hanging around
unexpectedly. So this reference counting doesn't buy anything in
the way of design simplicity. It also has the disadvantage of
possibly losing state that might be interesting. It also introduces
the race condition previously discussed, where the creating program
must stay alive until an opener attaches, which makes some kinds of
designs more difficult. For example, one couldn't start up a
process which parses some data into an in-memory format that it
records in shared memory and then exits, with other programs saving
parsing time by just getting the information from shared memory.
This doesn't work if those other programs don't get around to
opening the shared memory before the parser program exits.

The various open/create[_with_func] operations presently lock a
global internal-to-the-library shared mutex in order to ensure
atomicity of create vs open. There isn't any documentation
regarding that mutex. I've occasionally run into problems where
that mutex ended up locked by a program that crashed at an
unfortunate place, and had I not figured out the exiseance of and
where to that mutex I would have had to reboot the computer to
clear it. (Note that on MacOSX I think I will need to write a
little utility for this, since POSIX shared memory objects don't
seem to show up in the file system, so far as I can tell.)

The various open/create_with_func operations seem to run their
functor in the context of holding that global internal shared
mutex. This can be problematic (see previous mention of programs
crashing at unfortunate times). A possible improvement that I'd
mentioned in private email with the author might be to have a
specific to the shared memory object shared mutex in its header
(already there in at least some of the "front end" object variants,
I haven't checked whether it is there in the more primitive ones
that don't have a segment manager), so the sequence would be
- lock the global mutex
- create the shared memory object, initializing its header
- lock the specific shared mutex, ensuring unlock on exit
- unlock the global mutex
- invoke the functor
- mark shared memory object initialized, so openers can tell if
functor completed successfully

There is shared_memory::close(), but there is no way to distinguish
an open from a closed shared_memory object. Note that mmapped_file
does provide a is_open operation. This might be rendered moot by
the discussion of two-phase construction.

There is no interface to permission bits (the mode_t argument to
shm_open and the like) in this interface. Instead, this library
always creates shared memory objects and files world read/writable
on Linux. I don't know what happens on Windows. This is a missing
feature of the library which some might consider important.

------------------------------------------------------------------------------

Section "Process-shared mutexes"

For shared_recursive_mutex,

   "... that can be blocked several times ..."

change "blocked" to "locked"

The use of code to describe the interface here seems inappropriate.
Since the only public interface is the constructor and destructor,
plus the *missing* lock types (presumably in the "// Friend
classes... " comment), it seems like it would be better to just
link to the reference documentation, possibly with the preceding
summary. Private stuff has no place in user documentation.

Similarly for Process-shared conditions, Named semaphore, Named
mutex, &etc.

------------------------------------------------------------------------------

Section "Process-shared read/write mutexes"

Note that the boost.thread read-write mutexes were (temporarily,
one hopes) removed from the boost 1.33.1 (I think) release, because
the existing implementation is broken. If, as indicated, Shmem
shared read/write mutexes are derived from the boost.thread code,
it seems likely that the same bugs exist here. This may be another
maintenance problem due to forking from boost.threads.

Also, some discussion of read/write mutexes in "Open Issues" section.

------------------------------------------------------------------------------

Section "A shared memory pointer: offset_ptr"

Change the first paragraph to something like

   "As previously stated, Shmem cannot count on being able to map
   shared memory to the same address in all processes. Because of
   this, raw pointers stored in shared memory may not be valid in
   another process. To solve this problem, offsets can be used
   instead of absolute addresses to refer to an object in the same
   shared memory segment."

and add a link to the appropriate Limitations subsection.

In the second paragraph, the whole discussion of null pointers is
rather difficult, and needs to be rewritten. (Running out of time,
or I'd offer some suggested text. Maybe later.)

I can't think of a good reason to ever use full_offset_ptr instead
of the default offset_1_null_ptr.

------------------------------------------------------------------------------

Section "Named shared memory object allocation"

The basic_named_shared_object class doesn't provide an interface
for atomic creation and initialization, a la
shared_memory::create_with_func. It is sometimes desirable to
create a named_shared_object and perform some additional
initialization (such as creation of some named sub-objects that are
always supposed to exist in the shared object). The alternative is
to make all clients of those contained objects use the
find_or_create interface, but that means that all clients must know
the appropriate creation arguments.

The signature for open() incorrectly has a size argument. The
reference documentation is correct about this. Another example of
the problem of (almost) duplicating information.

basic_named_shared_object provides an atomic_func() operation,
which permits one to call a function in a context where no
allocation &etc will happen. Really all it does is lock a shared
mutex (in the shared memory object's header) around a call to the
provided function (which was exactly the implementation I expected
to find). I think it would often be more convenient to provide
access to the mutex so that the client can explicitly lock it
around some code. As it is now, doing some computation that
provides a result is quite a bit less convenient than it could be.
In earlier private discussion with the author, the objection was
raised that exposing the mutex would prevent (at least without
breaking existing clients) it from later being changed to a
read/write mutex. One could add binding classes (a la scoped_lock)
for readers and writers with an implementation which currently uses
scoped_lock on a mutex (ignoring the reader/writer distinction) and
later reimplement it using a reader-writer-lock. That way the
abstraction is present for use now, and the implementation can be
improved without requiring source changes. It might also be
desirable to permit control over which gets used (as a policy trait
or something like that), since the overhead vs (in)frequency of
multiple readers might lead to a preference for a mutex-based lock
for some shared memory objects.

There are a lot of operations on shared memory objects that look
like they will probably crash horribly if called before the object
has been opened. It would be good if the documentation were to
indicate which operations can be called before opening and which
may only be called while open. This is related to the whole
two-phase construction discussion. If two-phase construction were
going to survive (it doesn't look like it from the email) I would
suggest that these all at least assert (or BOOST_ASSERT), and some
indication in the documentation as to which operations were safe
when not open. Fortunately this all looks like it will become moot.

Nomenclature issue: named_shared_object and the like is a somewhat
confusing name. I realize that, for example, POSIX calls these
things "objects", but in the context of this library calling these
things "objects" is confusing, especially with the additional
concept of "named objects" that this library provides. I haven't
thought of a good alternative name for the named_shared_object
concept though. I note that in your code you seem to use "segment"
for variables of this type, and I've more or less copied that
convention.

The Reference for named_shared_object doesn't include most of the
operations that are described in "Named shared memory object
allocation / The Interface". It looks like this is because most of
the actual implementation is provided by the
basic_named_object_impl base class, which isn't documented because
it is a "detail".

==============================================================================

I ran out of review time here, so the remainder are just a bunch of
very raw comments transcribed directly from my notes.

==============================================================================

Section "Swapping Shmem STL compatible allocators"

The section title says "allocators" but the content appears to be
about "containers".

It seems like this section properly belongs under "Limitations".

------------------------------------------------------------------------------

This bug in an older version was previously reported by private
email to the author, who said it would be fixed in the next version,
but I haven't verified that the fix occurred.

In the POSIX version of shared_memory::priv_create(), if the first
attempt to shm_open() fails, and creatonly is true, a call to
priv_close_handles is made with mp_info having an uninitialized
value. Since that uninitialized value is unlikely to be the same as
MAP_FAILED, this typically results in a segmentation fault on
mp_info->get_user_size(). Possibly the right fix is to ensure that
mp_info is initialized to MAP_FAILED (at least when using POSIX).

I haven't looked at the Windows version for any similar problems,
nor have I looked at other objects similar to shared_memory to see
if similar problems exist elsewhere.

------------------------------------------------------------------------------

The Reference section of the documentation (at least, html) contains no
mention of the contents of the containers directory.

------------------------------------------------------------------------------

Possible bug, or maybe I'm just doing something that isn't supposed
to work. I reported this against an older version, have not
verified that it is still a problem, and have not had time to track
down what was going on here.

If sh_string is a boost::shmem::shared_string that is allocated in
shared memory, the following doesn't work properly:

   const std::string x(std::string(sh_string.begin(), sh_string.end()));
   const std::string y(std::string(sh_string.begin(), sh_string.end()));

Empirically, the first std::string constructor would work and
return the expected string value, but the second one would throw an
exception complaining of "attempt to create string with null
pointer" (gcc 3.3.5, glibc 2.3.4, SUSE9.3). It might be that with
different shared memory layout or something, the results would be
different. But basically, it looks like shared memory data
corruption is occurring in the first std::string constructor that
uses the shared memory iterators. I haven't tracked this down any
further, because the "workaround" (which might actually be better
anyway) of

   std::string(sh_string.c_str())

works fine, and I'm behind on the stuff I'm supposed to be working
on. If you need more of a test case to track this down, let me know
and I'll try to produce one.

------------------------------------------------------------------------------

boost/shmem/sync/shared_mutex.hpp includes boost/shmem/sync/shared_mutex.hpp.

Fortunately there is an include guard...

------------------------------------------------------------------------------

Problems on gcc 3.2. In vector.hpp, there are two friend function
definitions in the vector class (both for operator+ on iterators),
which this compiler can't cope with.

Changing the definitions to declarations and moving the definitions
to below the vector class definition works around the problem (with
suitable templatization of the moved definitions), though the
compiler still issues a warning about the friend function
declarations being non-templates. I think I recall there being a
boost workaround technique for that?

The two functions in question are

   const_iterator operator+(difference_type, const const_iterator&)
   iterator operator+(difference_type, const iterator&)

I expect there are additional occurrences of this idiom in other
files, but this is all that my code tripped over.

------------------------------------------------------------------------------

Rationale for forking parts of boost.threads is weak, at least for
me. Contrary to what someone recently said on one of the the boost
mailing lists about their experience being that most code is
single-threaded, nearly all of the code I write is expected to be
used in multi-threaded environments, and that has been true for
quite a while. The only significant exceptions are for code that is
targeted at 8bit microprocessors and the like, where this library
is not an issue. I would really have preferred that the library
author simply depended on boost.threads as much as possible, note
the restriction that code using this library must be compiled
multi-threaded, and leave dealing with a non-multi-threaded variant
of boost.threads to that library. I haven't examined how well that
approach would actually have worked, since the interface for
boost.threads may not expose some of the knobs needed by this
library, and the maintenance status of boost.threads seems to be
somewhat unclear at the moment.

------------------------------------------------------------------------------

This is probably moot due to the two-phase construction discussion, but
just in case...

Under what circumstances might basic_named_shared_object::create()
return false rather than throwing an exception? The only case I can
think of is if a shared object with the requested name already
exists. Other kinds of failures I think would be much more
appropriate to report as exceptions.

Similarly for open_or_create(), which doesn't have any obvious
return false case that I can think of.

Similarly for open, which I would guess might return false if the
requested object hasn't already been created.

With the present documentation there is really no way to know what
the results of these functions actually mean, which makes it
difficult to do anything useful with them. My actual usage so far
has been to just assert that the results are true, which in
practice isn't really that much different from failing to handle an
exception thrown by these functions to indicate the relevant
failures.

------------------------------------------------------------------------------