CODEWITHSUNDEEP
javasynchronizationmethods
gav
gav

Reputation: 29122

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

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

Many thanks! Gav

Upvotes: 0

Views: 274

Answers (6)

Huibert Gill
Huibert Gill

Reputation: 723

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

Upvotes: 0

Eddie
Eddie

Reputation: 54421

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.

Upvotes: 1

Brian Agnew
Brian Agnew

Reputation: 272217

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)

Upvotes: 4

akf
akf

Reputation: 39485

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.

Upvotes: 0

Matthew Flaschen
Matthew Flaschen

Reputation: 284786

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.

Upvotes: 2

duffymo
duffymo

Reputation: 308733

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.

Upvotes: 0

Related Questions