[PATCH] Decrease Toolkit.firePulse allocation rate

[Doswald Michael]

While profiling a JavaFX application that runs on embedded hardware, I have found that the Toolkit.firePulse method creates more garbage than necessary. In an application that simply animates an object without doing much else, I see that the firePulse method allocates a  fair amount of objects (compared to the overall number of allocated objects). I have written a small patch that decreases the allocation rate in said method. Since I'm not sure if such tweaks are desired in the JavaFX codebase, I didn't open a JIRA issue. I'm happy to do so if they are.

The following small application animates a single circle on screen. The allocation rate of the 'JavaFX Application Thread' is as follows (measured with jvisualvm):
openjfx 8u HEAD:        ~ 65 KB/s after start, ~ 49 KB/s after JIT compilation kicked in
with patch:                    ~ 25 KB/s, doesn't seem to change when JIT kicks in

public class AllocateApp extends Application {
    public static void main(String... args) {
      Application.launch(args);
    }

    @Override
    public void start(Stage primaryStage) throws Exception {
        Circle circle = new Circle(10, 25, 10);

        Timeline timeline = new Timeline(new KeyFrame(Duration.seconds(2), new KeyValue(circle.centerXProperty(), 490)));
        timeline.setCycleCount(Timeline.INDEFINITE);
        timeline.setAutoReverse(true);
        timeline.play();

        primaryStage.setScene(new Scene(new Pane(circle), 500, 50));
        primaryStage.show();
    }
}


The patch below only creates a single List object instead of three WeakHashMaps to make a local copy of the TKPulseListeners. It also uses the number of listeners in the maps to estimate the size of the list. The patch uses WeakReference objects in the list,  which I doubt is necessary, but it emulates the previous behaviour more accurately. I believe it would be possible to change that to a strong reference for the following reasons:
a) The list is local and the strong references would only be there for the time the firePulse method is run
b) The code would become more readable (less generics parameters, null-guard not necessary in loop)
c) Getting rid of the WeakReference objects would decrease garbage generation even more (down to ~ 21 KB/s)


Regards
Michael

diff -r f1c3eb85af4d modules/graphics/src/main/java/com/sun/javafx/tk/Toolkit.java
--- a/modules/graphics/src/main/java/com/sun/javafx/tk/Toolkit.java    Fri Jan 08 08:11:51 2016 -0800
+++ b/modules/graphics/src/main/java/com/sun/javafx/tk/Toolkit.java    Wed Jan 13 10:43:27 2016 +0100
@@ -51,7 +51,6 @@
 import javafx.scene.shape.StrokeType;
 import javafx.stage.FileChooser.ExtensionFilter;
 import javafx.stage.Modality;
-import javafx.stage.Stage;
 import javafx.stage.StageStyle;
 import javafx.stage.Window;
 import java.io.File;
@@ -94,6 +93,8 @@
 import com.sun.scenario.effect.Color4f;
 import com.sun.scenario.effect.FilterContext;
 import com.sun.scenario.effect.Filterable;
+import java.lang.ref.WeakReference;
+import javafx.util.Pair;
 
 
 public abstract class Toolkit {
@@ -362,32 +363,36 @@
         // and those changes propogated to scene before it gets its pulse to update
 
         // Copy of listener map
-        final Map<TKPulseListener,AccessControlContext> stagePulseList =
-                new WeakHashMap<TKPulseListener,AccessControlContext>();
-        final Map<TKPulseListener,AccessControlContext> scenePulseList =
-                new WeakHashMap<TKPulseListener,AccessControlContext>();
-        final Map<TKPulseListener,AccessControlContext> postScenePulseList =
-                new WeakHashMap<TKPulseListener,AccessControlContext>();
+        final List<Pair<WeakReference<TKPulseListener>,AccessControlContext>> listenersList;
 
         synchronized (this) {
-            stagePulseList.putAll(stagePulseListeners);
-            scenePulseList.putAll(scenePulseListeners);
-            postScenePulseList.putAll(postScenePulseListeners);
+          listenersList = new ArrayList<>(stagePulseListeners.size()+scenePulseListeners.size()+postScenePulseListeners.size());
+          copyListeners(stagePulseListeners, listenersList);
+          copyListeners(scenePulseListeners, listenersList);
+          copyListeners(postScenePulseListeners, listenersList);
         }
-        for (Map.Entry<TKPulseListener,AccessControlContext> entry : stagePulseList.entrySet()) {
-            runPulse(entry.getKey(), entry.getValue());
-        }
-        for (Map.Entry<TKPulseListener,AccessControlContext> entry : scenePulseList.entrySet()) {
-            runPulse(entry.getKey(), entry.getValue());
-        }
-        for (Map.Entry<TKPulseListener,AccessControlContext> entry : postScenePulseList.entrySet()) {
-            runPulse(entry.getKey(), entry.getValue());
+
+        for (int idx = 0, max = listenersList.size(); idx < max; idx++) {
+          Pair<WeakReference<TKPulseListener>, AccessControlContext> listenerEntry = listenersList.get(idx);
+          TKPulseListener pulseListener = listenerEntry.getKey().get();
+          if (pulseListener != null) {
+            runPulse(pulseListener, listenerEntry.getValue());
+          }
         }
 
         if (lastTkPulseListener != null) {
             runPulse(lastTkPulseListener, lastTkPulseAcc);
         }
     }
+
+    private void copyListeners(Map<TKPulseListener, AccessControlContext> listenerMap, List<Pair<WeakReference<TKPulseListener>,AccessControlContext>> listenersList) {
+        if (!listenerMap.isEmpty()) {
+            for (Map.Entry<TKPulseListener,AccessControlContext> entry : listenerMap.entrySet()) {
+                listenersList.add(new Pair<>(new WeakReference<>(entry.getKey()), entry.getValue()));
+            }
+        }
+    }
+
     public void addStageTkPulseListener(TKPulseListener listener) {
         if (listener == null) {
             return;