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

From: Tim Blechmann (tim_at_[hidden])
Date: 2008-07-06 06:33:10

thanks for having a look at my patch ...

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

i see your point ... double-checking before erasing value x should work 
around this issue ...

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

i see your point, it would be possible to extend the factory api with a 
find() memfun ... 
i didn't do that for now, since i think, this way certain factories could 
implement an more efficient insert() than a combined find()/insert() 
pair ...

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

i didn't know, that you were thinking about some architecture changes, 
but sure, go ahead and adapt my patch to your needs ... i would like to 
replace my own class with a similar concept as soon as flyweight provides 
rw locks :)

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

having a brief look at the new (1.35) boost.threads code, it seems that  
mutex/lock types are implemented header-only, but the used exceptions 
aren't ... 
however it would be quite straight-forward to implement the exceptions 
header-only, since the implementation provides just trivial constructors/
destructors and what() functions ...

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

sure, i could try to implement a small performance test for 
boost.flyweight to compare unique locks with shared locks ...

cheers, tim

-- 
tim_at_[hidden]
http://tim.klingt.org
The only people for me are the mad ones, the ones who are mad to live,
mad to talk, mad to be saved, desirous of everything at the same time,
the ones who never yawn or say a commonplace thing, but burn, burn,
burn, like fabulous yellow roman candles exploding like spiders across
the stars and in the middle you see the blue centerlight pop and
everybody goes "Awww!
  Jack Kerouac