Improve performance (#5539)

Improve performance by: - Caching fields as KeywordList - Remove duplicate change listener - Remove preference access in loop
JabRef · Oct 29, 2019 · f88fc31 · f88fc31
1 parent 3b6c355
commit f88fc31
Show file tree

Hide file tree

Showing 7 changed files with 93 additions and 36 deletions.
diff --git a/src/main/java/org/jabref/gui/BasePanel.java b/src/main/java/org/jabref/gui/BasePanel.java
@@ -908,10 +908,6 @@ public void updateEntryEditorIfShowing() {
 
     public void markBaseChanged() {
         baseChanged = true;
-        markBasedChangedInternal();
-    }
-
-    private void markBasedChangedInternal() {
         // Put an asterisk behind the filename to indicate the database has changed.
         frame.setWindowTitle();
         DefaultTaskExecutor.runInJavaFXThread(frame::updateAllTabTitles);
@@ -1085,11 +1081,6 @@ public void cut() {
         mainTable.cut();
     }
 
-    @Subscribe
-    public void listen(EntryChangedEvent entryChangedEvent) {
-        this.markBaseChanged();
-    }
-
     private static class SearchAndOpenFile {
 
         private final BibEntry entry;

diff --git a/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java b/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java
@@ -22,12 +22,13 @@
 public class MainTableDataModel {
     private final FilteredList<BibEntryTableViewModel> entriesFiltered;
     private final SortedList<BibEntryTableViewModel> entriesSorted;
+    private final GroupViewMode groupViewMode;
 
     public MainTableDataModel(BibDatabaseContext context) {
         ObservableList<BibEntry> allEntries = BindingsHelper.forUI(context.getDatabase().getEntries());
-        
+
         ObservableList<BibEntryTableViewModel> entriesViewModel = BindingsHelper.mapBacked(allEntries, BibEntryTableViewModel::new);
-        
+
         entriesFiltered = new FilteredList<>(entriesViewModel);
         entriesFiltered.predicateProperty().bind(
                 Bindings.createObjectBinding(() -> this::isMatched,
@@ -40,6 +41,7 @@ public MainTableDataModel(BibDatabaseContext context) {
         Globals.stateManager.setActiveSearchResultSize(context, resultSize);
         // We need to wrap the list since otherwise sorting in the table does not work
         entriesSorted = new SortedList<>(entriesFiltered);
+        groupViewMode = Globals.prefs.getGroupViewMode();
     }
 
     private boolean isMatched(BibEntryTableViewModel entry) {
@@ -64,7 +66,7 @@ private Optional<MatcherSet> createGroupMatcher(List<GroupTreeNode> selectedGrou
             return Optional.empty();
         }
 
-        final MatcherSet searchRules = MatcherSets.build(Globals.prefs.getGroupViewMode() == GroupViewMode.INTERSECTION ? MatcherSets.MatcherType.AND : MatcherSets.MatcherType.OR);
+        final MatcherSet searchRules = MatcherSets.build(groupViewMode == GroupViewMode.INTERSECTION ? MatcherSets.MatcherType.AND : MatcherSets.MatcherType.OR);
 
         for (GroupTreeNode node : selectedGroups) {
             searchRules.addRule(node.getSearchMatcher());

diff --git a/src/main/java/org/jabref/gui/util/BindingsHelper.java b/src/main/java/org/jabref/gui/util/BindingsHelper.java
@@ -204,6 +204,20 @@ public static <T> ObservableValue<T> ifThenElse(ObservableValue<Boolean> conditi
         });
     }
 
+    /**
+     * Invokes {@code subscriber} for the every new value of {@code observable}, but not for the current value.
+     *
+     * @param observable observable value to subscribe to
+     * @param subscriber action to invoke for values of {@code observable}.
+     * @return a subscription that can be used to stop invoking subscriber for any further {@code observable} changes.
+     * @apiNote {@link EasyBind#subscribe(ObservableValue, Consumer)} is similar but also invokes the {@code subscriber} for the current value
+     */
+    public static <T> Subscription subscribeFuture(ObservableValue<T> observable, Consumer<? super T> subscriber) {
+        ChangeListener<? super T> listener = (obs, oldValue, newValue) -> subscriber.accept(newValue);
+        observable.addListener(listener);
+        return () -> observable.removeListener(listener);
+    }
+
     private static class BidirectionalBinding<A, B> {
 
         private final ObservableValue<A> propertyA;

diff --git a/src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java b/src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java
@@ -15,7 +15,6 @@
 import javafx.application.Platform;
 import javafx.concurrent.Task;
 
-import org.fxmisc.easybind.EasyBind;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -117,9 +116,9 @@ private <V> Task<V> getJavaFXTask(BackgroundTask<V> task) {
         Task<V> javaTask = new Task<V>() {
 
             {
-                EasyBind.subscribe(task.progressProperty(), progress -> updateProgress(progress.getWorkDone(), progress.getMax()));
-                EasyBind.subscribe(task.messageProperty(), this::updateMessage);
-                EasyBind.subscribe(task.isCanceledProperty(), cancelled -> {
+                BindingsHelper.subscribeFuture(task.progressProperty(), progress -> updateProgress(progress.getWorkDone(), progress.getMax()));
+                BindingsHelper.subscribeFuture(task.messageProperty(), this::updateMessage);
+                BindingsHelper.subscribeFuture(task.isCanceledProperty(), cancelled -> {
                     if (cancelled) {
                         cancel();
                     }

diff --git a/src/main/java/org/jabref/model/entry/BibEntry.java b/src/main/java/org/jabref/model/entry/BibEntry.java
@@ -36,6 +36,7 @@
 import org.jabref.model.entry.types.StandardEntryType;
 import org.jabref.model.strings.LatexToUnicodeAdapter;
 import org.jabref.model.strings.StringUtil;
+import org.jabref.model.util.MultiKeyMap;
 
 import com.google.common.base.Strings;
 import com.google.common.eventbus.EventBus;
@@ -53,14 +54,22 @@ public class BibEntry implements Cloneable {
     private static final Logger LOGGER = LoggerFactory.getLogger(BibEntry.class);
     private static final Pattern REMOVE_TRAILING_WHITESPACE = Pattern.compile("\\s+$");
     private final SharedBibEntryData sharedBibEntryData;
+
     /**
      * Map to store the words in every field
      */
     private final Map<Field, Set<String>> fieldsAsWords = new HashMap<>();
+
     /**
      * Cache that stores latex free versions of fields.
      */
     private final Map<Field, String> latexFreeFields = new ConcurrentHashMap<>();
+
+    /**
+     * Cache that stores the field as keyword lists (format <Field, Separator, Keyword list>)
+     */
+    private MultiKeyMap<Field, Character, KeywordList> fieldsAsKeywords = new MultiKeyMap<>();
+
     private final EventBus eventBus = new EventBus();
     private String id;
     private final ObjectProperty<EntryType> type = new SimpleObjectProperty<>(DEFAULT_TYPE);
@@ -720,8 +729,7 @@ public void addKeywords(Collection<String> keywords, Character delimiter) {
     }
 
     public KeywordList getKeywords(Character delimiter) {
-        Optional<String> keywordsContent = getField(StandardField.KEYWORDS);
-        return keywordsContent.map(content -> KeywordList.parse(content, delimiter)).orElse(new KeywordList());
+        return getFieldAsKeywords(StandardField.KEYWORDS, delimiter);
     }
 
     public KeywordList getResolvedKeywords(Character delimiter, BibDatabase database) {
@@ -824,33 +832,46 @@ public Set<String> getFieldAsWords(Field field) {
         }
     }
 
+    public KeywordList getFieldAsKeywords(Field field, Character keywordSeparator) {
+        Optional<KeywordList> storedList = fieldsAsKeywords.get(field, keywordSeparator);
+        if (storedList.isPresent()) {
+            return storedList.get();
+        } else {
+            KeywordList keywords = getField(field)
+                    .map(content -> KeywordList.parse(content, keywordSeparator))
+                    .orElse(new KeywordList());
+            fieldsAsKeywords.put(field, keywordSeparator, keywords);
+            return keywords;
+        }
+    }
+
     public Optional<FieldChange> clearCiteKey() {
         return clearField(InternalField.KEY_FIELD);
     }
 
     private void invalidateFieldCache(Field field) {
         latexFreeFields.remove(field);
         fieldsAsWords.remove(field);
+        fieldsAsKeywords.remove(field);
     }
 
     public Optional<String> getLatexFreeField(Field field) {
-        if (!hasField(field) && !InternalField.TYPE_HEADER.equals(field)) {
-            return Optional.empty();
-        } else if (latexFreeFields.containsKey(field)) {
-            return Optional.ofNullable(latexFreeFields.get(field));
-        } else if (InternalField.KEY_FIELD.equals(field)) {
+        if (InternalField.KEY_FIELD.equals(field)) {
             // the key field should not be converted
-            Optional<String> citeKey = getCiteKeyOptional();
-            latexFreeFields.put(field, citeKey.get());
-            return citeKey;
+            return getCiteKeyOptional();
         } else if (InternalField.TYPE_HEADER.equals(field)) {
-            String typeName = type.get().getDisplayName();
-            latexFreeFields.put(field, typeName);
-            return Optional.of(typeName);
+            return Optional.of(type.get().getDisplayName());
+        } else if (latexFreeFields.containsKey(field)) {
+            return Optional.ofNullable(latexFreeFields.get(field));
         } else {
-            String latexFreeField = LatexToUnicodeAdapter.format(getField(field).get()).intern();
-            latexFreeFields.put(field, latexFreeField);
-            return Optional.of(latexFreeField);
+            Optional<String> fieldValue = getField(field);
+            if (fieldValue.isPresent()) {
+                String latexFreeField = LatexToUnicodeAdapter.format(fieldValue.get()).intern();
+                latexFreeFields.put(field, latexFreeField);
+                return Optional.of(latexFreeField);
+            } else {
+                return Optional.empty();
+            }
         }
     }
 

diff --git a/src/main/java/org/jabref/model/groups/WordKeywordGroup.java b/src/main/java/org/jabref/model/groups/WordKeywordGroup.java
@@ -2,7 +2,6 @@
 
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Objects;
@@ -119,9 +118,7 @@ private Set<String> getFieldContentAsWords(BibEntry entry) {
             if (InternalField.TYPE_HEADER.equals(searchField)) {
                 return searchWords.stream().filter(word -> entry.getType().getName().equalsIgnoreCase(word)).collect(Collectors.toSet());
             }
-            return entry.getField(searchField)
-                    .map(content -> KeywordList.parse(content, keywordSeparator).toStringList())
-                    .orElse(Collections.emptySet());
+            return entry.getFieldAsKeywords(searchField, keywordSeparator).toStringList();
         } else {
             return entry.getFieldAsWords(searchField);
         }

diff --git a/src/main/java/org/jabref/model/util/MultiKeyMap.java b/src/main/java/org/jabref/model/util/MultiKeyMap.java
@@ -0,0 +1,33 @@
+package org.jabref.model.util;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+
+public class MultiKeyMap<K1, K2, V> {
+    private final Map<K1, Map<K2, V>> map = new HashMap<>();
+
+    public Optional<V> get(K1 key1, K2 key2) {
+        Map<K2, V> metaValue = map.get(key1);
+        if (metaValue == null) {
+            return Optional.empty();
+        } else {
+            return Optional.ofNullable(metaValue.get(key2));
+        }
+    }
+
+    public void put(K1 key1, K2 key2, V value) {
+        Map<K2, V> metaValue = map.get(key1);
+        if (metaValue == null) {
+            Map<K2, V> newMetaValue = new HashMap<>();
+            newMetaValue.put(key2, value);
+            map.put(key1, newMetaValue);
+        } else {
+            metaValue.put(key2, value);
+        }
+    }
+
+    public void remove(K1 key1) {
+        map.remove(key1);
+    }
+}