Skip to content
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.

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

Improve performance by throttling database change events #5843

Merged
merged 2 commits into from
Jan 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
- We fixed an issue where the Medline fetcher was only working when JabRef was running from source. [#5645](https://github.com/JabRef/jabref/issues/5645)
- We fixed some visual issues in the dark theme. [#5764](https://github.com/JabRef/jabref/pull/5764) [#5753](https://github.com/JabRef/jabref/issues/5753)
- We fixed an issue where non-default previews didn't handle unicode characters. [#5779](https://github.com/JabRef/jabref/issues/5779)
- We improved the performance, especially changing field values in the entry should feel smoother now.
- We fixed an issue where the ampersand character wasn't rendering correctly on previews. [#3840](https://github.com/JabRef/jabref/issues/3840)
- We fixed an issue where an erroneous "The library has been modified by another program" message was shown when saving. [#4877](https://github.com/JabRef/jabref/issues/4877)
- We fixed an issue where the file extension was missing after downloading a file (we now fall-back to pdf). [#5816](https://github.com/JabRef/jabref/issues/5816)
Expand Down
5 changes: 4 additions & 1 deletion src/main/java/org/jabref/gui/groups/GroupNodeViewModel.java
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.jabref.gui.util.TaskExecutor;
import org.jabref.logic.groups.DefaultGroupsFactory;
import org.jabref.logic.layout.format.LatexToUnicodeFormatter;
import org.jabref.logic.util.DelayTaskThrottler;
import org.jabref.model.FieldChange;
import org.jabref.model.database.BibDatabaseContext;
import org.jabref.model.entry.BibEntry;
Expand Down Expand Up @@ -56,6 +57,7 @@ public class GroupNodeViewModel {
private final TaskExecutor taskExecutor;
private final CustomLocalDragboard localDragBoard;
private final ObservableList<BibEntry> entriesList;
private final DelayTaskThrottler throttler;

public GroupNodeViewModel(BibDatabaseContext databaseContext, StateManager stateManager, TaskExecutor taskExecutor, GroupTreeNode groupNode, CustomLocalDragboard localDragBoard) {
this.databaseContext = Objects.requireNonNull(databaseContext);
Expand Down Expand Up @@ -88,6 +90,7 @@ public GroupNodeViewModel(BibDatabaseContext databaseContext, StateManager state
// The wrapper created by the FXCollections will set a weak listener on the wrapped list. This weak listener gets garbage collected. Hence, we need to maintain a reference to this list.
entriesList = databaseContext.getDatabase().getEntries();
entriesList.addListener(this::onDatabaseChanged);
throttler = new DelayTaskThrottler(1000);

ObservableList<Boolean> selectedEntriesMatchStatus = EasyBind.map(stateManager.getSelectedEntries(), groupNode::matches);
anySelectedEntriesMatched = BindingsHelper.any(selectedEntriesMatchStatus, matched -> matched);
Expand Down Expand Up @@ -218,7 +221,7 @@ public GroupTreeNode getGroupNode() {
* Gets invoked if an entry in the current database changes.
*/
private void onDatabaseChanged(ListChangeListener.Change<? extends BibEntry> change) {
calculateNumberOfMatches();
throttler.schedule(this::calculateNumberOfMatches);
}

private void calculateNumberOfMatches() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,9 @@

import java.util.HashSet;
import java.util.Set;
import java.util.concurrent.Future;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.concurrent.TimeUnit;

import org.jabref.logic.util.DelayTaskThrottler;
import org.jabref.model.database.BibDatabaseContext;
import org.jabref.model.database.event.AutosaveEvent;
import org.jabref.model.database.event.BibDatabaseContextChangedEvent;
Expand All @@ -25,40 +23,34 @@
public class AutosaveManager {

private static final Logger LOGGER = LoggerFactory.getLogger(AutosaveManager.class);
private static final int AUTO_SAVE_DELAY = 200;

private static Set<AutosaveManager> runningInstances = new HashSet<>();

private final BibDatabaseContext bibDatabaseContext;
private final ScheduledExecutorService executor;

private final EventBus eventBus;
private final CoarseChangeFilter changeFilter;
private Future<?> scheduledSaveAction;
private final DelayTaskThrottler throttler;

private AutosaveManager(BibDatabaseContext bibDatabaseContext) {
this.bibDatabaseContext = bibDatabaseContext;
ScheduledThreadPoolExecutor executor = new ScheduledThreadPoolExecutor(1);
executor.setRemoveOnCancelPolicy(true); // This prevents memory leaks
this.executor = executor;
this.throttler = new DelayTaskThrottler(2000);
this.eventBus = new EventBus();
this.changeFilter = new CoarseChangeFilter(bibDatabaseContext);
changeFilter.registerListener(this);
}

@Subscribe
public synchronized void listen(@SuppressWarnings("unused") BibDatabaseContextChangedEvent event) {
if (scheduledSaveAction != null) {
scheduledSaveAction.cancel(false);
}
scheduledSaveAction = executor.schedule(() -> {
throttler.schedule(() -> {
eventBus.post(new AutosaveEvent());
}, AUTO_SAVE_DELAY, TimeUnit.MILLISECONDS);
});
}

private void shutdown() {
changeFilter.unregisterListener(this);
changeFilter.shutdown();
executor.shutdown();
throttler.shutdown();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,14 @@
import java.util.HashSet;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.RejectedExecutionException;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.TimeUnit;

import org.jabref.logic.bibtex.InvalidFieldValueException;
import org.jabref.logic.exporter.AtomicFileWriter;
import org.jabref.logic.exporter.BibtexDatabaseWriter;
import org.jabref.logic.exporter.SavePreferences;
import org.jabref.logic.util.DelayTaskThrottler;
import org.jabref.logic.util.io.FileUtil;
import org.jabref.model.database.BibDatabaseContext;
import org.jabref.model.database.event.BibDatabaseContextChangedEvent;
Expand Down Expand Up @@ -46,17 +43,15 @@ public class BackupManager {

private final BibDatabaseContext bibDatabaseContext;
private final JabRefPreferences preferences;
private final ExecutorService executor;
private final Runnable backupTask = () -> determineBackupPath().ifPresent(this::performBackup);
private final DelayTaskThrottler throttler;
private final CoarseChangeFilter changeFilter;
private final BibEntryTypesManager entryTypesManager;

private BackupManager(BibDatabaseContext bibDatabaseContext, BibEntryTypesManager entryTypesManager, JabRefPreferences preferences) {
this.bibDatabaseContext = bibDatabaseContext;
this.entryTypesManager = entryTypesManager;
this.preferences = preferences;
BlockingQueue<Runnable> workerQueue = new ArrayBlockingQueue<>(1);
this.executor = new ThreadPoolExecutor(1, 1, 0, TimeUnit.SECONDS, workerQueue);
this.throttler = new DelayTaskThrottler(15000);

changeFilter = new CoarseChangeFilter(bibDatabaseContext);
changeFilter.registerListener(this);
Expand All @@ -71,8 +66,6 @@ static Path getBackupPath(Path originalPath) {
* As long as no database file is present in {@link BibDatabaseContext}, the {@link BackupManager} will do nothing.
*
* @param bibDatabaseContext Associated {@link BibDatabaseContext}
* @param entryTypesManager
* @param preferences
*/
public static BackupManager start(BibDatabaseContext bibDatabaseContext, BibEntryTypesManager entryTypesManager, JabRefPreferences preferences) {
BackupManager backupManager = new BackupManager(bibDatabaseContext, entryTypesManager, preferences);
Expand Down Expand Up @@ -151,11 +144,7 @@ public synchronized void listen(@SuppressWarnings("unused") BibDatabaseContextCh
}

private void startBackupTask() {
try {
executor.submit(backupTask);
} catch (RejectedExecutionException e) {
LOGGER.debug("Rejecting while another backup process is already running.");
}
throttler.schedule(() -> determineBackupPath().ifPresent(this::performBackup));
}

/**
Expand All @@ -165,7 +154,7 @@ private void startBackupTask() {
private void shutdown() {
changeFilter.unregisterListener(this);
changeFilter.shutdown();
executor.shutdown();
throttler.shutdown();
determineBackupPath().ifPresent(this::deleteBackupFile);
}

Expand Down
51 changes: 51 additions & 0 deletions src/main/java/org/jabref/logic/util/DelayTaskThrottler.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package org.jabref.logic.util;

import java.util.concurrent.Future;
import java.util.concurrent.RejectedExecutionException;
import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.concurrent.TimeUnit;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* This class allows to throttle a list of tasks.
* Use case: you have an event that occurs often, and every time you want to invoke the same task.
* However, if a lot of events happen in a relatively short time span, then only one task should be invoked.
*
* @implNote Once {@link #schedule(Runnable)} is called, the task is delayed for a given time span.
* If during this time, {@link #schedule(Runnable)} is called again, then the original task is canceled and the new one scheduled.
*/
public class DelayTaskThrottler {

private static final Logger LOGGER = LoggerFactory.getLogger(DelayTaskThrottler.class);

private final ScheduledThreadPoolExecutor executor;
private final int delay;

private Future<?> scheduledTask;

/**
* @param delay delay in milliseconds
*/
public DelayTaskThrottler(int delay) {
this.delay = delay;
this.executor = new ScheduledThreadPoolExecutor(1);
this.executor.setRemoveOnCancelPolicy(true);
}

public void schedule(Runnable command) {
if (scheduledTask != null) {
scheduledTask.cancel(false);
}
try {
scheduledTask = executor.schedule(command, delay, TimeUnit.MILLISECONDS);
} catch (RejectedExecutionException e) {
LOGGER.debug("Rejecting while another process is already running.");
}
}

public void shutdown() {
executor.shutdown();
}
}