castor
  1. castor
  2. CASTOR-2940

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

        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
        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
        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
        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
        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
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: