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

From: Pavel Vozenilek (pavel_vozenilek_at_[hidden])
Date: 2006-02-09 02:10:49

I vote to accept the library into Boost.

I have been following shmem development
for long time and most of the issues
I had were resolved.

It covers important but so far neglected
area. Potential for tools based on shmem
is large.

Ion has created extensive (1MB of sources)
and powerful tool and I have full belief
he will maintain and improve it futher.

For practical reasons I do not recommend
attempt to split the library into
separate parts.

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.

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

   I think naming conventions should
   be discussed as the most important
   issue of the library since shmem
   has real chance to establish de-facto
   standard for C++ IPC and process synchonization.

/Pavel

__________________________________________________________
1. index.html:

"portable synchronization primitives"
==>>
"portable interprocess synchronization primitives"

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

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)

These terms should have link to glossary page
when they are used for the first time, ala:

<a ref="....">dynamic allocation</a> (<a ref="...#...">in glossary</a>)

__________________________________________________________
2. Naming: IMO the naming convention should be reconsidered.

It is rather hard to intuitively feel difference between
  "named_shared_object"
and
  "named_user_object"
for example.

I am not and English speaker so I don't dare
to suggest alternatives but current names are
quite confusing to me.

Anyway, all naming conventions should be
explained in glossary.

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

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

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

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.

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

Which reminds me: there should be a page
listing all examples with short description
what important and useful is in each example.

It is fast and cheap way for learning
and it is used e.g. in multi-index.

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

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.
It is unexpected on the first sight.

------------------
The "offset_ptr" word should be linked
to reference.  Generally, the docs should
be interlinked as much as possible.

------------------
An example with offset_ptr AND named object
may be added to ensure the first time
reader these tools are orthogonal.

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

Medieval publishers titled their books in similar
way and it sold well for centuries.

-------------------
Just a pedagogical nit. The snippets show:

int main() {
   ...
   if (!...) {
      return -1;
   }

Depending on OS the value (-1)
would produce strange effects. Use 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)

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

__________________________________________________________
5. limitations.html: more hyperlinks.

-------------
In sentence
  "References will only work if memory is ..."

the word "only" should be bold and red.

------------
Perhaps this page should be divided into two parts:
* shmem is always mapped to the same address
* shmem can be mapped to different addresses

and for each cases limitations listed.

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

__________________________________________________________
7. concepts.html: should be named glossary
   and expanded.

Some texts here sound strange:

"...memory algorithm is an object..."

Picture/diagram would help here.

The bolding is overused.

I feel uneasy about the term "front-end"
but have no suggestion.

__________________________________________________________
8. oswrappers.html:

"Base shmem classes" may be better name.

a the classes may have name
  "XYZ_base".

The word "basic" is also used
in two docs pages later and it is
very confusing (to me).

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

The code snippet should have link to this
"more" leaf page.

----------------
Perhaps the construction/initialisation
dichotomy for mapped_file is not needed.

The class looks cheap enough to use
constructor/desctructor for them.

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

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

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

Or will reboot be needed to cleanup
the system?

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

This may be also useful for self-restarting
applications.

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

Should I pick names I would use
"XYZ placed inside shmem" and
"XYZ identified by name".

---------------------
Generally, this page (oswrappers.html)
is /extremely/ dense with information.

It covers stuff that is taught
during whole university semester.

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

Links to examples and class diagrams would help here.

__________________________________________________________
9. named_shared_object.html:

The title may be:
  "Named user data places in shmem"
or something what forms full sentence.
I have trouble parsing the current name.

-----------------
Seeing the whole class synopsis is
a bit frightening experience.
(a blob on two pages).

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.

Other possibility would be SGI STL like
documentation with table and detailed
comments bellow the table.

-----------------
Title:

"Common named shared object classes"

==>

"Default specialisation of class doing named shared memory allocation"

;-)

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.

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

Links to examples and class diagrams would help here.

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

Instead of having boost::shmem::anonymous_instance
I would prefere to have function

   construct_unnamed(...)
or
   construct_anonynous(...)

Similarly
   construct_unique(....)

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

The discussion of "index types" feels
as something so advanced that it should
be moved futher down in the docs.

Anyway, current documentation too low
on details and actual purpose.

Missing is a /table/ comparing
each type and where it shines.

__________________________________________________________
9. stl_allocators.html:

A very dense page. A table on the top comparing
advantages of each allocator type would help.

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

I guess every allocator type is used in at least
one of shmem containers: there could be link
to show this as an example.

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

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

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.

__________________________________________________________
11. customizing_boost_shmem.html:

This should be separated into three parts: algorithms,
custom allocators and indexes.

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

The previous pages in the docs should have
hyperlinks like

   "see how to build your own XYZ (very advanced)"

pointing here.

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

Generally, interlinking should be used much more.

__________________________________________________________
12. beyond_shared_memory.html:

This name suggests super-advanced functionality
far beyond needs or abilities of anyone.

I think what is presented here are very useful
tools that could be used separately from any IPC
and synchronisation.

Complete examples would be very helpful here,
as well as careful wording that doesn't
suggest need to use whatever "shared"
(in OS sense).

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

The note about file mapping in initial
parts of the docs should have interlink
pointing here.

----------

I remember Pablo Halpern's proposal
for new allocator (on comp.std.c++).

One of the features was ability to 'move' objects
passed into an container under allocator of
this container.

At the moment of writing I have only vague
recollection but I think it could be possible
(via shmem containers):

* create complex data structures in specified
  memory areas (without the need to modify
  too much of user code).

* being able to "(de)serialize" complex data
  structures.

(As I said, these are vague ideas from the past.)

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

    At this moment I am not able to comment on it more.

__________________________________________________________
14. shmem_smart_ptr.html:

A teaser in form of miniexample should be on the top
of the page.

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

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

Danger of making mistake is too large.

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

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

------

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

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

I guess that people who will use shmem will
be the ones who happily use such tricks.

------

The "MyLess" in the example is not used consistently,
should be removed for clarity.

-------

The names like "mg1", "mg2" should be longer.

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

---------
I wonder whether and how it would be possible to build
'objects' passing queue above shared_message_queue
(subject of necessary limitations).

I know such tool can get easily misused
but it would be first step to implement
Erlang-like message passing framework.

__________________________________________________________
16. architecture.html:

Diagrams picturing the levels of the library and
interaction between the levels would be handy.

----------

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

__________________________________________________________
17. performance.html:

Examples would be helpful.

Examples that show a slow way as well as
examples tuned for performance.

__________________________________________________________
18. open_issues.html

Namespaces: I do not like idea of more than one namespace
as it would only complicate already feature rich tool.

If it is possible to implement (transparently
to the user) a "type" checking then add it.

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

__________________________________________________________
20. All shmem exception should have one common parent.

Name sem_exception is too short.

lock_exception should be deadlock_exception.

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

Possibly shmem::bad_alloc may inherit from
both common parent and std::bad_alloc.

This would make sense if release of "normal"
memory could have positive effect on availability
of "shared" memory. Just an idea.

__________________________________________________________
21. Wishes for additional functonality:

* ability to enumerate named from shmem.
  Useful for debugging/troubleshooting.

* debugging support: for example the first
  byte of shmem should be fixed constants
  and assert()s should be everywhere
  to check that user code didn't overwrote
  this.

  Possibly the metadata may have CRC stored
  and /always/ checked against.
  It may sounds as paranoia but multiprocess
  troubleshooting is such a pain that any measure
  that helps to catch a bug is justified.

* in "debug" mode the shared memory areas may
  be surrounded by guard areas (possibly too hard)
  and memory of an deleted object may be filled
  with 0xCC.

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

__________________________________________________________
22. Wishes for docs:

* a page named "What I can do with shmem"
  with one liner description and link to
  example or detailed docs.

  This should cover small practical tasks like
  using process wide mutex, adding data
  into shared queue etc.

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

* possibly a visual separation could be used
  for "basic" and "advanced" topics
  or for levels like
     "implementation details",
     "basic functionality" and
     "higher level tools".

* the docs may have section on typical
  (or expected) user mistakes, for example:

  - "what if I retrieve named object but
    use wrong type"

* the docs should say whether it is possible
  to somehow block shmem from working properly.

  E.g. both processes crash and leave
  a named OS resource hanging out,
  restarted processes will fail to
  connect the resource or it will
  be damaged beoynd repair.

  To discover such corner cases later
  in the project may very unpleasant.

* Pictures of inheritance diagrams could be relatively
  easily generated by Doxygen. On many places
  such pictures would come handy.

__________________________________________________________
EOF