________________________________________ 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