Is it a thread-safe mechanism?

Question

Is this class thread-safe?

class Counter {
  private ConcurrentMap<String, AtomicLong> map = 
    new ConcurrentHashMap<String, AtomicLong>();
  public long add(String name) {
    if (this.map.get(name) == null) {
      this.map.putIfAbsent(name, new AtomicLong());
    }
    return this.map.get(name).incrementAndGet();
  }
}

What do you think?

Answer 1

Yes, provided you make the map final. The if is not necessary but you can keep it for performance reasons if you want, although it will most likely not make a noticeable difference:

public long add(String name) {
  this.map.putIfAbsent(name, new AtomicLong());
  return this.map.get(name).incrementAndGet();
}

EDIT

For the sake of it, I have quickly tested both implementation (with and without the check). 10 millions calls on the same string take:

• 250 ms with the check
• 480 ms without the check

Which confirms what I said: unless you call this method millions of time or it is in performance critical part of your code, it does not make a difference.

EDIT 2

Full test result - see the BetterCounter which yields even better results. Now the test is very specific (no contention + the get always works) and does not necessarily correspond to your usage.

Counter: 482 ms
LazyCounter: 207 ms
MPCounter: 303 ms
BetterCounter: 135 ms

public class Test {

    public static void main(String args[]) throws IOException {
        Counter count = new Counter();
        LazyCounter lazyCount = new LazyCounter();
        MPCounter mpCount = new MPCounter();
        BetterCounter betterCount = new BetterCounter();

        //WARM UP
        for (int i = 0; i < 10_000_000; i++) {
            count.add("abc");
            lazyCount.add("abc");
            mpCount.add("abc");
            betterCount.add("abc");
        }

        //TEST
        long start = System.nanoTime();
        for (int i = 0; i < 10_000_000; i++) {
            count.add("abc");
        }
        long end = System.nanoTime();
        System.out.println((end - start) / 1000000);

        start = System.nanoTime();
        for (int i = 0; i < 10_000_000; i++) {
            lazyCount.add("abc");
        }
        end = System.nanoTime();
        System.out.println((end - start) / 1000000);

        start = System.nanoTime();
        for (int i = 0; i < 10_000_000; i++) {
            mpCount.add("abc");
        }
        end = System.nanoTime();
        System.out.println((end - start) / 1000000);

        start = System.nanoTime();
        for (int i = 0; i < 10_000_000; i++) {
            betterCount.add("abc");
        }
        end = System.nanoTime();
        System.out.println((end - start) / 1000000);        
    }

    static class Counter {

        private final ConcurrentMap<String, AtomicLong> map =
                new ConcurrentHashMap<String, AtomicLong>();

        public long add(String name) {
            this.map.putIfAbsent(name, new AtomicLong());
            return this.map.get(name).incrementAndGet();
        }
    }

    static class LazyCounter {

        private final ConcurrentMap<String, AtomicLong> map =
                new ConcurrentHashMap<String, AtomicLong>();

        public long add(String name) {
            if (this.map.get(name) == null) {
                this.map.putIfAbsent(name, new AtomicLong());
            }
            return this.map.get(name).incrementAndGet();
        }
    }

    static class BetterCounter {

        private final ConcurrentMap<String, AtomicLong> map =
                new ConcurrentHashMap<String, AtomicLong>();

            public long add(String name) {
                AtomicLong counter = this.map.get(name);
                if (counter != null)
                    return counter.incrementAndGet();

                AtomicLong newCounter = new AtomicLong();
                counter = this.map.putIfAbsent(name, newCounter);

                return (counter == null ? newCounter.incrementAndGet() : counter.incrementAndGet());
            }
    }

    static class MPCounter {

        private final ConcurrentMap<String, AtomicLong> map =
                new ConcurrentHashMap<String, AtomicLong>();

        public long add(String name) {
            final AtomicLong newVal = new AtomicLong(),
                    prevVal = map.putIfAbsent(name, newVal);
            return (prevVal != null ? prevVal : newVal).incrementAndGet();
        }
    }
}

Answer 2

No one seems to have the complete solution, which is:

  public long add(String name) {
    AtomicLong counter = this.map.get(name);
    if (counter == null) {
      AtomicLong newCounter = new AtomicLong();
      counter = this.map.putIfAbsent(name, newCounter);
      if(counter == null) {
        counter = newCounter;
      }
    }

    return counter.incrementAndGet();
  }

Answer 3

I think you would be better off with something like this:

class Counter { 
  private ConcurrentMap<String, AtomicLong> map = new ConcurrentHashMap<String, AtomicLong>();

  public long add(String name) {
    AtomicLong counter = this.map.get(name);
    if (counter == null) {
      AtomicLong newCounter = new AtomicLong();
      counter = this.map.putIfAbsent(name, newCounter);
      if (counter == null) {
        // The new counter was added - use it
        counter = newCounter;
      }
    }

    return counter.incrementAndGet();
  }
}

Otherwise multiple threads may add simultaneously and you wouldn't notice (since you ignore the value returned by putIfAbsent).

I assume that you never recreate the map.

Answer 4

This solution (note that I am showing only the body of the add method -- the rest stays the same!) spares you of any calls to get:

final AtomicLong newVal = new AtomicLong(), 
                 prevVal = map.putIfAbsent(name, newVal);
return (prevVal != null? prevVal : newVal).incrementAndGet();

In all probability an extra get is much costlier than an extra new AtomicLong().

Answer 5

What about this:

class Counter {

  private final ConcurrentMap<String, AtomicLong> map = 
    new ConcurrentHashMap<String, AtomicLong>();

  public long add(String name) {
    this.map.putIfAbsent(name, new AtomicLong());
    return this.map.get(name).incrementAndGet();
  }
}

• The map should be final to guarantee it is fully visible to all threads before the first method is invoked. (see 17.5 final Field Semantics (Java Language Specification) for details)
• I think the if is redundant, I hope I'm not overseeing anything.

Edit: Added a quote from the Java Language Specification:

Answer 6

EDIT

Yes if you make the map final. Otherwise, it's not guaranteed that all threads see the most recent version of the map data structure when they call add() for the first time.

Several threads can reach the body of the if(). The putIfAbsent() will make sure that only a single AtomicLong is put into the map.

There should be no way that putIfAbsent() can return without the new value being in the map.

So when the second get() is executed, it will never get a null value and since only a single AtomicLong can have been added to the map, all threads will get the same instance.

[EDIT2] The next question: How efficient is this?

This code is faster since it avoids unnecessary searches:

public long add(String name) {
    AtomicLong counter = map.get( name );
    if( null == counter ) {
        map.putIfAbsent( name, new AtomicLong() );
        counter = map.get( name ); // Have to get again!!!
    }
    return counter.incrementAndGet();
}

This is why I prefer Google's CacheBuilder which has a method that is called when a key can't be found. That way, the map is searched only once and I don't have to create extra instances.