Re: [boost] [review] Review of Flyweight library started January 21 and still running!

From: vicente.botet (vicente.botet_at_[hidden])
Date: 2008-02-03 05:59:20

Hi all,

Thank you very much Joaquín , I have learn a lot with your library
and the review. Thanks also for all the pertinent answers.

Thanks for extending the review period.

Ion Gaztañaga wrote:
> The formal review of Flyweight library, proposed by Joaquín M. López
> Muñoz, begins today (sorry for the late announcement):
> * What is your evaluation of the design?

The library has a simple to use interface based on a understandable
domain-specific language having a good configurability and extensibility
degree, even if the naming of some policies could  be improved.

This easy to use interface hides a more complex design. The relations
between the different policies are not evident, and even if the library
provides a elegant way to extend it, it's no evident to define such
extensions without reading the code. Maybe the documentation should
include a deeper description of the collaboration of the core implementation
and the different policies.

I like the way Boost.Parameter is used. In particular the use of is_locking,
is_tracking, ...  specialization in order to declare extensions.

I like the idea to encapsulate in flyweight<T> the acquisition and the
release of the shared resources, and as consequence masking the factory
initialization.

I like also the separation between the specifiers and the implementation
classes.

* performance test with intermodule_holder
The tests performed on example5 should be extended with a intermodule_holder
specifier.

* intermodule_holder could use static_holder_class
As the intermodule_holder is a kind of workaround for platforms where

DLL can duplicate static libraries, we can have a better performance on
the others for free.

On platforms on which static variables are unique inter DLL the
intermodule_holder specifier type can be a static_holder_class.
This could be controlled using conditional compilation.

struct intermodule_holder:holder_marker
{
  template<typename C>
  struct apply
  {
#ifdef BOOST_NO_DLL_UNIQUE_STATIC_VARIABLE
    typedef intermodule_holder_class<C> type;
#else
    typedef static_holder_class<C> type;
#endif
  };
};

This variable BOOST_NO_DLL_UNIQUE_STATIC_VARIABLE
or a better name should be included in the boost/config.h file.

I hope that the boost community could help to find on which platforms
BOOST_NO_DLL_UNIQUE_STATIC_VARIABLE must be defined.

* holder specifier renaming
The holder's main responsibility is to instantiate a unique instance for
its internal resources depending on the scope of the flyweight class.

So the variation here is the scope. I haven't a better suggestion, but in
my opinion holder do not reflects its responsibilities.

* concrete holder specifiers renaming
I'd RENAME static_holder by module_holder, and intermodule_holder
by process_holder.
module_holder reflects much more the scope accessibility intent for the
users, static_holder talks more about  the implementation.

So at the end there will be two holders:
. module_holder (renaming of static_holder) and
. process_holder

I don't think that simple_holder is better than static_holder.
Does simple_holder stands for simple implemented holder?

* factory specifier renaming
Maybe repository could be more adapted to the associated responsibilities,
store the shared values.

* equality semantic
It would be great if the library reach to manage with the pathological
cases without reducing the performances of the usual cases.
If in order to solve the problem you propose the user overrides the
operator== with non-& semantics this must be included in the documentation.

* adding a exclusive core specifier.
In order "to provide maximum customizability so as to meet everybody's needs
from basic users to programmers with very specific requirements" I think
that the core specifier is needed.

// default configuration
typedef flyweight<T> fw_t1;

// change core implementation
typedef flyweight<T, core<shared_ptr_core> > fw_t2;

// or even with the deduced parameters, change core implementation
typedef flyweight<T, shared_ptr_core > fw_t2;

// change some aspect of the default core as now
typedef flyweight<T, no_tracking > fw_t3;

This complicates only the flyweight class which needs to take care of
the exclusion of the parameters. And of course this will need a description
of what is expected from a core.

As the interface do not change, this could be for future work.

> * What is your evaluation of the implementation?

Quite good .

Only some details
*  Minor correction
The documentation states that the ..._specifier must be a ..._marker, but
this is not needed for the ..._class
On the code static_holder_class inherits from holder_marker, and
hashed_factory_class inherits from factory_marker.
Is this an error without importance, or there is something behind?

* refcounted_value is a class that could be public. Why it is declared in
the namespace detail.

* could you explain why detail::recursive_lightweight_mutex is needed and
boost::detail::lightweight_mutex is not used directly?

struct simple_locking:locking_marker
{
  typedef detail::recursive_lightweight_mutex mutex_type;
  typedef mutex_type::scoped_lock             lock_type;
};

A more general Boost question. Can boost libraries use the details
namespace (i.e. private implementation) of the other boost libraries?
What is needed to promote this shared private implementations to a public
boost library? Should someonepurpose the library?

> * What is your evaluation of the documentation?

Good!

It was really a pleasure to read the tutorial.

Some more elaborated examples should be included in order to show
the user how to manage with more complicated examples, for example
inter-process flyweights. These examples should be a probe of the
extensibility mechanisms and a check the orthogonality of the policies.

* intermodule_holder specifier
A reference to a document explaining the problem with  platforms not
ensuring the unicity of static variables when DLL are used will be welcome.

* Reference section
The reference section was much more hard to follow. I think that the
interactions between the different classes are very hard to describe
using the current style.
Un implementation section will help to understand how all this works,
describing the interactions between the core and the policies.

* Performance section
I expect a performace section to be included. And how the future
work  will improbe this performances.

* Rational
Adding of the map versus set problem  explanation.

> * What is your evaluation of the potential usefulness of the library?
Adapted to the described context.
The current Flyweight design do not support very well persistent Flyweights,
but this is another history.

> * Did you try to use the library? With what compiler?
No.

> * How much effort did you put into your evaluation?
In-depth reading of the docs and implementation.

> * Are you knowledgeable about the problem domain?
Yes, I have already used the Flyweight pattern in the context of mi work
applications. Once I used Flyweight on a database context in order to reduce
the space used by values that were duplicated too many times.

> And finally, every review should answer this question:
>
> * Do you think the library should be accepted as a Boost library?
I think that the library proposed by Joaquin should be accepted.

I think that this Flyweight library is useful as it is for some kind off
applications. No mayor changes are needed to the interface.

Most of the renaming suggestions can be done adding new specifiers and
using the extension mechanism.

Good luck Joaquín

---------------------------
Vicente Juan Botet Escriba