2

I have 2 separate threads in an application that does some graph theory simulations (using Java2D), one executes ~60 times per second and its task is rendering. The other one executes ~30 times per second and does some calculations.

Both of these threads share resources. To be specific, they share collections which they iterate through. A problem occurs when both of these threads access a collection at the same time. (One is trying to render objects in it, while the other is modifying the collection). That results in a ConcurrentModificationException. I've tried some solutions and one that I figured out is working properly (and is easy to implement) is using an empty object that the threads switch holding a lock on.

Like so (example code):

public class Question {

static Object syncObject;

public static void main(String []Args) {

    syncObject = new Object();

    Runnable rendering = new Runnable() { //called 60 times a second
        public void run() {
            synchronized(syncObject) {
                //doRendering
            }

        }
    };
    Runnable calculations = new Runnable() { //called 30 times a second
        public void run() {
            synchronized(syncObject) {
                //doModifications
            }       
        }
    };

    Thread t1 = new Thread(rendering);
    Thread t2 = new Thread(calculations);
    t1.start();
    t2.start();
    }
}

Now, this seems to work properly. This way I dont have to synchronize every single resource I'm working with that might be modified at the moment of rendering as well as the "logic" calculations update.

But I feel like this is not a very good solution tho. So I want to ask if this is somewhat acceptable and/or if there is a more proper solution. (Unless what I'm doing is completely wrong)

Also I don't want to rewrite half of the existing code that I've been working on for a while.

DUDSS
  • 110
  • 1
  • 9
  • 2
    This is perfectly acceptable, though I prefer something like this: `static final Object syncObject = new Object();` to prevent reassigning the lock object. – Tudor Jul 10 '18 at 14:35

2 Answers2

1

A global lock, as you have implemented above is a good solution when a 'coarse' lock is suitable. I feel that it meets your case well.

To improve it, you should make sure that the global lock is immutable.

private static final Object syncObject = new Object();

The limitation of this approach is that you are essentially serialising all access to the graph. This limits the potential of concurrency to just one at a time. Thus on a multi-cpu machine, you will have cpus sitting idle while the rendering and calculation work might be taking too long. The advantage is that it is simple to implement and it lets you access from different threads safely.

Other options, which are more complicated to implement are: 1) to use multiple fine grained locks, 2) investigate 'lockless algorithms' or 3) go 'immutable' using a mutate/publish approach to the data structure.

The following reference discusses coarse and fine grained locks: http://fileadmin.cs.lth.se/cs/education/eda015f/2013/herlihy4-5-presentation.pdf

Lockless algorithms can get very involved to implement correctly and will not work for all situations. A primer can be read here.

Option 3, using an immutable data structure has many advantages (and some limitations). What it means is that the graph, once created could not be changed. Mutation would occur by creating a new graph; reuse of parts of the graph would be allowed in order to reduce memory overheads. This would then mean that rendering can acquire a copy of the graph and render in complete isolation from the calculation that mutates the graph. When the mutations are complete, an AtomicReference could be used to make the updated graph available to the renderer. The downside is that it requires more object allocations and a more complicated mutation algorithm, however it benefits from allowing 'reader' threads to work in parallel to a single mutator thread. For more details see persistent vs immutable data structure, https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/atomic/AtomicReference.html and How does concurrency using immutable/persistent types and data structures work?.

Chris K
  • 11,622
  • 1
  • 36
  • 49
0

You can use any object to synchronise, so what you have done is correct. However, instead of using the intrinsic lock of an empty object, java concurrency packages provide specific lock objects for doing that. See https://docs.oracle.com/javase/tutorial/essential/concurrency/newlocks.html

blue_note
  • 27,712
  • 9
  • 72
  • 90
  • The `java.util.concurrent.locks.ReentrantLock` class is more _powerful_ than the language's built-in synchronization, but the power to do sophisticated, and possibly risky things (e.g., lock a lock in one routine and unlock it in another) does not necessarily make it _better_ for doing the simple things that built-in synchronization can achieve. – Solomon Slow Jul 10 '18 at 17:21
  • @jameslarge: didn't say it's *better*, just mentioned that what the OP does is correct, but there's an additional option. – blue_note Jul 11 '18 at 09:43