4

I have been reading about volatile and synchronized in Java but have been scratching my head in confusion. I am hoping someone can help me clear up a problem

private HashMap<String,int> map = new HashMap<String,int>();

In my thread

if (map.get("value") == null)
{
map.put("value",0);
}
map.put("value",map.get("value")+1);

My goal is to have all the threads share this map. If I add volatile it doesn't seem to fix the problem for me (I output and see that map is being override to each time). I then tried using ConcurrentHashMap and adding volatile in front of that... that also didn't seem to work. Based on my reading about volatile my understanding is that it should "lock" access to the map when map is being written to and then when map is done being written to the lock is released.

So... then I tried adding static

private static ConcurrentHashMap<String,int> map = new ConcurrentHashMap<String,int>();

and that seems to work perfectly... But... I keep reading that using static isn't the right way due to something about 'contention' (which I don't quite understand)

Thanks in advance

Bhesh Gurung
  • 50,430
  • 22
  • 93
  • 142
K2xL
  • 9,730
  • 18
  • 64
  • 101

3 Answers3

10

Volatile won't help here. Volatile is useful to solve visibility problems, but you are facing another problem: atomicity.

Oh, and volatile has absolutely nothing to do with locking. It won't acquire a lock when reading/writing, it won't release anything ever. What it does is this: all the actions that happened-before a write to a volatile field will be visible to every other thread, after they read the same volatile field. There's no locking involved (they are similar in that the memory effects of releasing/acquiring a lock are exactly the same).

The operations get and set are not atomic, meaning that other things may happen between the two.

For instance, one thread will get the value, then ANOTHER thread will get the same value, both will increment the value, then the first will set the new value, then the second will do the same. The final result is not what you expected.

The most common solution to this problem is to serialize access (ie, synchronize) to the shared variable, or to use compare-and-set (CAS) (so you won't need to do synchronization).

1. synchronized

private final Map<String, Integer> m = new ConcurrentHashMap<String, Integer>();
synchronized incrementValue(final String valueName) {
  m.put(valueName, m.get(valueName) + 1);
}

Note that if you use this solution then EVERY ACCESS to the map must synchronize on the same lock.

2. CAS

Many CAS algorithms are already implemented in the JVM, in a very performatic way (ie, they use native code, and the JIT may use instructions specific to the processor, that you cannot access in other ways -- check the class Unsafe in Sun's JVM for example).

One class that might be useful to you here is AtomicInteger. You can use it like this:

private final Map<String, AtomicInteger> m = new ConcurrentHashMap<String, AtomicInteger>();
incrementValue(final String valueName) {
  m.get(valueName).incrementAndGet();
}

What a CAS algorithm will do is something like this:

for (;;) {
  state = object.getCurrentState();
  if (object.updateValueAndStateIfStateDidntChange(state)) {
    break;
  }
}

It is assumed that the method updateValueAndStateIfStateDidntChange is atomic and will return true only if it was able to update the value. This way, if another thread modifies the value after you get the state and before you update the value, the method will return false and the loop will try it again.

Assuming you can implement that method in such a way that won't use synchronized (and you can, through the use of classes in java.util.concurrent), you will avoid contention (which means threads waiting to obtain locks held by another threads) and you may see a general improvement in performance.

I use a lot this kind of thing in distributed task execution system I wrote. The tasks must all be executed exactly once, and I have lots of machines executing tasks. The tasks are all specified in a single MySQL table. How to do it? You must have a column which purpose is to allow the implementation of CAS. Call it executing. Before starting the task, you must do something like: retrieve the next task, "update tasks set executing = 1 where id = :id AND executing = 0" and count the number of updated lines. If you updated 0 lines, it is because another thread/process/machine has already taken that task (and successfully executed that "update" query); in this case, you forget it and try the next task, because you know that this one is already being executed. If you updated 1 line, then it is good to go, you can execute it.

Another place where I use a lot this idea of CAS is in a very dynamic (in respect to its configuration) resource pool I wrote (I use it mostly to manage "connections", ie, sockets, but it is sufficiently generic to hold any kind of resource). Basically, it counts how many resources it is holding. When you try to acquire a resource, it reads the counter, decrements it, tries to update it (if nothing else modified the counter in between), and if this is successful, then you can simply take a resource from the pool and lend it (once the counter reaches 0, it won't lend a resource). If I ever publish this code, I will be certain to add a link to it here.

Community
  • 1
  • 1
Bruno Reis
  • 37,201
  • 11
  • 119
  • 156
  • thanks for the quick response. still a bit confused on one part: what is the benefit of synchronized over making the map static? when i made the map static i didn't run into problems where one thread overwrote changes that another thread made (or maybe i was just lucky) – K2xL Dec 15 '11 at 05:41
  • Be careful: `static` has absolutely nothing to do with thread-safety. The only keywords associated to thread-safety are `final` (in case of fields), `volatile`, `synchronized` and `synchronize(...)`. – Bruno Reis Dec 15 '11 at 05:46
  • So if I did private static final map would that have everything work as intended? what would be the drawbacks? – K2xL Dec 15 '11 at 05:52
  • If you use `static` to share a variable among threads, then you must ensure that all operations are atomic (here, they aren't: you are doing a get, then updating, then a set, 3 operations in total) and that you won't have visibility problems (here, the use of ConcurrentHashMap guarantees that visibility is ok). – Bruno Reis Dec 15 '11 at 05:56
  • In your specific case, since you didn't give more details, I cannot say why `static` **seemed** to solve your problems. From what you said, I might infer that each thread uses a different instance of the class containing the map, and if this is the case, without static each thread will have a different map; by using `static`, you are dissociating the map from the specific instance (ie, all instances share the same map), then you will make the threads share the same map. But, as I said: this won't solve concurrency problems. There are visibility and atomicity problems to deal with. – Bruno Reis Dec 15 '11 at 05:58
  • i am starting to see... basically i just have a series of nested hashmaps and need to increment some of it's values (a bunch of "worker" threads are going to be doing so)... i have a feeling though i'll probably have to increment local maps on each thread and then "flush" to some global memory map (since i imagine doing synchronized is slow).. or am i wrong in my assumptions? – K2xL Dec 15 '11 at 06:04
  • K2xL, if try and tell us exactly *what* you want to do, and not *how* you want to do it, it will be easier to suggest a correct (ie, thread-safe) and elegant solution. – Bruno Reis Dec 15 '11 at 06:06
  • i have a large nested hashmap... protected static ConcurrentHashMap>> Data. What I want to do is increment one of the inner HashMap's keys and values (the String,Double one), basically a value key and a double value. Each thread is passed a value which is to be incremented on that key.. – K2xL Dec 15 '11 at 06:12
  • let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/5866/discussion-between-k2xl-and-bruno-reis) – K2xL Dec 15 '11 at 06:39
0

Volatile ensures that threads are not caching this value. Volatile ensures write / read of this value from memory is atomic but it doesn't provide locking of this variable over multi step operation.

Replace Map with ConcurrentHashMap and use atomic CAS operation.

Use : ConcurrentHashMap.putIfAbsent()

Amrish Pandey
  • 800
  • 1
  • 7
  • 11
0

The answer is of course to use AtomicInteger, not int.

And if you use a ConcurrentHashMap, you need to use its special thread-safe method putIfAbsent().

Here's a javadoc excerpt from that method:

This is equivalent to

if (!map.containsKey(key))
    return map.put(key, value);
else
    return map.get(key);

except that the action is performed atomically.

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

AtomicInteger value = map.get("value");
if (value == null) {
    map.putIfAbsent("value", new AtomicInteger());
    value = map.get("value");
}
value.incrementAndGet();

Note how you don't have to put the new value back.

If you make that change to your code, it should all work just fine.

Bruno Reis
  • 37,201
  • 11
  • 119
  • 156
Bohemian
  • 412,405
  • 93
  • 575
  • 722
  • This won't solve the `map.put(x, map.get(x)+1)` part: it is still not atomic, and `putIfAbsent` wont help because, obviously, it is not absent anymore! It will solve the `if (map.get(x) == null) map.put(x, y);` part though. – Bruno Reis Dec 15 '11 at 06:00
  • 1
    @BrunoReis Check now. My solution will work, is threadsafe and is just a couple of lines. I just realised I'm basically plagiarising your answer, but I didn't fully read your answer - it wasn't intentional :) – Bohemian Dec 15 '11 at 06:41
  • btw, what if i wanted to increment by a value greater than 1? or a double instead of an int? – K2xL Dec 15 '11 at 06:50
  • Question 1 : See the AtomicInt.addAndGet(delta) method then. Question 2 : The documentation recommands you to use AtomicLong coupled with Double.doubleToLongBits, see here at the bottom :http://docs.oracle.com/javase/1.5.0/docs/api/java/util/concurrent/atomic/package-summary.html – n1r3 Dec 15 '11 at 13:46
  • @BrunoReis you should also update the map construction, you wrote "new HashMap()" which you may want to replace by "new ConcurrentHashMap()" – n1r3 Dec 15 '11 at 13:50