[hibernate-dev] Re: HSEARCH-178

[Emmanuel Bernard]

On  Apr 14, 2009, at 13:03, Sanne Grinovero wrote:

> I've 2 inline answers:
>
> 2009/4/14 Emmanuel Bernard <emmanuel at hibernate.org>:
>> Read inline
>>
>> On  Apr 13, 2009, at 17:22, Sanne Grinovero wrote:
>>
>>> After our chat about the topic I thought that I only needed some  
>>> minor
>>> changes,
>>> was quite wrong.
>>>
>>> I moved the flush listener to the usual  
>>> FullTextIndexEventListener, using
>>> delegation at first as we had agreed. This got me into troubles with
>>> the Serialization test of the FullTextEntityManager, I had to adapt
>>> EventSourceTransactionContext changing a field to transient and
>>> having the code manage the case in which the values are lost by
>>> deserialization.
>>> After this I removed the delegation moving the code to the
>>> FullTextIndexEventListener
>>> which resulted in simpler code.
>>>
>>> You made me think about what was happening in case of an error  
>>> during
>>> the flush processing in the default listener, so I've replaced the  
>>> Map
>>> with a combination
>>> of non-static ThreadLocal with a weak reference to the flushing  
>>> Session,
>>> which is checked for == identity to make sure the synch is  
>>> relevant to
>>> the same session,
>>> in case of a previously not cleaned-up flush by the same Thread  
>>> (as in
>>> a Map, but I only need
>>> the last stored value for the current session, and only for the same
>>> thread).
>>
>> I think I don't like the ThreadLocal approach. What is your  
>> reasoning for
>> using a thread local variable? What would make it less compelling  
>> if we were
>> not using a TL?
>
> the two event listeners "on-post-somethingchanged" and "flush" are  
> executed
> one after the other right away, atomically from a out-of-hibernate- 
> search point
> of view. In the test code if I put breakpoints in both the "register
> synch" routing
> and "find the synch from the flushlistener" thay both happen only when
> the client code is asking to flush.
> This made me think the two routines are always being called by the  
> same thread,
> and there is no chance to begin doing something else between the  
> first operation
> and the second operation: it's not possible that, for example, the  
> Session is
> passivated or suspendend (thinking about long running conversations).
> The only way to stop the second to happen is having
> an exception in the default hibernate flush listener: that's the
> reason for the weak
> reference to Session.
> These thoughts leaded me to think that a concurrent map is overkill,  
> especially
> considering that every thread in the application is to access
> this map twice per flush: to put in an object, and get it back
> immediately after that,
> and no thread trying to say anything to the others or wanting to store
> it for a longer
> time. So I thought the ThreadLocal was the natural answer.
> Still the resulting code is ugly, I'm happy to forget my performance
> thoughts if we
> could get with a cleaner solution.
> I've another version ready using a WeakIdentityHashMap, it's also
> passing the tests,
> and is a bit more readable, but I'm unsure about which is better.
> Shall I commit the one using WeakIdentityHashMap instead of  
> ThreadLocal?

I looked that the TL impl in Java SE and they are hosted on a per  
Thread object basis, so I guess there is no concurrency lock here. So  
it all boils down to which is faster:
  - a concurrent map look up access
  - getting the current Thread object + a map lookup (internal TL  
variable impl)

Up to you. I have the tendency to limit my TL use as it has caused  
issues in the past.
Remember this part of the code is used only as a workaround for people  
misconfiguring HSearch so I am not too fussed about the perf.