[hibernate-dev] HSEARCH-189 & concurrency

[Emmanuel Bernard]

Hey,
Can you tell me more about the piggyback synchronization. I could not  
find any decent knowledge online.
how far reading a volatile guarantee that all "local" values of the  
thread we are reading from will be pushed to the shared memory?
For example, could reading a volatile value after HSearch is done with  
initialization (all init is done in a single thread) guarantee that  
all states held by this thead will be pushed back to the shared memory?
The use case is quite specific, I init everything in a single thread,  
want to push all the state to the shared memory. I know post init()  
use of HSearch will never change the state so I don't "need" locking.

--
Emmanuel Bernard
http://in.relation.to/Bloggers/Emmanuel | http://blog.emmanuelbernard.com 
  | http://twitter.com/emmanuelbernard
Hibernate Search in Action (http://is.gd/Dl1)

On  Jul 4, 2008, at 04:33, Sanne Grinovero wrote:

> Hi Pavitar Singh,
>
> I thank you very much about your explanations but actually I opened  
> the issue myself
> because I have read the same specs and am aware of that.
> in H.Search (and several other hibernate code) there's this quite  
> common pattern for starting
> "replaceable" objects (something like user-written plugins, you can  
> provide your own implementation
> to do some stuff) but this same pattern is also used to start the  
> built-in default strategies.
>
> It looks like this:
> - an empty constructor, to use class.newInstance();
> - an initialize() to set configuration options
> - a start() method (eventually) used to start background tasks
> - some doStuff() and/or getXX() which need to be fast & threadsafe
>
> As you can see in Concurrecy in Practice at page 50, this is BAD, as  
> for
> example in the FSSlaveDirectoryProvider nobody takes care of locking
> or visibility, and nobody is doing anywhere where I see this pattern  
> used
> (several times in the project).
> I'm not saying it is all broken, because usually the threads consuming
> these unsafely-initialized objects are started after the  
> initialization, so
> that's ok. In this specific case the state will be used to communicate
> between threads, so some visibility fix is needed.
>
> I know I could use it only for final fields, but this is exactly  
> what I want:
> there are currently 10 instance variables, of these
> 4 have no concurrent use
> 4 are configuration constants and could use the "final" (they're not  
> safely published)
> 2 would need some lock/volatile anyway, bot only one is used  
> frequently, so IMHO 1 volatile is ok.
>
> I was thinking in using the same Piggyback technique you mention to
> safely publish the initialization constants,
> but I'm afraid the code will become more difficult to maintain and  
> more
> "unreadable", possibly breaking at the first next patch:
> IMHO using some unchanging fields "final" is the most clean and
> readable solution (and best performing), but I need a different
> constructor for that.
>
> your opinion is really welcome as I couldn't find other feedback,
> if you would like to take a look at the sources download the Search  
> trunk
> and look at:
> org.hibernate.search.store.FSSlaveDirectoryProvider
> or the FIXME in
> org.hibernate.search.reader.SharingBufferReaderProvider
>
> Sanne
>
> 2008/7/4 Pavitar Singh <pavitar.singh at gmail.com>:
> Hi Sanne,
>
> I don't think moving everything in constructor can guarantee safe  
> publication.
>
> From the JMM Specification Section 3.5
>
> "An object is considered to be completely initialized when its  
> constructor finishes. A thread that can only see a reference to an  
> object after that object has been completely initialized is  
> guaranteed to see the correctly initialized values for that object's  
> final fields."
>
> That means there are no guarantees for other fields which are non- 
> final.
>
> But once things are moved in constructor then by using Safe  
> Publication one can enforce visibility guarantees.
>
> Other thing i was wondering was why do one need to make every field  
> as volatile. As just by using a single volatile variable one can  
> enforce memory barriers. A technique documented in JCIP as Piggyback  
> Synchronizations and used by Doug Lea in implementing  
> ConcurrentHashMap.(You will find get method in ConcurrentHashMap is  
> without any locking and visibility is enforced by use of a single  
> volatile variable.)
>
> Also if you can elaborate more on how you are facing the visibility  
> issue then may be i can also spend time on it on figuring performant  
> solution.
>
> Regards
> Pavitar Singh
> http://pitfalls.wordpress.com
>
> On Fri, Jul 4, 2008 at 5:13 AM, Sanne Grinovero <sanne.grinovero at gmail.com 
> > wrote:
> Hello all,
> I'm sorry I've been very busy but as promised I would like to fix  
> HSEARCH-189
> (and others) very soon;
>
> I would like to propose a API extension (backwards compatible) that  
> would simplify the patch a lot:
> keeping it as is it is very tricky to fix the visibility issues in  
> FSSlaveDirectoryProvider
> and FSMasterDirectoryProvider without introducing a performance  
> penalty.
>
> I have these options to close the issue:
> 1) add a "volatile" to more than six fields per class (ugly and not  
> good for performance)
> 2) use some Locks/synch (more readable, still performance hits)
> 3) move the "initialize" arguments to a constructor.
>
> As Emmanuel knows I would really love the third option, but he's  
> worried about
> the fact we can't force a constructor in an interface*1, so my  
> proposal is:
>
> if we find there exists a constructor having the same arguments as  
> the initialize method,
> we use that, otherwise we use a no-arg constructor and then call the  
> initialize.
>
> Reflection is used anyway to instantiate these components,
> the code in DirectoryProviderFactory doesn't get much more complicated
> and much more cleanup is made possible in all DPs because of this
> (as the equals/hashcode comments already ask for).
>
> I actually think this same pattern is needed for other components,
> such as all ReaderProvider, so I hope you'll want to give it a try
> and let me apply it on other components too if you like the  
> resulting code.
>
> Sanne
>
> _______________________________________________
> hibernate-dev mailing list
> hibernate-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>
>
>
>
> -- 
> Pavitar Singh
> Blog: http://pitfalls.wordpress.com
>
> _______________________________________________
> hibernate-dev mailing list
> hibernate-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/hibernate-dev/attachments/20080704/133797d0/attachment.html