7

Let's say I have a Concurrent Map that is high-read, low-write, and needs to store application data:

ConcurrentMap<UUID, Data> map = new ConcurrentHashMap<UUID, Data>();

Then, during startup and through user input, data is added to the map:

public void createData(Data newData) {
    map.put(newId, newData); // etc...
}

If I then need to change the data, should I:

A) Make the Data class objects immutable, and then conduct a put operation every time a change is needed for a Data object:

public void changeData(UUID oldId, Foo newInfo) {
    Data oldData = map.get(oldId);
    Data newData = new Data(oldData, newInfo); // Constructor for demo only
    map.put(newData);
    saveToDatabase(newData);
}

B) Make the Data class objects mutable yet thread-safe with volatile fields, atomic references or final concurrent fields, and simply modify the object as needed:

public void changeData(UUID oldId, Foo newInfo) {
    Data data = map.get(id);
    data.changeSomething(newInfo);
    saveToDatabase(data);
}

C) None of the above

oberger
  • 1,217
  • 2
  • 16
  • 31
  • 1
    Depends on what you want to achieve :) – zapl Aug 20 '13 at 16:23
  • I want to achieve a get/put relationship consistent with the ConcurrentHashMap's guarantees, but I'm just not sure the order or correct way of doing object manipulation/creation. – oberger Aug 20 '13 at 16:30
  • 1
    If you go the immutable route and have multiple writing threads, you may want to use the replace method instead of put to ensure that you are replacing the value that you modified. – Michael Krussel Aug 20 '13 at 16:51
  • Okay, I will look into replace, and it looks like that method is atomic also, great. – oberger Aug 20 '13 at 17:05

3 Answers3

7

A) is the better option, for two reasons:

  1. Since in your scenario reads are more frequent, you should reduce the amount of overhead for them. Adding additional synchronization (such as volatile) works against you in this case.
  2. By using mutable objects with additional custom safeguards (which may have bugs) you're pretty much defeating the point of making your life easier by using ConcurrentHashMap.
mikołak
  • 9,605
  • 1
  • 48
  • 70
3

If you have an option of making an immutable class, you would be much better off with your implementation #A: in-place modifications are significantly harder to implement and maintain.

Sometimes going the immutable route may not be an option, because of the need to make frequent modifications to a relatively large object. In this case you may want to reconsider the application of the concurrent hash map to your design, because the fact that it is synchronized does not give you too much an advantage.

Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
  • Although I accepted the other answer because it helped me more for this question, I appreciate your response as far as large mutable objects and their place (not in concurrent hash maps), which solidified some other thinking for me. Thanks. – oberger Aug 20 '13 at 17:06
3

Just a thought. You stated that the write rate is low, but for the sake of the argument let's suppose multiple concurrent writes / calls of the changeData method. It is then possible that the thread that called the method the last, finishes first (in both approaches).

If your application logic assumes that the order of insertion will be honored it may yield wrong results. In that case, the body of the method changeData is your critical section which per definition means that it should not be executed concurrently.

Critical section definition is highly sensitive to application domain semantics and the code structure, so I can't really tell if that method is to be considered a critical section. Guessing by the names of the variables, and supposing that your map is a user data cache from database, I'd guess that you can disregard this answer. But do carefully think of it, though :)

If all the writes go through this method, this would be a sketch of the code (you can use non thread safe map implementation then):

public void changeData(UUID oldId, Foo newInfo) {
    synchronized(SomeClass.class) { // global lock
        //update logic
    }
}

This is just a sketch to illustrate the point of course. You can most probably use some of the Java concurrent constructs, if this is the problem.

linski
  • 5,046
  • 3
  • 22
  • 35