Calculating the value of Pi with Monte Carlo

Question

I made this code to calculate pi using MonteCarlo method.

I'm also learning how do Java threadpools and multithreading work.

Can you tell me if this method is thread-safe and how can I improve it if there is a deadlock chance.

private volatile boolean waiting = false;
private int reqsamples = 10000;
private int samples = 0;
private void job(){
 while(samples < reqsamples && started){
 // This lock is used to safely get arraylist data (points).
 if(lock.tryLock()){
 try {
 // Montecarlo algorithm
 double x = 500 * random.nextDouble();
 double y = 500 * random.nextDouble();
 double dist = Math.hypot(x, y);
 if(dist < 500) innersamples++;
 else outersamples++;
 points.add(new Point((int) x, (int) y));
 } finally {
 // Now it is possible to safely get arraylist data.
 lock.unlock();
 }
 samples++;
 if(sampledelay > 0){
 // here I use a scheduled thread pool to avoid Thread.sleep()
 // I don't know if it is safe to use Thread.sleep() here
 // instead of all of this code, because I know that
 // calling Thread.sleep() in a loop isn't that good
 timersvc.schedule(() -> {
 synchronized(waitlock){
 // EDIT: Here I wasn't using do-while
 // It was causing a deadlock :(
 do
 //that's another lock
 waitlock.notifyAll();
 while(!waiting);
 waiting = false;
 }
 }, (long)(sampledelay * 1000000), TimeUnit.NANOSECONDS);
 synchronized(waitlock){
 try {
 waiting = true;
 waitlock.wait();
 } catch (InterruptedException ex) {
 }
 }
 }
 }
 }
}

• user33306: A relatively minor quibble, Monte Carlo doesn't calculate PI it estimates the value of PI.
• TorbenPutkonen: We can not tell. The function relies on variables (and code) that you left out.

Answer 1

As has been said in the comments, there are bits missing here for a full answer.... however some general feedback. I'm assuming that the job method will be running on multiple threads.

samples

How important is it that the number of samples collected is exactly reqsamples? If it's important then you need to protect samples. As it stands, it's possible (although fairly unlikely) that a thread would unlock lock just as another thread enters the while loop, having checked the value of samples. This would result in an extra sample being created and added to points. It's also worth mentioning that sample++ isn't guaranteed to be threadsafe. If you're not going to protect it, it may be worth considering using AtomicInteger.

trylock

I don't really see the advantage of using trylock here, over just using lock. If you fail to get the lock, you're simply spinning checking if enough samples have been generated / it's been shutdown. This seems like wasted cycles that could be used elsewhere.

minimise locked time

In general, I try to keep the amount of work that's done within locks to a minimum. Sometimes that might mean doing a bit of extra work that's wasted but the net result can be beneficial. So, for example the generation of the x/y values and dist don't really need to be done during protected processing.

schedule vs sleep

I think the aversion to putting sleeps in loops is that it suggests that you're polling / performing an operation that might be handled better in another way. Looking at the code you've written, you're essentially replacing sleep with your own more complicated implementation of it. I don't see a benefit over using sleep here at all.

sampledelay

So, on the face of it, it looks like sampledelay is supposed to be how long to wait before generating the next sample. This would be fine if only one thread is running the job method. However, if you've got multiple threads running it, then the behaviour is going to be somewhat unpredictable since it's really telling each thread how long to wait before it starts trying to create the next sample (which might mean that multiple threads generate a sample close to each other, then they all wait for sampledelay then you get another batch of samples). This may(not) be what you're expecting. For the below, I'll assume that it is...

Tweaked version

Putting it together, you might end up with some code more like this:

private void job() {
 // Keep looping until we're done
 while (samples.get() < reqsamples && started) {
 // Generate the sample information outside the lock,
 // worst case each thread discards one.
 double x = 500 * random.nextDouble();
 double y = 500 * random.nextDouble();
 double dist = Math.hypot(x, y);
 Point newPoint = new Point((int) x, (int) y);
 try {
 lock.lock();
 // Now that we have the lock, check if we still need to actually
 // use our sample
 if (samples.get() < reqsamples && started) {
 points.add(newPoint);
 samples.incrementAndGet();
 if (dist < 500) innersamples++;
 else outersamples++;
 }
 } finally {
 lock.unlock();
 }
 // Only need to sleep if we're not done already
 if (sampledelay > 0 && (samples.get() < reqsamples && started)) {
 try {
 Thread.sleep(sampledelay * 1000);
 } catch (InterruptedException ex) {
 }
 }
 }
}