Which is better ConcurrentHashMap or Synchronized Hash Map in this case?

Question

I am writing a utility class which uses map as a cache store. Now since its going to be used in multithreaded environment. I came up with usage of either synchronized hashmap while doing put operation or ConcurrentHashMap( using putIfAbsent) which I am still confused about if it is prone to override of key-value(though key-value is unique in my case) and both have there pros and cons. So I am not able to decide. There might be some other cache store which i can use for this purpose, do suggest but I am more interested in knowing which is to be used CHM or Hashmap if this is the only option.

In Comments of program is CHM usage which I thought else I have used HashMap.

public final class DateTime {

  private static final Map<CountryLanguage, Locale> localeMap = new HashMap<>();

 /*private static final Map<CountryLanguage, Locale> localeMap = new ConcurrentHashMap<>();*/

public static String getDateTime1(String pattern, LocalDateTime localDateTime, String language,
      String country) {
    if (language == null || language.isEmpty()) {
      throw new NullPointerException("Language cannot be null or empty");
    }
    CountryLanguage countryLanguage = new CountryLanguage(language, country);
    if (!localeMap.containsKey(countryLanguage)) {
      synchronized (localeMap) {
        // Putting double lock
        if (!localeMap.containsKey(countryLanguage)) {
          for (Locale locale : Locale.getAvailableLocales()) {
            if (locale.getLanguage().equals(language) && locale.getCountry().equals(country)) {
              localeMap.put(new CountryLanguage(language, country), locale);
            }
          }
        }
      }
    }
      /*for (Locale locale : Locale.getAvailableLocales()) {
        if (locale.getLanguage().equals(language) && locale.getCountry().equals(country)) {
          localeMap.putIfAbsent(new CountryLanguage(language, country), locale);
        }
    }*/
    Locale locale = localeMap.get(countryLanguage);
    if (locale == null) {
      locale = Locale.ROOT;
    }
    DateTimeFormatter dateTimeFormatter = DateTimeFormatter.ofPattern(pattern, locale);
    return localDateTime.format(dateTimeFormatter);
  }

NOTE:

I have an inner class CountryLanguage which has String country,String language as member variable and both hashCode and equals methods have been overridden.

Edit1: I am not making whole map as synchronized I am just using synchronized on map while put operation. And I am using double check to make sure no two key-value exists

What would be the issue with adding the same `CountryLanguage` twice? — Marvin, Aug 18 '16 at 06:17
@Marvin: No issues but how can Map contains same key twice. I think concurrent hashmap can have multiple same key-value. — balboa_21, Aug 18 '16 at 06:20
You're not using synchronization correctly. If shared mutable state (the map) is accessed by several threads, then **all** accesses to that state, reads and writes, must be synchronized. Not just the writes. — JB Nizet, Aug 18 '16 at 06:25
@JBNizet: But if **synchronized is over map object** itself, wouldn't it make all the operations safe ? — balboa_21, Aug 18 '16 at 06:34
The map itself would be safe. Your code would not necessarily be. But your map is not synchronized anyway, so... — JB Nizet, Aug 18 '16 at 06:42
Skip the synchronization entirely and just put the `CountryLanguage`. You'll have more issues when you remove elements, but so far you don't. If you need a reliable cache better look at existing solutions, e.g. Guava. — Marvin, Aug 18 '16 at 06:44
@JBNizet: It would be very helpful if you can elaborate `Your code would not necessarily be.` I know there is something wrong with it but I am not able to create an environment where this will break.In short, what would happen if one thread will try to get access map[outside synchronized block] **but if map is locked** so will that thread wait for getting access over that map. If yes, what is wrong with it. And if No, will there be an exception ? — balboa_21, Aug 18 '16 at 06:51
*but if map is locked*: in your code, it's **not** locked, since you're using a regular, non-synchronized map, and you're not synchronizing your read operations. If you **were** using a synchronized map, then the map itself would be safe, but test-then-put operations, which are not atomic, could still make your code unsafe. That's why ConcurrentHashMap has such atomic operations, like putIfAbsent(). — JB Nizet, Aug 18 '16 at 06:56
@JBNizet: As per [doc](https://docs.oracle.com/javase/tutorial/essential/concurrency/locksync.html). _When a thread invokes a synchronized method, it automatically acquires the intrinsic lock for that method's object and releases it when the method returns. The lock release occurs even if the return was caused by an uncaught exception._ I know its about synchronized method but wouldn't same gets applied for objects on which synchronized is applied. — balboa_21, Aug 18 '16 at 07:07
@Marvin: Yes you are right, as of now I am going ahead with your mentioned comment. But I need to know more about the behaviour of ConcurrentHashMap vs my approach of HashMap. Thanks. — balboa_21, Aug 18 '16 at 07:10
Yes, it acquires the lock. But you don't need to hold the lock to call a method of an object. You only need it to enter a block/method that is **synchronized** on that lock. When a synchronized method of object foo is executed, other threads must wait to call another **synchronized** method of foo. But calling another non-synchronized method is still allowed. — JB Nizet, Aug 18 '16 at 07:16
@JBNizet: Got it. Thanks. I didn't think in this direction. Now just last question: What would you do in my case as I don't see any fault with my hashmap approach. I have put double check and I don't want read operation to be synchronized. As far as I can imagine cases I dont see any break down. — balboa_21, Aug 18 '16 at 07:23
Your code is clearly broken, since you're not synchronizing reads. Some tests showing that it doesn't break are not sufficient: it could break the millionth time you use that class. I would just use a ConcurrentHashMap, and use putIfAbsent(). But in reality, I don't even understand the point of this whole thing. You could just use `new Locale(countryLanguage.getLanguage(), countryLanguage.getCountry())` every time you need the locale corresponding to a CountryLanguage. Creating a Locale is fast, and already uses a cache internally. — JB Nizet, Aug 18 '16 at 07:59
@JBNizet: Thanks. I will make the changes. _Creating a Locale is fast, and already uses a cache internally_ ;a big thanks for this. I did not dig deep, I **over-think** that going for new Locale() might have some performance issues or GC performance. — balboa_21, Aug 18 '16 at 08:25
@balboa making unnecessary complex changes in the name of optimization without measuring beforehand is pretty much *the* worst thing you can do. Hell without measuring before and after you can't even know if you didn't make the problem worse (which you almost certainly did in this case, purely from a performance point of view). Optimizing 101: **Always** measure first. — Voo, Aug 18 '16 at 08:30

score 5 · Answer 1 · edited May 23 '17 at 12:01

5

Synchronized HashMap：

Each method is synchronized using an object level lock. SO the get and put methods on synchMap acquire a lock.

Locking the entire collection is a performance overhead. While one thread holds on to the lock, no other thread can use the collection.

ConcurrentHashMap: (was introduced in JDK 5)

There is no locking at the object level,The locking is at a much finer granularity. For a ConcurrentHashMap, the locks may be at a hashmap bucket level.

The effect of lower level locking is that you can have concurrent readers and writers which is not possible for synchronized collections. This leads to much more scalability.

ConcurrentHashMap does not throw a ConcurrentModificationException if one thread tries to modify it while another is iterating over it.

So, I recommend you ConcurrentHashMap, it won't block all the "cahce" all the time.

If you want more info HashMap vs ConcurrentHashMap

EDIT:

Some other post about this: Is ConcurrentHashMap totally safe?

edited May 23 '17 at 12:01

Community

1
1

answered Aug 18 '16 at 06:16

Raskayu

735
7
20

Hi. I am not making whole hashmap as synchronized just putting it while put operation, if you will see the code you will find that I am using a synchronized block while put operation.And concurrenthashmap can have multiple same key-value pair what I think. – balboa_21 Aug 18 '16 at 06:22
2

@balboa_21 Just synchronized the put is incorrect, and no, a ConcurrentMap is a Map, so it can't have duplicate keys. – JB Nizet Aug 18 '16 at 06:27
@JBNizet: Yes, map would not have multiple same key-value pair. But if multiple thread do putIfAbsent(key,value) concurrently then ? Because as per ConcurrentHashMap doc retrieval is not thread safe. – balboa_21 Aug 18 '16 at 06:38
Everything in the concurrent package is thread-safe. The concurrent package is meant to contain... concurrent classes. – JB Nizet Aug 18 '16 at 06:41
@balboa_21 as the following answer says, just control the return of putIfAbsent() http://stackoverflow.com/questions/14947723/is-concurrenthashmap-totally-safe – Raskayu Aug 18 '16 at 06:42

score 1 · Answer 2 · answered Aug 19 '16 at 15:19

I would implement this using a ConcurrentHashMap:

public final class DateTime
{
    private static final ConcurrentMap<CountryLanguage, Locale> localeMap = new ConcurrentHashMap<>();

    public static String getDateTime1(String pattern, LocalDateTime localDateTime, String language, String country)
    {
        if (language == null || language.isEmpty()) {
            throw new NullPointerException("Language cannot be null or empty");
        }

        CountryLanguage countryLanguage = new CountryLanguage(language, country);

        // See if it is already there
        Locale locale = localeMap.get(countryLanguage);
        if (locale == null) {
            // It's not there (probably), so look it up
            Local candidate = null;
            for (Locale l : Locale.getAvailableLocales()) {
                if (l.getLanguage().equals(language) && l.getCountry().equals(country)) {
                    candidate = l;
                    break;
                }
            }

            // If we found it, put it in the map
            if (candidate != null) {
                // It's possible that another thread beat us here, so use putIfAbsent()
                // and check the return value
                locale = localeMap.putIfAbsent(countryLanguage, candidate);
                if (locale == null) {
                    locale = candidate;
                }
            }
        }

        if (locale == null) {
            locale = Locale.ROOT;
        }

        DateTimeFormatter dateTimeFormatter = DateTimeFormatter.ofPattern(pattern, locale);
        return localDateTime.format(dateTimeFormatter);
    }
}

This is thread-safe and requires no external synchronization.

Which is better ConcurrentHashMap or Synchronized Hash Map in this case?

2 Answers2