[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 ... best, tim -- tim@klingt.org http://tim.klingt.org Cheat your landlord if you can and must, but do not try to shortchange the Muse. It cannot be done. You can't fake quality any more than you can fake a good meal. William S. Burroughs

________________________________________ De: Tim Blechmann [tim@klingt.org] Enviado el: sábado, 05 de julio de 2008 11:26 Para: boost@lists.boost.org; 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

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@klingt.org 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

________________________________________ De: boost-bounces@lists.boost.org [boost-bounces@lists.boost.org] En nombre de Tim Blechmann [tim@klingt.org] Enviado el: domingo, 06 de julio de 2008 12:33 Para: boost@lists.boost.org Asunto: Re: [boost] [flyweight] rw_locking policy patch for review
* 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: [...]
i see your point ... double-checking before erasing value x should work around this issue ...
Yep.
* 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 [...]
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 ...
Well, there's always that tradeoff lurking in... I prefer to keep the design orthogonal and risk losing that potential improvement (which hopefully would e minimal if it existed). [...]
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 ...
Nice! Please keep me informed, and do not hesitate to contact me if you need assistance in the process. I'm very curious to know the actual degree of performance improvement rw looks can bring about. Joaquín M López Muñoz Telefónica, Investigación y Desarrollo
participants (2)
-
JOAQUIN M. LOPEZ MUÑOZ
-
Tim Blechmann