On Nov 14, 2007, at 11:34, Chris Bowditch wrote:

Andreas L Delmelle wrote:

Hi Andreas,

A splendid idea!
I already thought my approach to be lacking somewhere, although it was too late last night to see where exactly. First off, the flag should, of course be declared 'volatile'. If not, then there is no guarantee a thread will see the most recent state. More importantly, there is the matter of cleanups that were already running at the time rehash() started acquiring its locks, as you point out. Introducing another dummy Object to synchronize on, seems like a very clean solution. The flag could still be used to avoid unnecessary cleanups or rehashes from being started. Synchronizing on a mutual object would still launch them, but they would merely have to wait.

I'm not sure another object to synchronize on is required as the 2 threads both already synchronize on the segments. I'm not familar with the PropertyCache and I was expecting you to say that the rehash method couldn't start whilst existing cleanups were running because of the sync on segments. But I guess if that was working then we wouldn't need the boolean flag either :-/

The picture is roughly:
If the size of one segment grows too large, a cleanup thread is launched (actually suboptimal -> see Jeremias' response: much better to have only one single thread; I'll look into this asap). That cleanup thread only locks the segment of the cache it is dealing with (just as put() does).

A rehash() OTOH needs a lock on the whole cache, not just one segment, so what it does is acquire these locks one by one. If there would be 10 segments, it first locks segment 9, then 8, then 7 and so on. Between the point where the lock on segment 9 is obtained, and the point where the whole cache is locked (segment 0), a cleanup thread could still beat rehash() to acquire the lock on segment 1 for example.

What you were expecting is /almost/ what happens: once the rehash method has acquired all the locks, any concurrent thread that needs synchronized access must wait until the rehashing is complete.

It's not clear to me from a quick glance at the code exactly what segments within the Property Cache are,

They are merely objects to distribute the synchronization, such that two concurrent threads do not have to wait for each other if they need access to different hash-buckets.

but gut feel is that synchronization between the 2 threads needs more work.

I share this gut feel. I decided to commit anyway a few months ago, since I gathered that to be /the/ way to find out what its effects are in real-time scenarios.

For now I will just leave the null check in place since I don't have time to persue this issue.

Regarding your observation of the OutOfMemoryError: this might need investigating. Do you have any indication as to which types of objects are leaking?

Yes I have been running a profiling tool on FOP this morning and discovered that the PropertyCache$CacheEntry inner class is the one that seems to grow and grow (its +89,000 instances after 1100 documents), but you knew that already ;)


Reply via email to