Review of proposed Boost.Shmem library Kim Barrett <kab@irobot.com> [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. ------------------------------------------------------------------------------