Geek asks and answers

Is this a thread safe mechanism?

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?

+3
source share
6 answers

Yes, if you make the card final. If this is not necessary, but you can save it for performance reasons, if you want, although this most likely will not lead to 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 this, I quickly tested the implementation (with and without verification). 10 million calls in one line accept:

  • 250 ms with check
  • 480 ms without verification

Which confirms what I said: if you do not call this method millions of times, or if it is in the performance-critical part of your code, it does not matter.

EDIT 2

- . BetterCounter, . ( + ) .

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

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();
        }
    }
}
+6

, final. , , add().

if(). putIfAbsent() , AtomicLong.

, putIfAbsent() , .

, get(), null, AtomicLong, .

[EDIT2] : ?

, :

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();
}

Google CacheBuilder, , , . , , .

+2

, :

  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();
  }
+1

:

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();
  }
}

: ​​ Java:

0
source

I think you will 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 be added at the same time and you would not notice (since you are ignoring the value returned by putIfAbsent).

I guess you never recreate a map.

0
source

This solution (note that I only show the body of the method add- the rest remains unchanged!) Saves you from any calls get:

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

In all likelihood, extra is getmuch more expensive than extra new AtomicLong().

0
source

More articles:


All Articles