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

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
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
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
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
@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

Raskayu Raskayu · Accepted Answer · 2016-08-18T06:16:04

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?

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

2 Answers