Re: [boost] [Review] Shmem (lomg)

From: Ion Gaztañaga (igaztanaga_at_[hidden])
Date: 2006-02-09 16:16:15

Hi Pavel,

> I vote to accept the library into Boost.

Good start! ;-)

> What I do not like is described in details bellow.
> Most of my objections fit into two groups:
> 
> 
> 1. The documentation is /very/ dense
>    with information and may easily
>    scare a reader.
> 
>    Examples to play with and pictures/diagrams
>    wold lessen the mental toll on reader.

I agree. I will try to add some diagrams.

> 2. (From someone who is not English native.)
> 
>    Currently used names are hard
>    to understand and remember and lack
>    intuitive meaning.
> 
>    Some words like "shared", "named"
>    or "object" are overused

They are not my favorite but I couldn't find better ones! I want also to
be a relationship between the front-ends, since all offer similar
functions but each ones operates in a different type of memory
(named_shared_object, named_mfile_object...). Now we have to find a name
to an object that creates shared memory/memory mapped file and allows
allocation of named objects there. I'm open to change it.

> __________________________________________________________
> 1. index.html:
> "portable synchronization primitives"
> ==>>
> "portable interprocess synchronization primitives"

Ok

> Need for glossary: the first page mentions
> several terms that are not really well defined:
> 
> * dynamic allocation
> * segment
> * named allocation
> * base address (not eveyone may know)

Well, maybe it's that I can see it through the eyes of a newbie. I will 
try to define them (as accurate as I can).

> I have uneasy feeling from these names:
> * named_shared_object (vague for me)
> * segment_manager (the word manager is overused)
> * segment (too much reminds me the x86 segments)
> * mixing of "object" and "class" together

Sorry, but I couldn't find better ones. Open to change if someone 
proposes good alternatives.

> __________________________________________________________
> 3. quick_guide.html: as it was suggested
>    a smart pointer should be used in the first example.
> 
> 
> I am against adding new typedef into shmem, seeing
> 
>   shared_ptr<void, shmem::deleter>
> 
> gives me immediate clue, while
> 
>   shmem_ptr_whatever
> 
> is something one needs to look up.

Ok, I wanted to keep example simple, but I don't think a shared_ptr will 
hurt.

> -----------------
> 
> The code snippets may have link to full
> source code of an example next to them.
> 
> Especially for first few snippets this
> will encourage novices to play and get
> immediate feedback.

Good idea. Since they taken from real code it's not difficult.

> Code snippet about retrieving named object
> from shmem (the fourth): the need for
> std::pair when extracting the object
> should be explained in a comment.

Ok.

> -------------------
> The title:
>   "Creating vectors in shared memory
>   with different base addresses"
> 
> should be
> 
>   "Shmem mapped to different base adresses?
>    Standard STL containers cannot be used.
>    Solution: shmem's own container set."
> 
> Long but self-explaining.

I find your phrase too long, but I will think in a better alternative.

> -------------------
> Just a pedagogical nit. The snippets show:
> 
> int main() {
>    ...
>    if (!...) {
>       return -1;
>    }
> 
> 
> Depending on OS the value (-1)
> would produce strange effects. Use 1.

Strange effects? I didn't know that. Which type of effects? I can change 
all return errors to 1.

> -------------------
> 
> Nits for snippet #6:
> 
> * the name ShmemAllocator should be shmem_allocator_t
> * alloc_inst  should be "my_allocator" or so
>   (the word "inst" means something in Prolog/Mercury)

No problem.

> __________________________________________________________
> 4. offset_ptr.html:the "offset_1_null_ptr" policy
>    is sufficient for all purposes.
> 
>    Since I cannot think of any use case where
>    an other pointer policy is required I recommend
>    to remove them. This should decrease code
>    complexity and most importantly mental load
>    on the user.

I know you were since the beginning against this. But I will try once 
more to resist. ;-)

> __________________________________________________________
> 5. limitations.html: more hyperlinks.
>   "References will only work if memory is ..."
> the word "only" should be bold and red.

Bold will be enough!

  > __________________________________________________________
> 6. rationale.html: the text about containers repeats
>    what was already said before.
> 
>    While repeating helps one to remember better
>    it is for the third time on last three pages
>    (quick guide/limitations/rationale)

Right. I will try to say it only once.

> ---------------
> The functions "create_with_func/open_with_func/..."
> 
> I still didn't got need for these (I am slow)
> but there should be a leaf page with an example
> and typical list of situations where this feature
> is necessary.

This functions allow atomic initialization of objects the shared memory 
when connecting/creating, so that two processes can create AND 
initialize shared memory without race conditions. The alternative would 
be to lock a external named mutex. It is used, for example, in the 
shared_message_queue, if two processes open_or_create the message queue, 
the creator, apart from creating the shared memory segment, will 
initialize queue members safely.

Others have requested similar functionality for named_shared_object, so 
they can create the segment and atomically create some named objects. 
It's a matter of taste I think.

> ---------------
> Huge (> 4 GB) files and mapped_file class:
> 
> I am not sure what size_t is
> on current 64 bit systems.
> 
> To be absolutely, positively, safe I would use
> fileoff_t everywhere instead of size_t.

I use size_t when referring to memory since size_t is enough to point 
all memory range. When referring to file offsets, I use fileoff_t. I 
think this is safe (if there is any 64 expert here, please correct me if 
I'm wrong).

> -------------------
> 
> (Absolutely uninformed guess follows.)
> 
> If process A crashes in the middle
> of shmem operation (I expect this
> to happen during development), will
> all the mutexes/shared memories/etc
> be destroyed automaticlalyy after
> process B is closed?

Good question. The answer is... it depends. I can't guarantee cleanup 
when a process crashes, because I can't register rollback functions in 
C++. In Windows, the shared memory is automatically released by the OS. 
In Unix the file-like interface does not do this. And this is really 
annoying. I haven't found solution for this (yes I could handle all 
signals including SIGSEGV, but I would let UNIX signals unusable). If 
any POSIX expert can help I would appreciate.

This is, in may opinion, the weakest point of the library. The behavior 
of the the shared memory is correct when all goes well. But when a 
process crashes, I can't do anything.

> In second case, could some API be added
> to destroy any named remnants on explicitly?

I think this is a good idea, so that we can make more robust 
applications. I would need to register in a process-level map (a static 
object perhaps all objects when I create them. However, this 
singleton-like interface can create problems if we create static named 
objects (when we will have a good singleton in C++?)

  > --------------------
> Use of term "process-shared" and "named".
> It is confusing (for me) and it took
> me a while to get it.
> 
> I suggest to add short info on the top
> of the page describing distinction between
> these two approaches in tabular form.

Ok.

> ---------------------
> Generally, this page (oswrappers.html)
> is /extremely/ dense with information.
> 
> It covers stuff that is taught
> during whole university semester.

I know, but I don't pretend to explain these concepts. I already suppose 
the programmer knows something about them.

> At least reader should be warned
> on the top of page that lot of time
> is needed to grok it.

Ok.

> __________________________________________________________
> 9. named_shared_object.html:
  > Perhaps a mini-synopsis with just
> template parameters and few most
> important functions (or groups of functions)
> could be shown and only then followed by
> the full class.

Ok.

> -----------------
> Title:
> 
> "Common named shared object classes"
> 
> ==>
> 
> "Default specialisation of class doing named shared memory allocation"

I will try to find an alternative.

> I do not like the group "object classes" and
> do not like that something with "object"
> in name is a class.
> 
> I think this should be discussed.

I agree. But I run out ideas some time ago.

  > __________________________________________________________
> 9. stl_allocators.html:
> 
> A very dense page. A table on the top comparing
> advantages of each allocator type would help.

Ok

> A link to header(s) implementing the allocators
> should be present so reader could jump there
> quickly.

Ok

  > __________________________________________________________
> 10. containers_explained.html:
> 
> An overview of Boost libraries compatible
> with shmem couled be added (I suspect multi-index
> is compatible, while say smart pointer generally isn't).

Sorry, but I'm not aware of any.

> The last code snippet may be better if
> separated into smaller and focused parts
> like "all in shmem", "nothing in shmem",
> "whatever inbetween" and so on.

Ok

  > __________________________________________________________
> 12. beyond_shared_memory.html:
> 
> This name suggests super-advanced functionality
> far beyond needs or abilities of anyone.

Any name suggestion?

> The relevation of memory mapped files feels
> too late, at this moment. Too many readers won't
> get that far or will skip the "beyond".

You are right. Maybe I should say it just after named_shared_object 
explanation, since you have basically the same features.

> __________________________________________________________
> 13. streams.html: there could be possibly overlap with
>     Boost.Iostreams library.

I don't know Boost.Iostream well. Anyway, I think bufferstream and 
vectorstream are very general tools that could replace many scanf/printf 
functions of C++ code alergic to stringstream overhead.

> __________________________________________________________
> 14. shmem_smart_ptr.html:
> 
> For examples I suggest to always use full qualification
> for boost::shmem::scoped_ptr (and the other one as well).

Ok

> __________________________________________________________
> 15. shared_message_queue.html:
> 
> The mention of named_user_object should have backlink
> and a comment that this means user-buffer-related tool.
> 
> 
> ------
> 
> There's no explicit info what happen if
> the buffer-is-too-small error happens:
> will it read part of message, will it
> stay in queue or will it be discared?

It will stay in the queue.

> This and info whether message can get fragmented
> should be added.

Message won't never be fragmented. I will write that.

> The double contruction/initialisation steps
> may be merged into one (no strong opinion,
> just a feeling).

I will add it as exception throwing alternative.

> ------
> Functions like: how-big-is-the-next-message()
> or peek() may be added.

Ok. With "peek" you mean copying the message but without extracting from 
the queue?

> The "MyLess" in the example is not used consistently,
> should be removed for clarity.
> The names like "mg1", "mg2" should be longer.

Ok.

> -----
> Information on atomicity of the queue should
> be told explicitly inside introduction paragraph.

Ok

> __________________________________________________________
> 16. architecture.html:
> 
> Diagrams picturing the levels of the library and
> interaction between the levels would be handy.

Ok

> ----------
> 
> Information how lifetime of OS resources is managed
> should be here (especially for Posix).

Ok

> __________________________________________________________
> 19. future_improvements.html
> 
> Security attributes - IMHO adding them is
> almost trivial (I know, I know) and they should
> be added before the library is thrown to public.

But how will you unify POSIX/Windows security attributes? I hard issue, 
in my opinion.

> __________________________________________________________
> 20. All shmem exception should have one common parent.
> 
> Name sem_exception is too short.
> 
> lock_exception should be deadlock_exception.

Lock exception is named after boost::thread name.

> __________________________________________________________
> 21. Wishes for additional functonality:
> 
> * ability to enumerate named from shmem.
>   Useful for debugging/troubleshooting.

I could fill (atomically) a vector with info about types. I will look at 
this.

> * debugging support: 

Ok, but this is quite a big task. I would like to implement them in 
future versions of Shmem.

> * primitive "transaction like" functionality
>   for shmem.
> 
>   Scenario:
>    - shmem segment exists
>    - I do copy (clone) of the shmem data
>    - many operations with shmem are executed
>    - something may fail but ability to revert into
>      well-defined state (in application sense,
>      not in low level C++ sense) is impossible
>      at this moment
>    - then the stored copy of shmem will be written
>      back into shmem segment, restoring the initial,
>      well defined state.

Uf. I think this is beyond my knowledge!

> * the current documentation should visually
>   distinguish parts showing problems, mistakes
>   and potentially wrong uses of shmem.
> 
>   Picture of a bomb is typically used
>   for this purpose.

Ok.

Thanks for the review! I've left some comments out of the reply, but I 
think you will happy if I address "only" those from above. It's just 
that replying your reviews is a very hard work! ;-)

Regards,

Ion