CODEWITHSUNDEEP
javaandroidmultithreadingvolatileconcurrenthashmap
Prilaga
Prilaga

Reputation: 857

ConcurrentHashMap as singletone cache with synchronized

how do you think, do we need to use synchronized block for better optimization of access to instance of Ad? The instance of Ad.class can be retrieved from different threads. Synchronized helps to get an instance in one time with one get operation from ConcurrentHashMap. ConcurrentHashMap store all values as volatile. I use it on java 1.7 for android, computeIfAbsent is available in java 1.8.

It will be great to get detailed answer, why not or why yes. Thank you!

public final class Ad {

    private final static Map<String, Ad> ads = new ConcurrentHashMap<>();

    public static Ad get(@NonNull String appId) {
        if (appId == null) appId = "";

        boolean containsAd = ads.containsKey(appId);

        Ad localInstance = containsAd ? ads.get(appId) : null;

        if (localInstance == null) {
            synchronized (Ad.class) {

                containsAd = ads.containsKey(appId);

                localInstance = containsAd ? ads.get(appId) : null;

                if (localInstance == null) {
                    localInstance = new Ad();
                    localInstance.setAdId(appId);
                    ads.put(appId, localInstance);
                }
            }
        }
        return localInstance;
    }

    private Ad() {
    }
}

UPDATE: Thanks to all for help. I replaced ConcurrentHashMap to HashMap.

Upvotes: 2

Views: 1457

Answers (2)

Eugene
Eugene

Reputation: 120998

From what I understand what you actually want to achieve is putIfAbsent and as such this is much simpler then what you do (your are using a double check locking):

public static Ad get(String appId) {
    String newId = appId == null ? "" : appId;
    ads.putIfAbsent(newId, new Ad());
    return map.get(newId);
}

Upvotes: 0

Matt Timmermans
Matt Timmermans

Reputation: 59303

This is not quite optimal. If multiple threads try initialize values at the same time, then they will block each other, even if they are looking for different keys.

You should use ConcurrentHashMap.computeIfAbsent to check for the add and create missing ones in a single step. That way you will not create any Ads that aren't used, and two threads will only block each other if they're trying to initialize the same entry:

public static Ad get(@NonNull String appId) {
    if (appId == null) appId = "";

    return ads.computeIfAbsent(appId, Ad::new);
}

private Ad(String appId) {
    this();
    setAdId(appId);
}

Upvotes: 1

Related Questions