JavaFX Application Thread slows and then freezes

Question

I have a multi-threaded JavaFX application. I have a bunch of background threads that are calling this method to update the main UI from the application thread.

public void writeToActivityLog(String message){

    class ThreadedTask implements Runnable {

        private String message;

        public ThreadedTask(String message){
            this.message = message;
        }

        @Override
        public void run() {
            try{
                //delete older text when there are more than 200 lines and append new text

                String text = outputLog.getText();
                String[] lines = text.split("\r\n");
                String newText = "";
                for(int i = 0; i < lines.length; i++){
                    if(i >= 200 || lines.length < 200){
                        newText += lines[i];
                    }
                }
                outputLog.setText(newText);
                outputLog.appendText(message + "\r\n");

                //scroll to the bottom of the log
                outputLog.setScrollTop(Double.MIN_VALUE);

            } catch(NullPointerException e){
                e.printStackTrace();
            }
        }
    }

    ThreadedTask thread = new ThreadedTask(message);
    Platform.runLater(thread);

}

At first, this method works fine. But a few seconds into the program, it begins to slow, and a few more seconds later the whole UI freezes and stops responding to user input (but it doesn't trigger the Windows "This program is not responding" dialogue). However, looking at application log files and my IDE console, the background threads are still executing and seem to be doing just fine.

Is there some limit as to the number of Platform.runLater() requests that can be queued up? Or is there some way I might have a memory leak that is killing the main application thread but isn't doing anything to the background threads? I am still very new to multi-threaded programming, so I don't know what to think here.

I also know that JavaFX has another concurrency tool called Services, but I can't find any explanation as to when and why I should be using them instead of Platform.runLater(). I have written other JavaFX apps and never had any issues using Platform.runLater().

Edit:

Thanks a lot to the two answers below. The problem was nothing with JavaFX or threads per se but just plain bad code. Here is the fixed code, for completion sake:

public void writeToActivityLog(String message){
    //Create a new thread to update the UI so that it doesn't freeze due to concurrency issues
    class ThreadedTask implements Runnable {

        private String message;

        public ThreadedTask(String message){
            this.message = message;
        }

        @Override
        public void run() {
            outputLog.setText(message);

            //scroll to the bottom of the log
            outputLog.setScrollTop(Double.MIN_VALUE);
        }
    }

    String wholeMessage = new StringBuilder().append(outputLog.getText()).append(message).toString();
    //modify the text of the output log so that it is 200 lines or less
    StringBuilder newText = new StringBuilder();
    String[] lines = wholeMessage.split("\r\n");

    for (int i=Math.max(0, lines.length - 200); i<lines.length; i++) {
        newText.append(new StringBuilder().append(lines[i]).append("\r\n").toString());
    }

    ThreadedTask thread = new ThreadedTask(newText.toString());
    Platform.runLater(thread);
}

Answer 1

By concatenating strings in a loop using the += operator, you're performing an enormous amount of work. If your buffer has 199 lines, doing successive concatenation will copy the first line nearly 10,000 times. (Though I can't quite figure out what the i >= 200 condition is doing. In addition, you're taking a text buffer of up to 200 lines and splitting it into separate strings. This involves a lot of copying.

The problem is that you're doing all this work on the event dispatch thread, which blocks up processing of the user interface. I suspect what's happening is that this work is taking a noticeable amount of time, and the UI appears to freeze while this task is being performed.

I'd suggest moving the logic of updating the log's lines outside of the event handling, and have the thread that is appending the message do the work of updating the text for the buffer. Then, call runLater() on a task that simply sets the text of the outputLog. This avoids excessive string processing happening on the event thread.

If you're going to be doing a lot of string concatenation, use a StringBuilder to append successive lines of text and then call toString() once you're all done. This avoids redundant copying.

EDIT

Missed this earlier. The original code splits on "\r\n" but concatenates the lines together again using

newText += lines[i];

which does not restore the line breaks. As lines get added they end up appending to the "same" line, so the text just gets longer and longer even though it's only one line long. The N-squared appending behavior might not be entering into the problem. Instead, the output log (I assume it's a text node of some sort) has to run its line-breaking algorithm on ever-larger pieces of text. That's probably why things slow down progressively.

In any case, fixing the appending problem, fixing the line-rotation logic, and moving processing out of the event loop should help.

Answer 2

A couple of suggestions.

First, and this is just a general Java thing, it's a pretty bad idea to build a String by concatenation in a loop like this. In many cases a modern compiler will fix this for you, but your if clause in there makes it harder to do so and less likely you will get this optimization automatically. You should make the following modifications:

// String newText = "" ;
StringBuilder newText = new StringBuilder();
// ...
// newText += lines[i] ;
newText.append(lines[i]);
// ...
// outputLog.setText(newText);
outputLog.setText(newText.toString());

The second point is a JavaFX concurrency issue. You're not really saving anything by creating your thread here, since you simply create the thread, then schedule it to run on the FX Application Thread by passing it directly to Platform.runLater(...). So the entire run() method is just being executed on the FX Application Thread anyway. (There are no background threads in your code!)

There are two rules to abide by: 1. Only update "live" nodes on the FX Application Thread 2. Don't perform any long-running tasks on that thread

The Task class in javafx.concurrency is a Runnable (actually a Callable, which is a little more flexible) that provides some helpful lifecycle methods that are guaranteed to be run on the FX Application Thread. So you can use a Task to compute your new log text, return that new log text, and then just use setOnSucceeded(..) to update the UI once it's done. This would look like:

class UpdateLogTask extends Task<String> { // a Task<T> has a call() method returning a T
    private final String currentLog ;
    private final String message ;
    public UpdateLogTask(String currentLog, String message) {
      this.currentLog = currentLog ;
      this.message = message ;
    }
    @Override
    public String call() throws Exception {
      String[] lines = currentLog.split("\r\n");
      StringBuilder text = new StringBuilder();
      for (int i=0; i<lines.length; i++) {
        if (i>=200 || lines.length < 200) {
          text.append(lines[i]);
        }
      }
      text.append(message).append("\r\n");
      return text.toString() ;
    }
}
final Task<String> updateLogTask = new UpdateLogTask(outputLog.getText(), message);
updateLogTask.setOnSucceeded(new EventHandler<WorkerStateEvenet>() {
    @Override
    public void handle(WorkerStateEvent event) {
      outputLog.setText(updateLogTask.getValue());
    }
});
Thread t = new Thread(updateLogTask);
t.setDaemon(true); // will terminate if JavaFX runtime terminates
t.start();

And finally, I think if you want the last 200 lines of text, your logic is wrong. Try

for (int i=Math.max(0, lines.length - 200); i<lines.length; i++) {
  text.append(lines[i]);
}