-2

I have a CHM 

private ConcurrentHashMap<Integer,Integer>pR = new ConcurrentHashMap<>();

I have a Method that increments its value 

public void incrementPR(int count){

        Integer value = this.pR.get(count);
       if(value == null){
           this.pR.put(count,1);
       } else {
           this.pR.put(count,value+1);
       }
    }

This method is called from an endpoint using jmeter if i fire 500 concurrent request the value that the hashmap holes its not 500 but 437 , 430 etc its not behaving in thread safe manner , how do we acheive thready saftey for the same

Rahul
  • 427
  • 3
  • 7
  • 16

2 Answers2

1

You break "get current value" and "increment it" and "save it back to map" in some statements. Now, when these statements run concurrently you got wired result. Your failure is not related to the concurrent map. You should use some concurrency control mechanism for your code (like semaphore, lock..). For further information refer to this question: Java Concurrency Incrementing a Value

If you interested in efficient solution (not using Lock mechanism) for your intent, you can use AtomicInteger:

AtomicInteger atomicInteger = new AtomicInteger();
//when you want to add your number, use this code
atomicInteger.getAndAdd(1);

But if you have heavy load on this statement, using an AtomicLong can be a bottleneck (because it uses compare-and-swap cpu instruction). In this situation it's better use LongAdder as:

LongAdder longAdder = new LongAdder();
//when you want to add your number, use this code
longAdder.increment()
m.ghoreshi
  • 782
  • 5
  • 12
  • you are telling about synchronized right ? is there any other better approach to this – Rahul Jun 17 '17 at 21:24
  • How do i implement this in my map which expects an integer as an input ? – Rahul Jun 17 '17 at 21:48
  • Just declare map as `Map` now use `map.get(myKey).increment()` when you need to increment your value. – m.ghoreshi Jun 17 '17 at 21:56
  • if no key is present will it insert key in thread safe manner – Rahul Jun 17 '17 at 22:00
  • I suppose your keys are your metrics, in this case, you can have many thread-safe counters just using the mentioned map: `map.get("numberOfX").increment();` or `map.get("numberOfY").increment()` and so on. – m.ghoreshi Jun 17 '17 at 22:22
0

When a class is described as 'thread-safe' what is usually meant is that the class will ensure sufficient exclusion between concurrent calls to its methods to maintain class integrity (and relevant invariants).

What it doesn't mean is that it will ensure another class can't modify it between calls.

In your case if one thread calls this.pR.get(count); and then another calls it before the call to put , they are in a race as to which will update the object and it appears 'lose' counts.

You need to put some synchronization around calls to incrementPR.

The easy and obvious answer in this case is:

public synchronized void incrementPR(int count){
    //....
}

In fact with synchronization there it may not be necessary to use a synchronized map. That isn't clear from the snippet provided.

Persixty
  • 8,165
  • 2
  • 13
  • 35
  • yup i used synchronized and it works fine , i was hoping for some better approach as i feel that synchronization leads to performance impact and though not obvious it will refect when other variables are included in the model – Rahul Jun 17 '17 at 21:29
  • which is better lock or synchronization in this case ? – Rahul Jun 17 '17 at 21:32
  • @Rahul I've provided the easiest answer but in fact I would recommend a lock. I believe that `synchronized` is a poor design in Java and you should explicitly declare and use `Lock` objects. Providing every object with a monitor and the method bloat (and possible performance bloat) in doing that was quite unnecessary. In a good implementation you should be able to achieve the same performance with either unless you need to experiment with lock types in which case `synchronized` fails pretty quickly. – Persixty Jun 17 '17 at 21:47
  • the link mentioned in the duplicate marked worked without any lock or synchronized – Rahul Jun 17 '17 at 22:06
  • @Rahul Which one? One of them is broken and they all use some kind of synchronization. Implicit or explicit. I was proposing the shortest cut to safety for you. Premature optimization is the root of all evil. I wouldn't sweat a simple solution until you think it's a bottleneck. – Persixty Jun 18 '17 at 07:11
  • the by @ZhekaKozlov its working fine, should i use that – Rahul Jun 18 '17 at 17:03
  • That one works and is guaranteed by the documentation. Whether that is better depends on the profile of what your program is doing and particularly load and contention. In some cases (many realistic cases) it won't have the best performance. `AtomicLong` may suit better than `LongAdder`. – Persixty Jun 18 '17 at 17:14