0

It's my first time using threads in java and I came across with weird problem.

    public class ServerSender implements Runnable {

    DatagramSocket udpSocket;
    DatagramPacket sendPacket;

    long currentTime = 0;
    long elapsed = 0;

    public ServerSender(DatagramSocket udpSocket) {
        this.udpSocket = udpSocket;
        System.out.println("Sender live!");
    }

    public void sendToClient(byte[] bytes) throws IOException {
            sendPacket = new DatagramPacket(bytes, bytes.length, Server.clientIp, Server.clientPort);
            udpSocket.send(sendPacket);
    }

    @Override
    public void run() {
        while(true) 
        {
            if(Server.isGameLive) 
            {
                currentTime = System.nanoTime();
                if(elapsed / 1000000 >= 100) // send snapshot every 100ms
                {
                    synchronized(Server.snapshot) 
                    { 
                            try 
                            {
                                sendToClient(Server.snapshot.toByteArray());
                            } 
                            catch (IOException e) 
                            {
                                e.printStackTrace();
                            }
                    }
                    elapsed = 0;
                    System.out.println("Sending snapshot to client...");
                } else {
                    System.out.println("not elapsed");
                }
                elapsed += System.nanoTime() - currentTime;
                System.out.println(elapsed / 1000000);
            } else {
                System.out.println("not live");
            }
        }
    }
}

This code works how it should but when I remove else statements from run method it's not working...

    @Override
public void run() {
    while(true) 
    {
        if(Server.isGameLive) 
        {
            currentTime = System.nanoTime();
            if(elapsed / 1000000 >= 100) // send snapshot every 100ms
            {
                synchronized(Server.snapshot) 
                { 
                        try 
                        {
                            sendToClient(Server.snapshot.toByteArray());
                        } 
                        catch (IOException e) 
                        {
                            e.printStackTrace();
                        }
                }
                elapsed = 0;
                System.out.println("Sending snapshot to client...");
            }
            elapsed += System.nanoTime() - currentTime;
            System.out.println(elapsed / 1000000);
        }
    }
}

Can someone explain what's wrong there?

  • 2
    "is not working" how? – kolossus Jul 19 '14 at 19:26
  • possible duplicate of [Loop doesn't see changed value without a print statement](http://stackoverflow.com/questions/25425130/loop-doesnt-see-changed-value-without-a-print-statement) – Boann Aug 23 '14 at 10:25

3 Answers3

3

My completely wild guess is that you are reading non-volatile values which you are changed in another thread.

When you read a non-volatile field, the JIT is free to inline the value and thus it might never see the value you changed it to.

However, if you slow down the loop, e.g. by writing to the console, it might not run enough times to be JITted so you don't see this optimisation and the loop stops as expected.

http://vanillajava.blogspot.com/2012/01/demonstrating-when-volatile-is-required.html

Peter Lawrey
  • 525,659
  • 79
  • 751
  • 1,130
  • Added volatile keyword to isGameLive boolean and now it works. Thank you! :) –  Jul 19 '14 at 19:40
  • 2
    Writing to the console solves problems where variables are hoisted out of a loop, not because it slows down the loop, but because it introduces memory fences. – dcastro Jul 19 '14 at 20:41
  • @dcastro Adding a fence can help but in this case, it matter whether the JIT inlines the value or not. Adding a fence can prevent it inlining and it is an indirect effect. Making the field `volatile` directly tells the compiler to not optimise it in certain ways. – Peter Lawrey Jul 19 '14 at 20:50
  • 1
    @PeterLawrey I wasn't arguing against making the field volatile, I was just clarifying on why writing to the console helps :) I don't consider it to be a side effect. Hoisting the variable out of a loop is effectively the same as reordering instructions: you move the read backwards in time and use a cached value. Fences are designed to prevent that. Fences will tell the compiler, the jitter and the cpu not to hoist the variable. The fact that writing to the console happens to emit fences - that, I consider to be a side effect. – dcastro Jul 19 '14 at 20:56
  • 1
    @dcastro +1 I agree with that. – Peter Lawrey Jul 19 '14 at 20:58
0

First, make sure Server.isGameLive is volatile, so you're not running into memory visibility issues.

The piece of code you're measuring could take less time to run than the resolution of System.nanoTime() with this code.

Your time calculation should rather be something like:

public void run() {
    currentTime = System.nanoTime();
    while(true) {
        if(Server.isGameLive) {

            if(elapsed / 1000000 >= 100) {
                synchronized(Server.snapshot) { 
                        try {
                           sendToClient(Server.snapshot.toByteArray());

                        } catch (IOException e) {
                            e.printStackTrace();
                        }
                }
                elapsed = 0;
                currentTime = System.nanoTime();
                System.out.println("Sending snapshot to client...");
            }
            elapsed = System.nanoTime() - currentTime;
            System.out.println(elapsed / 1000000);
        }
    }
}
nos
  • 223,662
  • 58
  • 417
  • 506
0

I think that removed println was slowing the operation a little down, so you can add a Thread.sleep(1); at the end of the loop.

This way not only you prevent this problem but also will allow other threads to use the CPU.

Alireza
  • 4,976
  • 1
  • 23
  • 36