Java Synchronisation - Do I need it for this simple method?

Question

Hi All,

I have a simple method which is called from multiple threads;

@Override
public Bitmap getFullBitmap(Filter f, ProgressCallback<String> pc) {
 // Requires synchronisation?
 Bitmap bitmap = fullMap.get(f.id);
 if(bitmap == null){
  f.setProgressCallback(pc);
  bitmap = f.e.evaluate(currentBitmap);
  fullMap.put(f.id, bitmap);
 }
 return bitmap;
}

As none of the objects used are fields of the class (Apart from fullMap) is it ok just to call this or might one thread change the value of bitmap for example, while the method is executing?

fullMap is a SoftHashMap which maintains SoftReferences of Bitmap objects indexed but the Filter's id that was used to create it. If that makes any sense.

I haven't had any problems but I thought I might need it.

Please ask for clarification if this is not clear, the question makes sense in my head ;)

EDIT

• currentBitmap is an object of type Bitmap, there is one bitmap in the system which is considered current and it's managed by this class.
• This code forms a very basic cache, the bitmap returned will always be the same for each id and is not modified outside of this method.
• By using Soft References in a SoftHashMap as descibed by Dr Heinx and a FIFO queue of hard references for the 10 most recently added I hope to avoid expensive calls to f.e.evaluate. That being said, the call f.e.evaluate will return an identical bitmap object if it is given the same input. After some thought it seems that synchronizing the method is a good idea as nothing positive comes of two threads executing this code for the same filter.
• In addition I made bitmap final as it shouldn't be mutated after creation.

Many thanks! Gav

Answer 1

If your method only uses parameters that are passed in and local variables, with no shared state, then I'd say it's thread-safe and no synchronization is needed.

Thread safety has to worry about mutable, shared state. Is fullmap part of that object's state? If yes, then you have to synchronize its access.

Answer 2

2 threads could access the map fullMap at the same time. Both could determine that the map doesn't contain a value for the same key, each create one, and then write it back, thus inserting a key twice.

This may well not be a problem beyond one of efficiency. However it can be a source of confusion, and may cause problems in the future as your solution evolves (how expensive will it be to create these objects in the future? What happens if someone copies/paste the code somewhere less appropriate!)

I would strongly recommend synchronising on the above (most probably on fullMap itself rather than the containing object, but more context would be useful before deciding exactly what's required)

Answer 3

SoftHashMap.put itself may just not be thread-safe. SoftHashMap isn't in the standard library, but WeakHashMap is, and it's not synchronized. Besides synchronizing the method on the map, you may want to use Collections.synchronizedMap to ensure other methods don't modify the map concurrently.

Answer 4

I would opt for the synchronization path if the Bitmap returned is modified outside of the method. You run the risk of 2 threads accessing the method above while the Bitmap at f.id is null, both creating one and adding it to the map, with the second one overwriting the first in the Map. Now you have two, one that is going to be modified by thread-1 but go out of scope once thread-1 is done with its processing, and the other from thread-2 which will remain in the map and be provided to all future requesters.

Answer 5

You absolutely need synchronization, because you can have two threads decide that f.id is not in the map, construct and then add one. Each thread will be returned a difference instance for f.id, even though the map will only contain the one that finished last.

At issue isn't the variable bitmap. That is thread-safe as it's local to a single thread. However, access to `fullMap -- which I assume is a field of the class -- needs to be synchronized due to the fact that you're doing a "put-if-absent".

Assuming the cost of constructing a bitmap is expensive, the best way to do this is just to synchronize the method getFullBitmap(). If it was very cheap to construct -- cheaper than synchronization -- then I would suggest always constructing the new object and doing putIfAbsent on a ConcurrentMap. But when the object is expensive to construct, this is a bad idea.

Answer 6

I' not sure about this, but AFAIK you could get thrown a kind of "Concurent Modification Exception" if you have code, iterating over the "fullMap". This must not be in your code, but could happen in the library routines for SoftMap. This might lead to your code breaking at runtime every now and then for no obvious reason, and without a good way for you handle the situation.

Just a complicated way of saying: "if in doubt, be carefull". BTW: don't think about performance first with todays computers.

Happy Hacking

Huibert Gill