Boost logo

Boost :

From: Ion Gaztañaga (igaztanaga_at_[hidden])
Date: 2006-02-17 16:57:40


Hi Kim,

Sorry for the delay, but I needed time to read carefully all your comments.

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

It seems quite through to me ;-)

> 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.

Proposals:

Changing "mmapped_file" to mapped_file"

What about changing named_shared_object, named_mfile_object,
named_user_object, named_heap_object to something like:

1)
shared_memory_framework
mmapped_file_framework
heap_memory_framework
external_buffer_framework

2)
shared_memory_manager
mapped_file_manager
heap_memory_manager
external_buffer_manager

3)
shared_memory_services
mapped_file_services
heap_memory_services
external_buffer_services

4)
smart_shared_memory
smart_mapped_file
smart_heap_memory
smart_external_buffer

> 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.

I will try to resolve the weak points you've seen. Maybe the
documentation is not enough due to the amount of issues the library
tries to solve.

> - 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?

It's not up to date. I think that boosters prefer a Boost.Date_Time
approach, since it seems the official boost time library and there is

> 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.

Ok.

> 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.

Ok. I will fix that problem.

>
------------------------------------------------------------------------------
>
> 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.

I will revise that.

>
> 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".

Thanks.

> 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, 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.

Ok.

>
> 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."

Ok.

>
------------------------------------------------------------------------------
>
> 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++?

Yes. Is GNU libstdc++ the complete name?

> I think "STLPort" should be "STLport".

Ok.

> 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?

There is intention to support this in GNU libstdc++ and Dinkumware STL
uses the allocator::pointer typedef. Metrowerks is also close. I don't
have information about if these libraries are going to support smart
pointer typedef approach some I will remove the sentence.

> 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.

Yes. It's unrelated to 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".

I think that "this is likely to change soon" phrase is unfortunate. It's
clear that there is no way to specify the additional pointers that the
container might contain.

It's not an easy task no. It should

> 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.

Ok.

> 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.

That was my option and it was a temporary solution so that Shmem could
be developed without depending on a Boost thread change since it is not
actively mantained. I also think that it was a bad idea.

> 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.

I would prefer making a generic locker that can be used with any class
that has public lock and unlock functions. I will try to do a proposal
this weekend.

> 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 ..."

Ok

> 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.

This has been already said, so segment manager will be moved out of
detail namespace and documented.

> 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.

I'm open to suggestions. That was the only name I could imagine.

> 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.

They have more functions, like flush/grow and other funtions related
with the memory back-end type. But I'm open to alternatives.

> 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.

It's possible to keep internal data in a shared_ptr member so that
"front ends" they can be copyable, but I don't if this feature is needed
to be implemented internally or it's acceptable to use shared_ptr and
dynamic memory.

>
------------------------------------------------------------------------------
>
> 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.

So you prefer removing class synopsis from the top and have just a link?

> 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.)

Ok.

> 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.

open_or_create just opens if it's not created so the size argument is
only applied when trying to create the segment. If boosters prefer other
behaviour, I can change it.

> 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.

You can have problems in POSIX systems, but I couldn't find a better way
to implement this. If a program crashes there is no way control
anything. The unique solution would be to provide a function that
destroys all named objects that I can register with every creation so
that you can catch signals and call that functions to free all objects.
In windows the OS frees the resources automatically. For standard C++
IPC mechanisms I would request OS help for program crashes, just like
heap memory is freed automatically.

> 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.

I would like to implement the POSIX-like behaviour in windows, but that
would require some permanent store that or a server/ process/service
that serves named IPC mechanisms windows. You can use memory mapped
files for this behaviour. Take in care that POSIX unlink mechanism is
also complex so that if a process unlinks the shared memory, if another
process can create a new shared memory with the old name while older
processes are still attached to the old shared memory. I need help from
POSIX experts.

> 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.)

Sorry for that, but

> 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

I tried to implement that, but behaviour can't be that simple, since
when an error occurs in the functor (the initialization fails) you have
to maybe unlink the segment. But since you have not the global lock,
another process can have tried to do an open/create and you have to
manage that complex race condition. I'm not saying that it's not
possible, but that is more difficult than this double lock approach.
With mapped files I could use file locking, so that if I have to create
the mapped file and initialize the segment manager, I can lock the
global mutex, lock the file, unlock the global mutex and continue
working without problems. But file mapping has not the same semantics as
shm_open/shm_close/shm_unlink and I think that wouldn't be as hard as
shared memory. I don't like also the reference count, since you must
have some metadata in the first bytes of the segment and you have that
"user/total segment size" problem.

I think that the ideal would be to have both approaches in both systems,
but in windows we would need some kind of external process that mantains
the segment alive to emulate file-like shared memory and in POSIX we
need some nasty tricks to implement reference counted approach. I think
both approaches have their points. But with program crashes, it's hard
to execute cleanups without OS help.

> 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.

This is a consequence of the reference count issue. If I choose the
reference count I need write-permission. Not that I'm a big fan of this
approach. I would be glad if you someone can help me on this. However,
if you want named_shared_object for read only, you need write access
because you lock mutexes constructed in the shared memory and that means
writing to shared memory. Simpler uses of shared memory might require
read-only access.

> 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.

I will try to do a proposal on this.

> 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.

I have no problem to remove it. I don't like much the boost.thread
interface. Some one is using Shmem version because since it is derived
from 1.32 version, it seems that there are no problems. I can propose a
new class in the future.

> 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.

This question has been raised. I've been thinking a bit, and I'm ready
to simplify making offset_ptr a offset_1_null_ptr class only. If someone
needs full_offset_ptr approach, can write its own pointer type and use
it with Shmem framework. Pavel will be happy with this!

> 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.

That's not difficult to implement, since it can forward functor to the
underliying shared_memory object functor initialization constructor.

> 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.

Ok.

> 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.

I really don't like the idea of exposing the mutex. The framework can
use internally another locking mechanism.

> 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.

That would prevent simultaneous find functions from several processes.
If you have a lot of named objects find can take some time that is
negligible if you compare it with locking 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.

You are talking about having some kind of specialized scoped lock class
(initialized with the segment manager, for example) that can be locked
for find/change policy? I'm ready to think about it but I would ask that
to be developed for a future version, so that I can propose it in the
mailing list and have opinions from others. Would you accept this?

> 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.

You really like configurability! I could add another member to
mutex_family structure that configures synchronization configuration
with a compile time boolean saying that we want read-write approach or
simple mutex approach. But I would let this for a future version. I have
to see the whole picture after with more time.

> 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.

Two-phase construction is dead.

> 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 will write all inherited functions with forwarding code so because
BoostBook/Doxygen does not show inherited members. I will add also the
comments so you have a full reference. I will also request inherited
functions in documentation mailing list for BoostBook, but sometimes,
with template parameters is hard to tell what is being inherited.

> 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".

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

I propose not to document STL containers since those are kown and it
would not add useful information. I should document flat_map family
since they are not standard containers. These containers can be in the
future proposed for general boost containers.

> 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()));

I had no time to check this, but I will try to see what's happening. In
theory, if a return correct iterators (if even begin is equal to end)
the code should not crash. Can you see if the string is empty when this
error occurs and if the both iterators return the same value to have
some clues to reproduce the bug?

>
------------------------------------------------------------------------------
>
> boost/shmem/sync/shared_mutex.hpp includes
boost/shmem/sync/shared_mutex.hpp.
>
> Fortunately there is an include guard...

Thanks to point it out.

> 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.

After revising the library I can work on portability issues. If you have
more problems, let me know.

> 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.

That would not give me access to scoped_locks though. But now that
Date_Time is proposed there is way to reuse Boost.Threads.

> 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.

Or the operating system can't map the memory, the user atomic functor
returns false... I like to revise Beman's proposal for error reporting,
but I must find a way to notify native OS errors and also a portable
error handling. I have not much experience with exception reporting so I
appreciate help to design a correct error reporting. Would you like to help?

Thanks for this through review and your bug reports. I agree that there
are rough edges in the library and I should make it more strong against
crashes. But I don't know any portable shared memory implementation that
has solved this. Cygwin has not shm_open, apache portable is not really
portable enough with unlinking semantics... I don't know how Unix
Services for Windows from Microsoft solves this or they have OS help
that we don't know.

Regards,

Ion


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk