Skip to content

Improve robustness of alarm tree update logic #3987

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
LordIdra wants to merge 1 commit into
base: master
Choose a base branch
from
+108 −119
Open

Improve robustness of alarm tree update logic #3987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions app/alarm/ui/src/main/java/org/phoebus/applications/alarm/ui/area/AlarmAreaView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,11 @@
*/
private final AtomicReference<List<String>> item_changes = new AtomicReference<>();

/** Set of items that have updated, with iteration order based on when they updated */
private final Set<String> items_to_update = new LinkedHashSet<>();
/** Set of items that have updated, with iteration order based on when they updated
*
* SYNC on access
*/
private final Set<String> item_paths_to_update = new LinkedHashSet<>();

Check warning on line 82 in app/alarm/ui/src/main/java/org/phoebus/applications/alarm/ui/area/AlarmAreaView.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Rename this field "item_paths_to_update" to match the regular expression '^[a-z][a-zA-Z0-9]*$'. 


See more on https://sonarcloud.io/project/issues?id=ControlSystemStudio_phoebus&issues=AaF9L42BV5Jxo1fiw26W&open=AaF9L42BV5Jxo1fiw26W&pullRequest=3987

/** Throttle [5Hz] used for updates of existing items */
private final UpdateThrottle update_throttle = new UpdateThrottle(200, TimeUnit.MILLISECONDS, this::updateItems);
Expand Down Expand Up @@ -165,9 +168,9 @@
if (null == item_name)
return;

synchronized (items_to_update)
synchronized (item_paths_to_update)
{
items_to_update.add(item_name);
item_paths_to_update.add(item_name);
}
update_throttle.trigger();
}
Expand All @@ -178,10 +181,10 @@
final List<String> changed_items = item_changes.getAndSet(null);

final String[] update_array;
synchronized (items_to_update)
synchronized (item_paths_to_update)
{
update_array = items_to_update.toArray(new String[items_to_update.size()]);
items_to_update.clear();
update_array = item_paths_to_update.toArray(new String[item_paths_to_update.size()]);
item_paths_to_update.clear();
}
Platform.runLater(() ->
{
Expand Down
210 changes: 98 additions & 112 deletions app/alarm/ui/src/main/java/org/phoebus/applications/alarm/ui/tree/AlarmTreeView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,11 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;
Expand Down Expand Up @@ -80,6 +81,14 @@
* as generic JavaFX Node, only calling public methods
* defined on this class.
*
* <p>The model (AlarmClient) and the AlarmTreeView are kept in
* sync by a lockstep between model and view updates. When the
* model is changed, it calls item[removed|added]. The AlarmClient
* then blocks until the method returns (i.e. until the UI has
* been updated) and then can move on to the next update. The
* exception to this is itemUpdated, which adds to a queue of
* items to be updated which is then regularly processed.
*
* @author Kay Kasemir
*/
@SuppressWarnings("nls")
Expand Down Expand Up @@ -112,13 +121,13 @@
private final CountDownLatch block_item_changes = new CountDownLatch(1);

/** Map from alarm tree path to view's TreeItem */
private final ConcurrentHashMap<String, TreeItem<AlarmTreeItem<?>>> path2view = new ConcurrentHashMap<>();
private final Map<String, TreeItem<AlarmTreeItem<?>>> path2view = new HashMap<>();

/** Items to update, ordered by time of original update request
*
* SYNC on access
* SYNC on access
*/
private final Set<TreeItem<AlarmTreeItem<?>>> items_to_update = new LinkedHashSet<>();
private final Set<String> item_paths_to_update = new LinkedHashSet<>();

Check warning on line 130 in app/alarm/ui/src/main/java/org/phoebus/applications/alarm/ui/tree/AlarmTreeView.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Rename this field "item_paths_to_update" to match the regular expression '^[a-z][a-zA-Z0-9]*$'. 


See more on https://sonarcloud.io/project/issues?id=ControlSystemStudio_phoebus&issues=AaF9L4wAV5Jxo1fiw26V&open=AaF9L4wAV5Jxo1fiw26V&pullRequest=3987

/** Throttle [5Hz] used for updates of existing items */
private final UpdateThrottle throttle = new UpdateThrottle(200, TimeUnit.MILLISECONDS, this::performUpdates);
Expand Down Expand Up @@ -161,7 +170,7 @@
* @param model Model to represent. Must <u>not</u> be running, yet
* @param itemName item name that may be expanded or given focus
*/
public AlarmTreeView(final AlarmClient model, String itemName)
public AlarmTreeView(final AlarmClient model, final String itemName)
{
if (model.isRunning())
throw new IllegalStateException();
Expand Down Expand Up @@ -387,37 +396,34 @@
public void itemAdded(final AlarmTreeItem<?> item)
{
blockItemChanges();
// System.out.println(Thread.currentThread() + " Add " + item.getPathName());

// Parent must already exist
final AlarmTreeItem<BasicState> model_parent = item.getParent();
final TreeItem<AlarmTreeItem<?>> view_parent = path2view.get(model_parent.getPathName());

if (view_parent == null)
{
dumpTree(tree_view.getRoot());
throw new IllegalStateException("Missing parent view item for " + item.getPathName());
}
// Create view item ASAP so that following updates will find it..
final TreeItem<AlarmTreeItem<?>> view_item = createViewItem(item);

// .. but defer showing it on screen to UI thread
final CountDownLatch done = new CountDownLatch(1);

Platform.runLater(() ->
{
indicateChange();

// Parent must already exist
final AlarmTreeItem<BasicState> model_parent = item.getParent();
final TreeItem<AlarmTreeItem<?>> view_parent = path2view.get(model_parent.getPathName());

if (view_parent == null) {
dumpTree(tree_view.getRoot());
throw new IllegalStateException("Missing parent view item for " + item.getPathName());
}
final TreeItem<AlarmTreeItem<?>> view_item = createViewItem(item);

// Keep sorted by inserting at appropriate index
final List<TreeItem<AlarmTreeItem<?>>> items = view_parent.getChildren();
final int index = Collections.binarySearch(items, view_item,
(a, b) -> CompareNatural.compareTo(a.getValue().getName(),
b.getValue().getName()));
(a, b) -> CompareNatural.compareTo(a.getValue().getName(),
b.getValue().getName()));
if (index < 0)
items.add(-index-1, view_item);
items.add(-index - 1, view_item);
else
items.add(index, view_item);
done.countDown();
});
updateStats();

// Waiting on the UI thread throttles the model's updates
// to a rate that the UI can handle.
Expand All @@ -438,18 +444,19 @@
public void itemRemoved(final AlarmTreeItem<?> item)
{
blockItemChanges();
// System.out.println(Thread.currentThread() + " Removed " + item.getPathName());

// Remove item and all sub-items from model2ui
final TreeItem<AlarmTreeItem<?>> view_item = removeViewItems(item);
if (view_item == null)
throw new IllegalStateException("No view item for " + item.getPathName());

// Remove the corresponding view
final CountDownLatch done = new CountDownLatch(1);
Platform.runLater(() ->
{
indicateChange();

// Remove item and all sub-items from model2ui
final TreeItem<AlarmTreeItem<?>> view_item = removeViewItems(item);
if (view_item == null)
throw new IllegalStateException("No view item for " + item.getPathName());

// Remove the corresponding view
//
// Can only locate the parent view item on UI thread,
// because item might just have been created by itemAdded() event
// and won't be on the screen until UI thread runs.
Expand All @@ -459,7 +466,6 @@
view_parent.getChildren().remove(view_item);
done.countDown();
});
updateStats();

// Waiting on the UI thread throttles the model's updates
// to a rate that the UI can handle.
Expand Down Expand Up @@ -493,87 +499,85 @@
public void itemUpdated(final AlarmTreeItem<?> item)
{
blockItemChanges();
// System.out.println(Thread.currentThread() + " Updated " + item.getPathName());
final TreeItem<AlarmTreeItem<?>> view_item = path2view.get(item.getPathName());
if (view_item == null)
{
System.out.println("Unknown view for " + item.getPathName());
path2view.keySet().stream().forEach(System.out::println);
throw new IllegalStateException("No view item for " + item.getPathName());
}

// UI update of existing item, i.e.
// Platform.runLater(() -> TreeHelper.triggerTreeItemRefresh(view_item));
// is throttled.
// If several items update, they're all redrawn in one Platform call,
// and rapid updates of the same item are merged into just one final update
synchronized (items_to_update)
// We push the path instead of the view item in case the view item is removed
// between now and the update being handled.
synchronized (item_paths_to_update)
{
items_to_update.add(view_item);
item_paths_to_update.add(item.getPathName());
}
throttle.trigger();
updateStats();
}

/** Called by throttle to perform accumulated updates */
@SuppressWarnings("unchecked")
private void performUpdates()
{
final TreeItem<AlarmTreeItem<?>>[] view_items;
synchronized (items_to_update)

// Drain items to update in one go; we will process it later on the main thread
final String[] view_item_paths;
synchronized (item_paths_to_update)
{
// Creating a direct copy, i.e. another new LinkedHashSet<>(items_to_update),
// would be expensive, since we only need a _list_ of what's to update.
// Could use type-safe
// new ArrayList<TreeItem<AlarmTreeItem<?>>>(items_to_update)
// but that calls toArray() internally, so doing that directly
view_items = items_to_update.toArray(new TreeItem[items_to_update.size()]);
items_to_update.clear();
view_item_paths = item_paths_to_update.toArray(new String[item_paths_to_update.size()]);
item_paths_to_update.clear();
}

// Remember selection
final ObservableList<TreeItem<AlarmTreeItem<?>>> updatedSelectedItems =
FXCollections.observableArrayList(tree_view.getSelectionModel().getSelectedItems());

// How to update alarm tree cells when data changed?
// `setValue()` with a truly new value (not 'equal') should suffice,
// but there are two problems:
// Since we're currently using the alarm tree model item as a value,
// the value as seen by the TreeView remains the same.
// We could use a model item wrapper class as the cell value
// and replace it (while still holding the same model item!)
// for the TreeView to see a different wrapper value, but
// as shown in org.phoebus.applications.alarm.TreeItemUpdateDemo,
// replacing a tree cell value fails to trigger refreshes
// for certain hidden items.
// Only replacing the TreeItem gives reliable refreshes.
for (final TreeItem<AlarmTreeItem<?>> view_item : view_items)
// Top-level item has no parent, and is not visible, so we keep it
if (view_item.getParent() != null)
{
// Locate item in tree parent
final TreeItem<AlarmTreeItem<?>> parent = view_item.getParent();
final int index = parent.getChildren().indexOf(view_item);

// Create new TreeItem for that value
final AlarmTreeItem<?> value = view_item.getValue();
final TreeItem<AlarmTreeItem<?>> update = new TreeItem<>(value);
if (updatedSelectedItems.contains(view_item)) {
updatedSelectedItems.remove(view_item);
updatedSelectedItems.add(update);
Platform.runLater(() -> {
// Remember selection
final ObservableList<TreeItem<AlarmTreeItem<?>>> updatedSelectedItems =
FXCollections.observableArrayList(tree_view.getSelectionModel().getSelectedItems());

// How to update alarm tree cells when data changed?
// `setValue()` with a truly new value (not 'equal') should suffice,
// but there are two problems:
// Since we're currently using the alarm tree model item as a value,
// the value as seen by the TreeView remains the same.
// We could use a model item wrapper class as the cell value
// and replace it (while still holding the same model item!)
// for the TreeView to see a different wrapper value, but
// as shown in org.phoebus.applications.alarm.TreeItemUpdateDemo,
// replacing a tree cell value fails to trigger refreshes
// for certain hidden items.
// Only replacing the TreeItem gives reliable refreshes.
for (final String view_item_path : view_item_paths) {
final TreeItem<AlarmTreeItem<?>> view_item = path2view.get(view_item_path);

Check warning on line 547 in app/alarm/ui/src/main/java/org/phoebus/applications/alarm/ui/tree/AlarmTreeView.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Rename this local variable to match the regular expression '^[a-z][a-zA-Z0-9]*$'. 


See more on https://sonarcloud.io/project/issues?id=ControlSystemStudio_phoebus&issues=AaF9L4wAV5Jxo1fiw26U&open=AaF9L4wAV5Jxo1fiw26U&pullRequest=3987
if (view_item == null) {
// Possible if the item has been removed since it was added to the update queue
continue;
}

// Top-level item has no parent, and is not visible, so we keep it
if (view_item.getParent() != null) {
// Locate item in tree parent
final TreeItem<AlarmTreeItem<?>> parent = view_item.getParent();
final int index = parent.getChildren().indexOf(view_item);

// Create new TreeItem for that value
final AlarmTreeItem<?> value = view_item.getValue();
final TreeItem<AlarmTreeItem<?>> update = new TreeItem<>(value);
if (updatedSelectedItems.contains(view_item)) {
updatedSelectedItems.remove(view_item);
updatedSelectedItems.add(update);
}
// Move child links to new item
final ArrayList<TreeItem<AlarmTreeItem<?>>> children = new ArrayList<>(view_item.getChildren());
view_item.getChildren().clear();
update.getChildren().addAll(children);
update.setExpanded(view_item.isExpanded());

path2view.put(value.getPathName(), update);
parent.getChildren().set(index, update);
}
// Move child links to new item
final ArrayList<TreeItem<AlarmTreeItem<?>>> children = new ArrayList<>(view_item.getChildren());
view_item.getChildren().clear();
update.getChildren().addAll(children);
update.setExpanded(view_item.isExpanded());

path2view.put(value.getPathName(), update);
parent.getChildren().set(index, update);
}
// Restore selection
tree_view.getSelectionModel().clearSelection();
updatedSelectedItems.forEach(item -> tree_view.getSelectionModel().select(item));

// Restore selection
tree_view.getSelectionModel().clearSelection();
updatedSelectedItems.forEach(item -> tree_view.getSelectionModel().select(item));
});
}

/** Context menu, details depend on selected items */
Expand Down Expand Up @@ -689,25 +693,7 @@
});
}

// private long next_stats = 0;
// private final AtomicInteger update_count = new AtomicInteger();
// private volatile double updates_per_sec = 0.0;

private void updateStats()
{
// final long time = System.currentTimeMillis();
// if (time > next_stats)
// {
// final int updates = update_count.getAndSet(0);
// updates_per_sec = updates_per_sec * 0.9 + updates * 0.1;
// next_stats = time + 1000;
// System.out.format("%.2f updates/sec\n", updates_per_sec);
// }
// else
// update_count.incrementAndGet();
}

private void dumpTree(TreeItem<AlarmTreeItem<?>> item)
private static void dumpTree(final TreeItem<AlarmTreeItem<?>> item)
{
final ObservableList<TreeItem<AlarmTreeItem<?>>> children = item.getChildren();
System.out.printf("item: %s , has %d children.\n", item.getValue().getName(), children.size());
Expand Down
Loading