castor

Replace Hashtable and synchronized HashMap by ConcurrentHashMap to improve cache performance

Details

  • Type: Improvement Improvement
  • Status: Resolved Resolved
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.3.2
  • Fix Version/s: 1.3.3rc1
  • Component/s: Core
  • Labels:
    None
  • Number of attachments :
    1

Description

Description
In the org.castor.cache.* cache implementations Hashtable and synchronized HashMap will be used as underlying data structures.

Reason for change
Hashtable is a synchronized hash table implementation similarly to a HashMap, where all accesses will be synchronized.
Synchronization means locking and access serialization and thus, performance bottleneck.

Proposal
Replace the two hash table implementations by ConcurrentHashMap, which is part of Java since 1.5.

Benefits
ConcurrentHashMap allows for concurrent access by not locking the whole data structure, but only the so called buckets, which hold part of the data.
Compared to Hashtable in terms of scalability: ConcurrentHashMap provides linear scalability, while Hashtable provides exponential scalability.

Drawbacks
By simply introducing ConcurrentHashMap doesn't mean that all synchronizations can be eliminated, but rather that their usage can significantly be reduced. In order to allow for this thorough analysis and load tests are required.

Additional
This is a container issue for all concrete tasks required to improve the cache implementations.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Andras Hatvani added a comment -

References:

Show
Andras Hatvani added a comment - References: http://java.sun.com/javase/6/docs/api/java/util/concurrent/ConcurrentHashMap.html http://java.sun.com/javase/6/docs/api/java/util/Hashtable.html http://java.sun.com/javase/6/docs/api/java/util/HashMap.html http://www.ibm.com/developerworks/java/library/j-jtp07233.html
Hide
Permalink
Wensheng Dou added a comment -

In this patch, I replace Hashtable by ConcurrentHashMap in Unlimited cache. It is a small change.

Show
Wensheng Dou added a comment - In this patch, I replace Hashtable by ConcurrentHashMap in Unlimited cache. It is a small change.
Hide
Permalink
Ralf Joachim added a comment -

At CASTOR-3158 Wensheng asked:

After some investigations, I find that the Container (FastIteratingContainer and MapContainer) could be protected properly by AbstractHashbelt.lock(). Because we can't access the Container directly, we just access the Container through AbstractHashbelt, so the AbstractHashbelt.lock() will protect the Container. So I think the synchronized/locking on the Container (FastIteratingContainer and MapContainer) is not necessary.

But the AbstractHashbelt.lock() can't protect WeakReferenceContainer properly, because WeakReferenceContainer.get need to get the writeLock, this has different meaning with AbstractHashbelt.lock(). So synchronized/locking on WeakReferenceContainer is necessary.

Is there something wrong with this comment?

I think this is correct.

In addition I think you should check:

  • _timestamp of all containers is not synchronized/locked
  • At FastIteratingContainer _conatiner, _keys and _values may get inconcistent as they are not properly protected.

I haven't looked at WeakReferenceConatiner, reapers and hashbelt implementations in detail.

As far as I recall we have a bunch of tests that check that all methods work properly in static situation. But tests of concurrent access where the caches should bring advantages are missing.

Show
Ralf Joachim added a comment - At CASTOR-3158 Wensheng asked: After some investigations, I find that the Container (FastIteratingContainer and MapContainer) could be protected properly by AbstractHashbelt.lock(). Because we can't access the Container directly, we just access the Container through AbstractHashbelt, so the AbstractHashbelt.lock() will protect the Container. So I think the synchronized/locking on the Container (FastIteratingContainer and MapContainer) is not necessary. But the AbstractHashbelt.lock() can't protect WeakReferenceContainer properly, because WeakReferenceContainer.get need to get the writeLock, this has different meaning with AbstractHashbelt.lock(). So synchronized/locking on WeakReferenceContainer is necessary. Is there something wrong with this comment? I think this is correct. In addition I think you should check: _timestamp of all containers is not synchronized/locked At FastIteratingContainer _conatiner, _keys and _values may get inconcistent as they are not properly protected. I haven't looked at WeakReferenceConatiner, reapers and hashbelt implementations in detail. As far as I recall we have a bunch of tests that check that all methods work properly in static situation. But tests of concurrent access where the caches should bring advantages are missing.
Hide
Permalink
Wensheng Dou added a comment -

synchronized/locked for _timestamp of all containers is not necessary, because the access of _timestamp is protected by AbstractHashbelt.lock() properly, no extra sychronized is necessary.
I think FastIteratingContainer's _conatiner, _keys and _values don't need synchronized too, all of the fields are protected properly by AbstractHashbelt.lock().

Show
Wensheng Dou added a comment - synchronized/locked for _timestamp of all containers is not necessary, because the access of _timestamp is protected by AbstractHashbelt.lock() properly, no extra sychronized is necessary. I think FastIteratingContainer's _conatiner, _keys and _values don't need synchronized too, all of the fields are protected properly by AbstractHashbelt.lock().
Hide
Permalink
Ralf Joachim added a comment -

After a bunch of improvements done at various subtasks we can close this one also

Show
Ralf Joachim added a comment - After a bunch of improvements done at various subtasks we can close this one also

People

  • Assignee:
    Wensheng Dou
    Reporter:
    Andras Hatvani
Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: