[hibernate-dev] Re: HSEARCH-178

[Emmanuel Bernard]

On  Apr 14, 2009, at 16:22, Sanne Grinovero wrote:

> I'm going for the Map approach, to favor cleaner code and because you
> appear to be more comfortable with it.
> Even so there are two more points to consider:
>
> 1)If in the future a code change of the Synchronization (stored in the
> WeakMap as a value)
> would be changed in any way to get a (hard) reference to the session,
> we'll get a memory leak, as in HSEARCH-314.
> As much as the threadlocal is dangerous, we have to be carefull with
> the WeakMap.

Yes I know. Ideally we would add that to the Synchornize JavaDoc,  
except we can't :)
Add a warning to PostTransactionWorkQueueSynchronization and  
IndexWorkFlushEventListener#addSynchronization


>
>
> 2)Performance: you are reminding this is just a workaround for people
> which don't like transactions, but the flushlistener is registered for
> everybody.
> So nothing will be put in the map (skipping the Synchronization  
> registration) in
> a correct usage case, but still during the flushlistener I've to  
> look into
> the map to verify there's nothing to do.
> I'm not worried about the performance impact of looking into an  
> empty map,
> but felt I had to keep you informed.

I guess we could add a volatile flag marking the activation but i  
don't think it is worth the effort.

>
>
>
> 2009/4/14 Emmanuel Bernard <emmanuel at hibernate.org>:
>>
>> 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.
>>