Re: [boost] [flyweight] rw_locking policy patch for review

From: JOAQUIN M. LOPEZ MUÑOZ (joaquin_at_[hidden])
Date: 2008-07-05 16:04:36

________________________________________
De: Tim Blechmann [tim_at_[hidden]]
Enviado el: sábado, 05 de julio de 2008 11:26
Para: boost_at_[hidden]; JOAQUIN M. LOPEZ MUÑOZ
Asunto: [flyweight] rw_locking policy patch for review

> i have implemented a rw_locking policy for flyweight factories, based on
> boost::shared_mutex. to achieve that, i had to adapt the factory
> interface ...
>
> joaquín, would it be possible for you to review the patch? it is based
> against the version of flyweight from the boost sandbox svn ...

Hello Tim,

First of all, thank you for taking interest in Boost.Flyweight to the point
of implementing this very interesting extension. I've got several
comments on your code and a couple of requests:

- The patch looks very clean (which I'm happy about, cause that
probably means the original code is understandable enough for others
to work on it). After a cursory look I found what I think it's an error
and also a design issue:
  * The error is in flyweight_core::erase(). You have moved the invocation
  to check(h) outside of the write lock, which can cause a race condition:
     · Thread A destroys the last flyweight associated to some value x.
    check(h) passes as there are no more references and just when factory.
    erase() is about to be called the thread is interrupted.
    · Thread B creates a flyweight with value x.
    · Thread A resumes and the entry with x is destroyed leaving a dangling
    reference in B.
  * The design issue has to do with your moving all the knowledge of
  the locks to the factory itself. As I envision the design, all the lock stuff
  should be made by flyweight_core and factories should be kept as
  dumb as possible: see for instance that you've got a fair amount of code
  repetition across the insert() and erase() memfuns of the different factories,
  this is precisely a sign that you can factor this out, and the proper place
  (IMHO) to have it is in flyweight_core.

Apart from this the patch is nice and I'd like to use it as inspiration
(with your permission) to enhance the lib in the future when rw locks are
included. Unfortunately I haven't been able to devote much time to the redesign
the lib after the thorough and valuable criticisms from Alberto Barbati, maybe
this summer I'll be able to make progress. As I see it, rw locks will be an
additional option coexisting with traditional lock --this will be so because, to
the best of my knowledge, Boost.Thread needs linking and I would like to keep
Boost.Flyweight header only when possible.
Could you please confirm/deny that using rw locks requires building and linking
the Boost.Threads module?

My last request is: now that you've got rw locks implemented is a very
good opportunity to test them agains traditional locks in the context of
Boost.Flyweight and see how they compare (after all, if the gain is not
significative maybe rw locks shouldn't be used after all). Do you plan to
write some performance tests of rw against traditional locks? That would
be terrific.

Joaquín M López Muñoz
Telefónica, Investigación y Desarrollo