Repository: jabref/jabref
Issue: Performance problems with new groups search/filter and large databases

JabRef 4.0.0-dev--snapshot--2017-05-18--master--018173ebd
Windows 10 10.0 amd64 
Java 1.8.0_131

The newly implemented groups filter/search (https://github.com/JabRef/jabref/pull/2588) exhibits massive performance problems when used on a large database (>10,000 entries, ~1,000 static groups).

1. Open large database
2. Start to enter any term you want to search for in the "Filter Groups" part of the groups panel.
3. You will immediately see a massive performance decrease. It takes minutes to just complete entering the word you want to search for (because the filtering starts immediately). The groups filtering itself also takes minutes to complete. During that time JabRef behaves as if it had crashed.

This issue has been reported before (https://github.com/JabRef/jabref/pull/2588#issuecomment-283042778, https://github.com/JabRef/jabref/pull/2588#issuecomment-302629336) but I have been advised to open a new ticket so that the issue is not forgotten. 

Changed files:
src/main/java/org/jabref/gui/groups/GroupTreeController.java, src/main/java/org/jabref/gui/util/BindingsHelper.java, src/main/java/org/jabref/gui/util/RecursiveTreeItem.java, src/main/java/org/jabref/gui/groups/GroupSidePane.java, src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java

diff --git a/src/main/java/org/jabref/gui/groups/GroupSidePane.java b/src/main/java/org/jabref/gui/groups/GroupSidePane.java
index 326685e0d1..86e4392c61 100644
--- a/src/main/java/org/jabref/gui/groups/GroupSidePane.java
+++ b/src/main/java/org/jabref/gui/groups/GroupSidePane.java
@@ -1,5 +1,6 @@
 package org.jabref.gui.groups;
 
+import java.util.Collections;
 import java.util.List;
 
 import javafx.application.Platform;
@@ -16,6 +17,7 @@ import org.jabref.gui.SidePaneManager;
 import org.jabref.gui.customjfx.CustomJFXPanel;
 import org.jabref.gui.keyboard.KeyBinding;
 import org.jabref.gui.maintable.MainTableDataModel;
+import org.jabref.logic.groups.DefaultGroupsFactory;
 import org.jabref.logic.l10n.Localization;
 import org.jabref.logic.util.OS;
 import org.jabref.model.entry.FieldName;
@@ -83,8 +85,8 @@ public class GroupSidePane extends SidePaneComponent {
 
     private void updateShownEntriesAccordingToSelectedGroups(List<GroupTreeNode> selectedGroups) {
         if ((selectedGroups == null) || selectedGroups.isEmpty()) {
-            // No selected group, nothing to do
-            return;
+            // No selected group, show all entries
+            selectedGroups = Collections.singletonList(new GroupTreeNode(DefaultGroupsFactory.getAllEntriesGroup()));
         }
 
         final MatcherSet searchRules = MatcherSets.build(
diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeController.java b/src/main/java/org/jabref/gui/groups/GroupTreeController.java
index 357331789c..983bdb4c7e 100644
--- a/src/main/java/org/jabref/gui/groups/GroupTreeController.java
+++ b/src/main/java/org/jabref/gui/groups/GroupTreeController.java
@@ -2,6 +2,7 @@ package org.jabref.gui.groups;
 
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
+import java.time.Duration;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Optional;
@@ -41,12 +42,15 @@ import org.jabref.gui.util.RecursiveTreeItem;
 import org.jabref.gui.util.TaskExecutor;
 import org.jabref.gui.util.ViewModelTreeTableCellFactory;
 import org.jabref.logic.l10n.Localization;
+import org.jabref.model.groups.AllEntriesGroup;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.controlsfx.control.textfield.CustomTextField;
 import org.controlsfx.control.textfield.TextFields;
 import org.fxmisc.easybind.EasyBind;
+import org.reactfx.util.FxTimer;
+import org.reactfx.util.Timer;
 
 public class GroupTreeController extends AbstractController<GroupTreeViewModel> {
 
@@ -87,7 +91,13 @@ public class GroupTreeController extends AbstractController<GroupTreeViewModel>
                 this::updateSelection
         );
 
-        viewModel.filterTextProperty().bind(searchField.textProperty());
+        // We try to to prevent publishing changes in the search field directly to the search task that takes some time
+        // for larger group structures.
+        final Timer searchTask = FxTimer.create(Duration.ofMillis(400), () -> {
+            LOGGER.debug("Run group search " + searchField.getText());
+            viewModel.filterTextProperty().setValue(searchField.textProperty().getValue());
+        });
+        searchField.textProperty().addListener((observable, oldValue, newValue) -> searchTask.restart());
 
         groupTree.rootProperty().bind(
                 EasyBind.map(viewModel.rootGroupProperty(),
@@ -241,12 +251,12 @@ public class GroupTreeController extends AbstractController<GroupTreeViewModel>
     }
 
     private void updateSelection(List<TreeItem<GroupNodeViewModel>> newSelectedGroups) {
-        if (newSelectedGroups == null) {
+        if (newSelectedGroups == null || newSelectedGroups.isEmpty()) {
             viewModel.selectedGroupsProperty().clear();
         } else {
             List<GroupNodeViewModel> list = new ArrayList<>();
             for (TreeItem<GroupNodeViewModel> model : newSelectedGroups) {
-                if (model != null && model.getValue() != null) {
+                if (model != null && model.getValue() != null && !(model.getValue().getGroupNode().getGroup() instanceof AllEntriesGroup)) {
                     list.add(model.getValue());
                 }
             }
diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java
index bc11a195de..a4e2889b9e 100644
--- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java
+++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java
@@ -92,7 +92,7 @@ public class GroupTreeViewModel extends AbstractViewModel {
         }
 
         currentDatabase.ifPresent(database -> {
-            if (newValue == null) {
+            if (newValue == null || newValue.isEmpty()) {
                 stateManager.clearSelectedGroups(database);
             } else {
                 stateManager.setSelectedGroups(database, newValue.stream().map(GroupNodeViewModel::getGroupNode).collect(Collectors.toList()));
diff --git a/src/main/java/org/jabref/gui/util/BindingsHelper.java b/src/main/java/org/jabref/gui/util/BindingsHelper.java
index 22b26f2db8..716ca03b89 100644
--- a/src/main/java/org/jabref/gui/util/BindingsHelper.java
+++ b/src/main/java/org/jabref/gui/util/BindingsHelper.java
@@ -68,7 +68,7 @@ public class BindingsHelper {
      * the items are converted when the are inserted (and at the initialization) instead of when they are accessed.
      * Thus the initial CPU overhead and memory consumption is higher but the access to list items is quicker.
      */
-    public static <A, B> MappedList mapBacked(ObservableList<A> source, Function<A, B> mapper) {
+    public static <A, B> MappedList<B, A> mapBacked(ObservableList<A> source, Function<A, B> mapper) {
         return new MappedList<>(source, mapper);
     }
 
diff --git a/src/main/java/org/jabref/gui/util/RecursiveTreeItem.java b/src/main/java/org/jabref/gui/util/RecursiveTreeItem.java
index 1117f9dad3..c8c1f9db4c 100644
--- a/src/main/java/org/jabref/gui/util/RecursiveTreeItem.java
+++ b/src/main/java/org/jabref/gui/util/RecursiveTreeItem.java
@@ -1,8 +1,6 @@
 package org.jabref.gui.util;
 
-import java.util.List;
 import java.util.function.Predicate;
-import java.util.stream.Collectors;
 
 import javafx.beans.binding.Bindings;
 import javafx.beans.property.BooleanProperty;
@@ -24,7 +22,7 @@ public class RecursiveTreeItem<T> extends TreeItem<T> {
     private final Callback<T, BooleanProperty> expandedProperty;
     private Callback<T, ObservableList<T>> childrenFactory;
     private ObjectProperty<Predicate<T>> filter = new SimpleObjectProperty<>();
-    private FilteredList<T> children;
+    private FilteredList<RecursiveTreeItem<T>> children;
 
     public RecursiveTreeItem(final T value, Callback<T, ObservableList<T>> func) {
         this(value, func, null, null);
@@ -52,7 +50,7 @@ public class RecursiveTreeItem<T> extends TreeItem<T> {
             bindExpandedProperty(value, expandedProperty);
         }
 
-        valueProperty().addListener((obs, oldValue, newValue)-> {
+        valueProperty().addListener((obs, oldValue, newValue) -> {
             if (newValue != null) {
                 addChildrenListener(newValue);
                 bindExpandedProperty(newValue, expandedProperty);
@@ -67,44 +65,38 @@ public class RecursiveTreeItem<T> extends TreeItem<T> {
     }
 
     private void addChildrenListener(T value) {
-        children = new FilteredList<>(childrenFactory.call(value));
+        children = new FilteredList<>(
+                BindingsHelper.mapBacked(childrenFactory.call(value),
+                        child -> new RecursiveTreeItem<>(child, getGraphic(), childrenFactory, expandedProperty, filter)));
         children.predicateProperty().bind(Bindings.createObjectBinding(() -> this::showNode, filter));
 
-        addAsChildren(children, 0);
+        getChildren().addAll(0, children);
 
-        children.addListener((ListChangeListener<T>) change -> {
+        children.addListener((ListChangeListener<RecursiveTreeItem<T>>) change -> {
             while (change.next()) {
 
                 if (change.wasRemoved()) {
-                    change.getRemoved().forEach(t-> {
-                        final List<TreeItem<T>> itemsToRemove = getChildren().stream().filter(treeItem -> treeItem.getValue().equals(t)).collect(Collectors.toList());
-                        getChildren().removeAll(itemsToRemove);
-                    });
+                    getChildren().removeAll(change.getRemoved());
                 }
 
                 if (change.wasAdded()) {
-                    addAsChildren(change.getAddedSubList(), change.getFrom());
+                    getChildren().addAll(change.getFrom(), change.getAddedSubList());
                 }
             }
         });
     }
 
-    private void addAsChildren(List<? extends T> children, int startIndex) {
-        List<RecursiveTreeItem<T>> treeItems = children.stream().map(child -> new RecursiveTreeItem<>(child, getGraphic(), childrenFactory, expandedProperty, filter)).collect(Collectors.toList());
-        getChildren().addAll(startIndex, treeItems);
-    }
-
-    private boolean showNode(T t) {
+    private boolean showNode(RecursiveTreeItem<T> node) {
         if (filter.get() == null) {
             return true;
         }
 
-        if (filter.get().test(t)) {
+        if (filter.get().test(node.getValue())) {
             // Node is directly matched -> so show it
             return true;
         }
 
         // Are there children (or children of children...) that are matched? If yes we also need to show this node
-        return childrenFactory.call(t).stream().anyMatch(this::showNode);
+        return node.children.getSource().stream().anyMatch(this::showNode);
     }
 }