Multithreading in java...slow or am I missing something

[khwain]

I'm looking at testing out of a class for next semester, and one of the topics the class covers that I'm not too familiar with is multithreaded programming in Java. To practice for this, I tried to do the final project for the semester of Spring 2010, but for some reason my version(s) runs twice as slow as the standard single threaded iterative example, and I can't figure out why (~5600ms vs 114000ms on a dual core system. It gets slower with 4 threads on quad core). The reference/base code is available http://web.ics.purdue.edu/~cs180/Spring2010Web/, and if anyone is able to help, I can upload my current version somewhere. I've tried a couple different ways to divide the workload, so my current revision is a bit messy...
TL;DR: I have a functioning multithreaded program but can't figure out why it's running so slow.

 

[ambientZ]

Threads work just fine in java.

Your link takes you to the main page for your course, not to anything useful.

 

[-BokkeN-]

I'll need to see your code to say for sure, but I can try. Disclaimer: I don't have a CS degree.

Going by estimation alone, resource locking is probably what is killing you. Whenever you attempt to access shared memory, Java has to acquire locks for that resource, which are expensive and can block threads. To alleviate this, make sure that you're using immutable data structure every where you can.

I'd recommend first loading all of you data up, then using Collections.unmodifiable* methods to create objects to pass to your threads.

Any time you need to write data, be sure to use a concurrent queue, something like this.

Best of luck!

 

[jiminator]

~5600ms vs 114000ms is not twice as slow, it is 20 times slower.... 5 seconds vs 2 minutes

 

[khwain]

Thanks for your responses. I'll try to address each of your replies.
Ambient: Sorry, they used frames, so the address in bar didn't change. The actual link is here

-BokkeN-: If you don't mind taking a look, my code can be found here, including the input data. However I'm not sure what you mean by resource locking; I was under the impression that that these locks aren't implicit, which is the reason the synchronized keyword exists. Currently, what I'm doing is splitting the work so each thread does 1/n of the image and it's merged at the end, but earlier I had a current location counter on the RayCasting class so each thread would actually pull the location of the next pixel, which did utilize resource locking, but it runs at about the same speed anyways.

jiminator: Sorry, I made a typo. The 5600 is supposed to 56000, you probably knew what I meant

 

[ambientZ]

Your threads have pretty massive contention on
JetColorMap
public synchronized Color getColor(double value)

2 threads with synchronized:
42695

remove the synchronized:
2 threads = 18000
1 thread = 29500

getColor isn't doing anything with shared state, so i dont believe you need it to be synchronized.


I used the eclipse profiling tools to discover this to see that when there are 2 threads, they each wind up blocked for about a minute. (the run takes ~90 seconds when thread profiling). And looking at the stack traces when blocked they are trying to call getColor

 

[khwain]

That would do it. I didn't realize there was more than one instance in the default code where they used synchronized, as I assumed they wouldn't because it was already single threaded. I'll go try to deal with that and report back with results. Thank you so much!

Edit: I guess I replied after you edited. Execution Time: 38513 ms .

 

[-BokkeN-]

I see that ambientZ has you covered for the synchronization piece.

A few other things I did was:

* Make VolumeData and RayCasting cloneable with each clone method returning a new VolumeData or RayCasting with the same parameters as the original.

* Fix the PerspectiveCamera.clone() method to return a new PerspectiveCamera. As it stands, using clone will throw a StackOverflowException.

* Each instance of Caster got

rc = new RayCasting(data.clone(), colorMap, stepSize);

In the constructor and

//Color c = castRay(rayOrig, rayNorm);
Color c = rc.castRay(rayOrig, rayNorm);

In the run method. Those changes were worth another 20% performance increase (on top of the synchronized removal increase) on my MacBook. 58000ms -> 38000ms -> 32500ms.

Thanks for the cool project.

However I'm not sure what you mean by resource locking; I was under the impression that that these locks aren't implicit, which is the reason the synchronized keyword exists.

What I meant was, reading and writing shared memory that is synchronized is expensive because each thread must be blocked while the others access it -- basically, what you saw in your application. When dealing with multithreaded applications, it is almost always better to pass each thread a copy of the data to prevent resource locking.

 

[khwain]

Just to make sure I'm understanding what you did correctly...so what you're doing is giving each thread a copy of the existing raycaster object+data that's in a different location and using that object's methods to do the ray casting. This runs faster, but I assume the downside is that it uses more memory (correct me if I'm wrong).

Also, is the reason it runs faster because sharing resources between threads is slower even if it's just reading and doesn't have any synchronization requirements?

 

[-BokkeN-]

Just to make sure I'm understanding what you did correctly...so what you're doing is giving each thread a copy of the existing raycaster object+data that's in a different location and using that object's methods to do the ray casting. This runs faster, but I assume the downside is that it uses more memory (correct me if I'm wrong).

Yes and yes.

Also, is the reason it runs faster because sharing resources between threads is slower even if it's just reading and doesn't have any synchronization requirements?

Not exactly. Sharing read only (final) objects is fine, which is why immutable objects are very popular for concurrent programming in Java. The problem with sharing objects stems from data encapsulation, you never know (unless you wrote the code) if executing a method is overwriting data. The data may or may not be synchronized, but, either way it is bad. If the data isn't synchronized, you risk corruption, if it is, then your program can spend a lot of time waiting for that resource.

When using multi-threading, be careful what you declare synchronized. With your JetColorMap method, you didn't need to synchronize it because you weren't updating local variables.

Looking back, sharing the RayCaster is actually fine. But, in concurrent programming, passing by value is generally always the favorable approach because it eliminates the need for synchronization, and thus, the issues associated with it. Languages, like Erlang, that are designed for concurrency nearly always copy data between threads (as are systems that implement a Map/Reduce paradigm).

Again, I'm certainly no expert on the subject. These techniques are ones I've picked up from reading and emulating what other smarter people have done. I encourage you to read up on Sun's documentation further for more specific details.