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