From 1e729ed567830b3327198245b52dc5825b0245a2 Mon Sep 17 00:00:00 2001 From: Abraham Wolk Date: Fri, 28 Jun 2024 12:08:41 +0200 Subject: [PATCH 1/3] CSSTUDIO-2472 Bugfix: Fix freezes of Display Runtime occurring when rapidly changing between embedded displays. --- .../EmbeddedDisplayRepresentation.java | 136 +++++++++--------- .../javafx/widgets/JFXBaseRepresentation.java | 1 - .../widgets/plots/XYPlotRepresentation.java | 1 - .../representation/WidgetRepresentation.java | 4 - 4 files changed, 65 insertions(+), 77 deletions(-) diff --git a/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/EmbeddedDisplayRepresentation.java b/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/EmbeddedDisplayRepresentation.java index e6b032542d..1cd4e049e8 100644 --- a/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/EmbeddedDisplayRepresentation.java +++ b/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/EmbeddedDisplayRepresentation.java @@ -15,6 +15,7 @@ import java.util.concurrent.atomic.AtomicReference; import java.util.logging.Level; +import javafx.application.Platform; import org.csstudio.display.builder.model.DirtyFlag; import org.csstudio.display.builder.model.DisplayModel; import org.csstudio.display.builder.model.UntypedWidgetPropertyListener; @@ -256,7 +257,7 @@ private void fileChanged(final WidgetProperty property, final Object old_valu /** Update to the next pending display * - *

Synchronized to serialize the background threads. + *

Executed on the JavaFX Application Thread to serialize the background threads. * *

Example: Displays A, B, C are requested in quick succession. * @@ -270,65 +271,56 @@ private void fileChanged(final WidgetProperty property, final Object old_valu * As thread C finally continues, it finds pending_display_and_group empty. * --> Showing A, then C, skipping B. */ - private synchronized void updatePendingDisplay(final JobMonitor monitor) - { - try - { - final DisplayAndGroup handle = pending_display_and_group.getAndSet(null); - if (handle == null) - { - // System.out.println("Nothing to handle"); - return; - } - if (inner == null) - { - // System.out.println("Aborted: " + handle); - return; - } + private void updatePendingDisplay(final JobMonitor monitor) { + Platform.runLater(() -> { + try { + final DisplayAndGroup handle = pending_display_and_group.getAndSet(null); + if (handle == null) { + // System.out.println("Nothing to handle"); + return; + } + if (inner == null) { + // System.out.println("Aborted: " + handle); + return; + } - monitor.beginTask("Load " + handle); - try - { // Load new model (potentially slow) - final DisplayModel new_model = loadDisplayModel(model_widget, handle); + monitor.beginTask("Load " + handle); + try { // Load new model (potentially slow) + final DisplayModel new_model = loadDisplayModel(model_widget, handle); - // Stop (old) runtime - // EmbeddedWidgetRuntime tracks this property to start/stop the embedded model's runtime - model_widget.runtimePropEmbeddedModel().setValue(null); + // Stop (old) runtime + // EmbeddedWidgetRuntime tracks this property to start/stop the embedded model's runtime + model_widget.runtimePropEmbeddedModel().setValue(null); - // Atomically update the 'active' model - final DisplayModel old_model = active_content_model.getAndSet(new_model); - new_model.propBackgroundColor().addUntypedPropertyListener(backgroundChangedListener); + // Atomically update the 'active' model + final DisplayModel old_model = active_content_model.getAndSet(new_model); + new_model.propBackgroundColor().addUntypedPropertyListener(backgroundChangedListener); - if (old_model != null) - { // Dispose old model + if (old_model != null) { // Dispose old model + final Future completion = toolkit.submit(() -> + { + toolkit.disposeRepresentation(old_model); + return null; + }); + checkCompletion(model_widget, completion, "timeout disposing old representation"); + } + // Represent new model on UI thread + toolkit.onRepresentationStarted(); final Future completion = toolkit.submit(() -> { - toolkit.disposeRepresentation(old_model); + representContent(new_model); return null; }); - checkCompletion(model_widget, completion, "timeout disposing old representation"); + checkCompletion(model_widget, completion, "timeout representing new content"); + // Allow EmbeddedWidgetRuntime to start the new runtime + model_widget.runtimePropEmbeddedModel().setValue(new_model); + } catch (Exception ex) { + logger.log(Level.WARNING, "Failed to handle embedded display " + handle, ex); } - // Represent new model on UI thread - toolkit.onRepresentationStarted(); - final Future completion = toolkit.submit(() -> - { - representContent(new_model); - return null; - }); - checkCompletion(model_widget, completion, "timeout representing new content"); - - // Allow EmbeddedWidgetRuntime to start the new runtime - model_widget.runtimePropEmbeddedModel().setValue(new_model); + } finally { + toolkit.onRepresentationFinished(); } - catch (Exception ex) - { - logger.log(Level.WARNING, "Failed to handle embedded display " + handle, ex); - } - } - finally - { - toolkit.onRepresentationFinished(); - } + }); } /** @param content_model Model to represent */ @@ -471,26 +463,28 @@ else if (inner.getHeight() != 0.0 && inner.getWidth() != 0.0) } @Override - public void dispose() - { - // When the file name is changed, updatePendingDisplay() - // will atomically update the active_content_model, - // represent the new model, and then set runtimePropEmbeddedModel. - // - // Fetching the embedded model from active_content_model - // could dispose a representation that hasn't been represented, yet. - // Fetching the embedded model from runtimePropEmbeddedModel - // could fail to dispose what's just now being represented. - // - // --> Very unlikely to happen because runtime has been stopped, - // so nothing is changing the file name right now. - final DisplayModel em = active_content_model.getAndSet(null); - model_widget.runtimePropEmbeddedModel().setValue(null); - - if (inner != null && em != null) - toolkit.disposeRepresentation(em); - inner = null; - - super.dispose(); + public void dispose() { + Platform.runLater(() -> { + // When the file name is changed, updatePendingDisplay() + // will atomically update the active_content_model, + // represent the new model, and then set runtimePropEmbeddedModel. + // + // Fetching the embedded model from active_content_model + // could dispose a representation that hasn't been represented, yet. + // Fetching the embedded model from runtimePropEmbeddedModel + // could fail to dispose what's just now being represented. + // + // --> Very unlikely to happen because runtime has been stopped, + // so nothing is changing the file name right now. + + final DisplayModel em = active_content_model.getAndSet(null); + model_widget.runtimePropEmbeddedModel().setValue(null); + + if (inner != null && em != null) + toolkit.disposeRepresentation(em); + inner = null; + + super.dispose(); + }); } } diff --git a/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/JFXBaseRepresentation.java b/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/JFXBaseRepresentation.java index 3ba4f67132..40f03b8a0c 100644 --- a/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/JFXBaseRepresentation.java +++ b/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/JFXBaseRepresentation.java @@ -251,7 +251,6 @@ public void dispose() logger.log(Level.WARNING, "Missing JFX parent for " + model_widget); else JFXRepresentation.getChildren(parent).remove(jfx_node); - jfx_node = null; } /** Get parent that would be used for child-widgets. diff --git a/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/plots/XYPlotRepresentation.java b/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/plots/XYPlotRepresentation.java index 9ae909fced..11105cf3fa 100644 --- a/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/plots/XYPlotRepresentation.java +++ b/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/plots/XYPlotRepresentation.java @@ -736,6 +736,5 @@ public void dispose() { super.dispose(); plot.dispose(); - plot = null; } } diff --git a/app/display/representation/src/main/java/org/csstudio/display/builder/representation/WidgetRepresentation.java b/app/display/representation/src/main/java/org/csstudio/display/builder/representation/WidgetRepresentation.java index e4a8018ce0..cc9515bf1e 100644 --- a/app/display/representation/src/main/java/org/csstudio/display/builder/representation/WidgetRepresentation.java +++ b/app/display/representation/src/main/java/org/csstudio/display/builder/representation/WidgetRepresentation.java @@ -86,9 +86,5 @@ public void initialize(final ToolkitRepresentation toolkit, void destroy() { dispose(); - - // Speedup GC - model_widget = null; - toolkit = null; } } From 86b178605375bf649f4d4673afbeeb7625a136b2 Mon Sep 17 00:00:00 2001 From: Abraham Wolk Date: Thu, 4 Jul 2024 11:15:18 +0200 Subject: [PATCH 2/3] CSSTUDIO-2472 Revert changes to "EmbeddedDisplayRepresentation". --- .../EmbeddedDisplayRepresentation.java | 136 +++++++++--------- 1 file changed, 71 insertions(+), 65 deletions(-) diff --git a/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/EmbeddedDisplayRepresentation.java b/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/EmbeddedDisplayRepresentation.java index 1cd4e049e8..e6b032542d 100644 --- a/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/EmbeddedDisplayRepresentation.java +++ b/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/EmbeddedDisplayRepresentation.java @@ -15,7 +15,6 @@ import java.util.concurrent.atomic.AtomicReference; import java.util.logging.Level; -import javafx.application.Platform; import org.csstudio.display.builder.model.DirtyFlag; import org.csstudio.display.builder.model.DisplayModel; import org.csstudio.display.builder.model.UntypedWidgetPropertyListener; @@ -257,7 +256,7 @@ private void fileChanged(final WidgetProperty property, final Object old_valu /** Update to the next pending display * - *

Executed on the JavaFX Application Thread to serialize the background threads. + *

Synchronized to serialize the background threads. * *

Example: Displays A, B, C are requested in quick succession. * @@ -271,56 +270,65 @@ private void fileChanged(final WidgetProperty property, final Object old_valu * As thread C finally continues, it finds pending_display_and_group empty. * --> Showing A, then C, skipping B. */ - private void updatePendingDisplay(final JobMonitor monitor) { - Platform.runLater(() -> { - try { - final DisplayAndGroup handle = pending_display_and_group.getAndSet(null); - if (handle == null) { - // System.out.println("Nothing to handle"); - return; - } - if (inner == null) { - // System.out.println("Aborted: " + handle); - return; - } + private synchronized void updatePendingDisplay(final JobMonitor monitor) + { + try + { + final DisplayAndGroup handle = pending_display_and_group.getAndSet(null); + if (handle == null) + { + // System.out.println("Nothing to handle"); + return; + } + if (inner == null) + { + // System.out.println("Aborted: " + handle); + return; + } - monitor.beginTask("Load " + handle); - try { // Load new model (potentially slow) - final DisplayModel new_model = loadDisplayModel(model_widget, handle); + monitor.beginTask("Load " + handle); + try + { // Load new model (potentially slow) + final DisplayModel new_model = loadDisplayModel(model_widget, handle); - // Stop (old) runtime - // EmbeddedWidgetRuntime tracks this property to start/stop the embedded model's runtime - model_widget.runtimePropEmbeddedModel().setValue(null); + // Stop (old) runtime + // EmbeddedWidgetRuntime tracks this property to start/stop the embedded model's runtime + model_widget.runtimePropEmbeddedModel().setValue(null); - // Atomically update the 'active' model - final DisplayModel old_model = active_content_model.getAndSet(new_model); - new_model.propBackgroundColor().addUntypedPropertyListener(backgroundChangedListener); + // Atomically update the 'active' model + final DisplayModel old_model = active_content_model.getAndSet(new_model); + new_model.propBackgroundColor().addUntypedPropertyListener(backgroundChangedListener); - if (old_model != null) { // Dispose old model - final Future completion = toolkit.submit(() -> - { - toolkit.disposeRepresentation(old_model); - return null; - }); - checkCompletion(model_widget, completion, "timeout disposing old representation"); - } - // Represent new model on UI thread - toolkit.onRepresentationStarted(); + if (old_model != null) + { // Dispose old model final Future completion = toolkit.submit(() -> { - representContent(new_model); + toolkit.disposeRepresentation(old_model); return null; }); - checkCompletion(model_widget, completion, "timeout representing new content"); - // Allow EmbeddedWidgetRuntime to start the new runtime - model_widget.runtimePropEmbeddedModel().setValue(new_model); - } catch (Exception ex) { - logger.log(Level.WARNING, "Failed to handle embedded display " + handle, ex); + checkCompletion(model_widget, completion, "timeout disposing old representation"); } - } finally { - toolkit.onRepresentationFinished(); + // Represent new model on UI thread + toolkit.onRepresentationStarted(); + final Future completion = toolkit.submit(() -> + { + representContent(new_model); + return null; + }); + checkCompletion(model_widget, completion, "timeout representing new content"); + + // Allow EmbeddedWidgetRuntime to start the new runtime + model_widget.runtimePropEmbeddedModel().setValue(new_model); } - }); + catch (Exception ex) + { + logger.log(Level.WARNING, "Failed to handle embedded display " + handle, ex); + } + } + finally + { + toolkit.onRepresentationFinished(); + } } /** @param content_model Model to represent */ @@ -463,28 +471,26 @@ else if (inner.getHeight() != 0.0 && inner.getWidth() != 0.0) } @Override - public void dispose() { - Platform.runLater(() -> { - // When the file name is changed, updatePendingDisplay() - // will atomically update the active_content_model, - // represent the new model, and then set runtimePropEmbeddedModel. - // - // Fetching the embedded model from active_content_model - // could dispose a representation that hasn't been represented, yet. - // Fetching the embedded model from runtimePropEmbeddedModel - // could fail to dispose what's just now being represented. - // - // --> Very unlikely to happen because runtime has been stopped, - // so nothing is changing the file name right now. - - final DisplayModel em = active_content_model.getAndSet(null); - model_widget.runtimePropEmbeddedModel().setValue(null); - - if (inner != null && em != null) - toolkit.disposeRepresentation(em); - inner = null; - - super.dispose(); - }); + public void dispose() + { + // When the file name is changed, updatePendingDisplay() + // will atomically update the active_content_model, + // represent the new model, and then set runtimePropEmbeddedModel. + // + // Fetching the embedded model from active_content_model + // could dispose a representation that hasn't been represented, yet. + // Fetching the embedded model from runtimePropEmbeddedModel + // could fail to dispose what's just now being represented. + // + // --> Very unlikely to happen because runtime has been stopped, + // so nothing is changing the file name right now. + final DisplayModel em = active_content_model.getAndSet(null); + model_widget.runtimePropEmbeddedModel().setValue(null); + + if (inner != null && em != null) + toolkit.disposeRepresentation(em); + inner = null; + + super.dispose(); } } From 27cc9d27859aca6bb3b27a3b7cee142e7e8e56aa Mon Sep 17 00:00:00 2001 From: Abraham Wolk Date: Thu, 4 Jul 2024 11:16:58 +0200 Subject: [PATCH 3/3] CSSTUDIO-2472 Run dispose() on the UI-thread. --- .../EmbeddedDisplayRepresentation.java | 41 ++++++++++--------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/EmbeddedDisplayRepresentation.java b/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/EmbeddedDisplayRepresentation.java index e6b032542d..dde8845324 100644 --- a/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/EmbeddedDisplayRepresentation.java +++ b/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/EmbeddedDisplayRepresentation.java @@ -15,6 +15,7 @@ import java.util.concurrent.atomic.AtomicReference; import java.util.logging.Level; +import javafx.application.Platform; import org.csstudio.display.builder.model.DirtyFlag; import org.csstudio.display.builder.model.DisplayModel; import org.csstudio.display.builder.model.UntypedWidgetPropertyListener; @@ -473,24 +474,26 @@ else if (inner.getHeight() != 0.0 && inner.getWidth() != 0.0) @Override public void dispose() { - // When the file name is changed, updatePendingDisplay() - // will atomically update the active_content_model, - // represent the new model, and then set runtimePropEmbeddedModel. - // - // Fetching the embedded model from active_content_model - // could dispose a representation that hasn't been represented, yet. - // Fetching the embedded model from runtimePropEmbeddedModel - // could fail to dispose what's just now being represented. - // - // --> Very unlikely to happen because runtime has been stopped, - // so nothing is changing the file name right now. - final DisplayModel em = active_content_model.getAndSet(null); - model_widget.runtimePropEmbeddedModel().setValue(null); - - if (inner != null && em != null) - toolkit.disposeRepresentation(em); - inner = null; - - super.dispose(); + Platform.runLater(() -> { + // When the file name is changed, updatePendingDisplay() + // will atomically update the active_content_model, + // represent the new model, and then set runtimePropEmbeddedModel. + // + // Fetching the embedded model from active_content_model + // could dispose a representation that hasn't been represented, yet. + // Fetching the embedded model from runtimePropEmbeddedModel + // could fail to dispose what's just now being represented. + // + // --> Very unlikely to happen because runtime has been stopped, + // so nothing is changing the file name right now. + final DisplayModel em = active_content_model.getAndSet(null); + model_widget.runtimePropEmbeddedModel().setValue(null); + + if (inner != null && em != null) + toolkit.disposeRepresentation(em); + inner = null; + + super.dispose(); + }); } }