Skip to content

WIP: Dbsync performance improvements #8496

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

Closed
Siedlerchr wants to merge 26 commits into from
Closed

WIP: Dbsync performance improvements #8496

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
2b78ac7
improve db sync
Siedlerchr Feb 7, 2022
25888f4
Handling of field change events as background task
kai-franke-dlr-pi Feb 9, 2022
90e0969
fix architecture violation
Siedlerchr Feb 9, 2022
fbaf907
Merge branch 'Kai-Franke-DLR-dbsync' into dbsync
Siedlerchr Feb 9, 2022
e608b1f
try to fix tests
Siedlerchr Feb 9, 2022
a2dd44e
checkstyle
Siedlerchr Feb 9, 2022
04842c0
use default task executor
Siedlerchr Feb 9, 2022
30a12f9
fix missing task executor
Siedlerchr Feb 9, 2022
4251dc0
fix some threding errors
Siedlerchr Feb 9, 2022
6fc9ecb
implement dummy fx thread
Siedlerchr Feb 9, 2022
51aa6a6
use current thread task
Siedlerchr Feb 9, 2022
cf209ee
fix checkstyle
Siedlerchr Feb 9, 2022
a35b4b0
remove useless fx code
Siedlerchr Feb 9, 2022
033a57e
convert default task executor to execute dummy fx thread
Siedlerchr Feb 9, 2022
e7b8219
make checkstyle and olly happy
Siedlerchr Feb 9, 2022
e73f8bc
Fix UI Thread blocking
Siedlerchr Feb 11, 2022
d8ba668
Fix UI Thraddlist blocking
Siedlerchr Feb 13, 2022
c1f74a5
try to improve threading
Siedlerchr Feb 13, 2022
08f243a
Merge remote-tracking branch 'upstream/main' into dbsync
Siedlerchr Feb 14, 2022
4b936a3
experiment with threading
Siedlerchr Feb 19, 2022
bf51775
Merge remote-tracking branch 'upstream/main' into dbsync
Siedlerchr Sep 2, 2022
d0e7237
test further
Siedlerchr Sep 2, 2022
bd0fc17
Merge remote-tracking branch 'upstream/main' into dbsync
Siedlerchr Sep 3, 2022
391a274
fix item
Siedlerchr Sep 3, 2022
311f81b
Merge remote-tracking branch 'upstream/main' into dbsync
Siedlerchr Sep 3, 2022
8010a47
argh
Siedlerchr Sep 3, 2022
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
4 changes: 2 additions & 2 deletions src/main/java/org/jabref/gui/shared/SharedDatabaseUIManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ public LibraryTab openNewSharedDatabaseTab(DBMSConnectionProperties dbmsConnecti

BibDatabaseContext bibDatabaseContext = new BibDatabaseContext();
bibDatabaseContext.setMode(Globals.prefs.getGeneralPreferences().getDefaultBibDatabaseMode());
DBMSSynchronizer synchronizer = new DBMSSynchronizer(bibDatabaseContext, Globals.prefs.getKeywordDelimiter(), Globals.prefs.getGlobalCitationKeyPattern(), Globals.getFileUpdateMonitor());
DBMSSynchronizer synchronizer = new DBMSSynchronizer(bibDatabaseContext, Globals.prefs.getKeywordDelimiter(), Globals.prefs.getGlobalCitationKeyPattern(), Globals.getFileUpdateMonitor(), Globals.TASK_EXECUTOR);
bibDatabaseContext.convertToSharedDatabase(synchronizer);

dbmsSynchronizer = bibDatabaseContext.getDBMSSynchronizer();
Expand All @@ -166,7 +166,7 @@ public void openSharedDatabaseFromParserResult(ParserResult parserResult)

BibDatabaseContext bibDatabaseContext = new BibDatabaseContext();
bibDatabaseContext.setMode(Globals.prefs.getGeneralPreferences().getDefaultBibDatabaseMode());
DBMSSynchronizer synchronizer = new DBMSSynchronizer(bibDatabaseContext, Globals.prefs.getKeywordDelimiter(), Globals.prefs.getGlobalCitationKeyPattern(), Globals.getFileUpdateMonitor());
DBMSSynchronizer synchronizer = new DBMSSynchronizer(bibDatabaseContext, Globals.prefs.getKeywordDelimiter(), Globals.prefs.getGlobalCitationKeyPattern(), Globals.getFileUpdateMonitor(), Globals.TASK_EXECUTOR);
bibDatabaseContext.convertToSharedDatabase(synchronizer);

bibDatabaseContext.getDatabase().setSharedDatabaseID(sharedDatabaseID);
Expand Down
8 changes: 8 additions & 0 deletions src/main/java/org/jabref/gui/util/CurrentThreadTaskExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,4 +106,12 @@ public boolean isDone() {
return true;
}
}

/**
* Executes the runnable on the current thread, used for testing
*/
@Override
public void runInFXThread(Runnable runnable) {
runnable.run();
}
}
7 changes: 6 additions & 1 deletion src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ public DelayTaskThrottler createThrottler(int delay) {
}

private <V> Task<V> getJavaFXTask(BackgroundTask<V> task) {
Task<V> javaTask = new Task<V>() {
Task<V> javaTask = new Task<>() {

{
this.updateMessage(task.messageProperty().get());
Expand Down Expand Up @@ -184,4 +184,9 @@ private Exception convertToException(Throwable throwable) {
return new Exception(throwable);
}
}

@Override
public void runInFXThread(Runnable runnable) {
DefaultTaskExecutor.runInJavaFXThread(runnable);
}
}
8 changes: 8 additions & 0 deletions src/main/java/org/jabref/gui/util/TaskExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,12 @@ public interface TaskExecutor {
* Creates a new task throttler, and registers it so that it gets properly shutdown.
*/
DelayTaskThrottler createThrottler(int delay);

/**
* If {@link TaskExecutor} is an instance of {@link DefaultTaskExecutor} then it is equivalent of calling {@link DefaultTaskExecutor#runInJavaFXThread(Runnable)}
* otherwise it is executed on the same thread as the caller. This is extremely useful for testing code that does calls to the FXThread
* @param runnable The runnable to execute
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Sep 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.javadoc.RequireEmptyLineBeforeBlockTagGroupCheck> reported by reviewdog 🐶
Javadoc tag '@param' should be preceded with an empty line.

*/
void runInFXThread(Runnable runnable);
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Sep 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.regexp.RegexpMultilineCheck> reported by reviewdog 🐶
Blank line at end of block should be removed


}
4 changes: 2 additions & 2 deletions src/main/java/org/jabref/logic/shared/DBMSProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,12 +282,12 @@ protected void insertIntoFieldTable(List<BibEntry> bibEntries) {
}

/**
* Updates the whole {@link BibEntry} on shared database.
* Updates the whole {@link BibEntry } on shared database.
*
* @param localBibEntry {@link BibEntry} affected by changes
* @throws SQLException
*/
public void updateEntry(BibEntry localBibEntry) throws OfflineLockException, SQLException {
public synchronized void updateEntry(BibEntry localBibEntry) throws OfflineLockException, SQLException {
connection.setAutoCommit(false); // disable auto commit due to transaction

try {
Expand Down
33 changes: 23 additions & 10 deletions src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
import java.util.Set;
import java.util.stream.Collectors;

import org.jabref.gui.util.BackgroundTask;
import org.jabref.gui.util.TaskExecutor;
import org.jabref.logic.citationkeypattern.GlobalCitationKeyPattern;
import org.jabref.logic.exporter.BibDatabaseWriter;
import org.jabref.logic.exporter.MetaDataSerializer;
Expand Down Expand Up @@ -55,9 +57,10 @@ public class DBMSSynchronizer implements DatabaseSynchronizer {
private final GlobalCitationKeyPattern globalCiteKeyPattern;
private final FileUpdateMonitor fileMonitor;
private Optional<BibEntry> lastEntryChanged;
private final TaskExecutor taskExecutor;

public DBMSSynchronizer(BibDatabaseContext bibDatabaseContext, Character keywordSeparator,
GlobalCitationKeyPattern globalCiteKeyPattern, FileUpdateMonitor fileMonitor) {
GlobalCitationKeyPattern globalCiteKeyPattern, FileUpdateMonitor fileMonitor, TaskExecutor taskExecutor) {
this.bibDatabaseContext = Objects.requireNonNull(bibDatabaseContext);
this.bibDatabase = bibDatabaseContext.getDatabase();
this.metaData = bibDatabaseContext.getMetaData();
Expand All @@ -66,6 +69,7 @@ public DBMSSynchronizer(BibDatabaseContext bibDatabaseContext, Character keyword
this.keywordSeparator = keywordSeparator;
this.globalCiteKeyPattern = Objects.requireNonNull(globalCiteKeyPattern);
this.lastEntryChanged = Optional.empty();
this.taskExecutor = taskExecutor;
}

/**
Expand Down Expand Up @@ -98,10 +102,12 @@ public void listen(FieldChangedEvent event) {
// While synchronizing the local database (see synchronizeLocalDatabase() below), some EntriesEvents may be posted.
// In this case DBSynchronizer should not try to update the bibEntry entry again (but it would not harm).
if (isPresentLocalBibEntry(bibEntry) && isEventSourceAccepted(event) && checkCurrentConnection() && !event.isFilteredOut()) {
synchronizeLocalMetaData();
pullWithLastEntry();
synchronizeSharedEntry(bibEntry);
synchronizeLocalDatabase(); // Pull changes for the case that there were some
BackgroundTask.wrap(() -> {
synchronizeLocalMetaData();
pullWithLastEntry();
synchronizeSharedEntry(bibEntry);
synchronizeLocalDatabase(); // Pull changes for the case that there were some
}).executeWith(taskExecutor);
} else {
// Set new BibEntry that has been changed last
lastEntryChanged = Optional.of(bibEntry);
Expand Down Expand Up @@ -166,8 +172,12 @@ public void initializeDatabases() throws DatabaseNotSupportedException {
}

dbmsProcessor.startNotificationListener(this);
synchronizeLocalMetaData();
synchronizeLocalDatabase();

BackgroundTask.wrap(() -> {
synchronizeLocalMetaData();
synchronizeLocalDatabase();
}).executeWith(taskExecutor);

}

/**
Expand Down Expand Up @@ -221,7 +231,8 @@ public void synchronizeLocalDatabase() {

if (!entriesToInsertIntoLocalDatabase.isEmpty()) {
// in case entries should be added into the local database, insert them
bibDatabase.insertEntries(dbmsProcessor.getSharedEntries(entriesToInsertIntoLocalDatabase), EntriesEventSource.SHARED);
taskExecutor.runInFXThread(() ->
Copy link
Copy Markdown
Member

@tobiasdiez tobiasdiez Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really necessary? I would say it's the job of the ui to make sure it handles notifications about new entries on the right thread, and not of the logic. In fact, the logic shouldn't even know about the concept of fx-threads.

Copy link
Copy Markdown
Member Author

@Siedlerchr Siedlerchr Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, while I agree with you that from an architectural standpoint this looks odd, the key problem is that if you insert anything in the bib database or modify a field, the GUI observables in the entry editor or maintable are called as well because they are bound to the bib database object.

Copy link
Copy Markdown
Member

@tobiasdiez tobiasdiez Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this situation, we have the "forUi" helper

jabref/src/main/java/org/jabref/gui/util/BindingsHelper.java

Lines 165 to 170 in de114d5

/**
* Returns a wrapper around the given list that posts changes on the JavaFX thread.
*/
public static <T> ObservableList<T> forUI(ObservableList<T> list) {
return new UiThreadList<>(list);
}

Copy link
Copy Markdown
Member Author

@Siedlerchr Siedlerchr Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I have encountered this before, this leads to a deadlock under certain conditions:
Comment that line out and connect to the shared SQL library in #8485
See here. I can only solve this by adding a timeout in the UI Thread list

grafik

Copy link
Copy Markdown
Member

@tobiasdiez tobiasdiez Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: applications should avoid flooding JavaFX with too many pending Runnables. Otherwise, the application may become unresponsive. Applications are encouraged to batch up multiple operations into fewer runLater calls. Additionally, long-running operations should be done on a background thread where possible, freeing up the JavaFX Application Thread for GUI operations.

That might be the reason. How many source changes does the database trigger?

Copy link
Copy Markdown
Member

@tobiasdiez tobiasdiez Feb 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice analysis, thanks!

What I don't understand is why does the background thread pauses in the UiThreadList? Shouldn't it go on, sync the rest of the changes and finally give the synced entry list free?

Maybe it works if one adds the forUi call at the last position here

jabref/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java

Line 57 in ba5beb2

entriesSorted = new SortedList<>(entriesFiltered);
(or for the entriesFiltered, if otherwise the search no longer works). Then all the syncs / updates of the mapped database would still run in the background thread.

Copy link
Copy Markdown
Member Author

@Siedlerchr Siedlerchr Feb 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the BackgroundThread only indirectly pauses in the UIList because

  1. When calling add() on the SynchronizedLIst, Background Thread acquires a mutex for the whole list.
  2. it's waiting for the CountDownLatch that never happens; this is why I tested to solve it with a timeout in the latch.
     CountDownLatch latch = new CountDownLatch(1);
            Platform.runLater(() -> {
                fireChange(change);
                latch.countDown(); //never gets called, because we are stil in fireChange  and bocked
            });

            try {
                latch.await();
               // boolean latched = latch.await(3L, TimeUnit.SECONDS);
               // LOGGER.debug("Result from latch {}", latched); // if this is false, it was canceled by the timeout

            } catch (InterruptedException e) {
                LOGGER.error("Error while running on JavaFX thread", e);
            } finally {
                latch.countDown();
            }

Copy link
Copy Markdown
Member Author

@Siedlerchr Siedlerchr Feb 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tobiasdiez I could solve it for the table, but I discovered that this also affects the GroupNodeViewModel, where a listener is registered. this can happend when switching lbraries now.
May also need to adapt it for the FilteredList...

entriesList = databaseContext.getDatabase().getEntries();
entriesList.addListener(this::onDatabaseChanged);

Copy link
Copy Markdown
Member

@tobiasdiez tobiasdiez Feb 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhh, maybe it's a good idea to step back and try to come up with a general solution to these problems?
Off the top of my head, we have two ways to handle background threads that result in updates to the UI:

  • Background threads are only allowed to calculate things, but never change any of the data model classes directly.
  • Background threads are allowed to change data models, but at every point where the data model classes are bind to the UI we make sure that the updates are happening on the JavaFX thread (that essentially only concerns the main table, group view and entry editor).

I think both ways have advantages and disadvantages. I don't have a strong preference right now, do you? But I do think it would make sense to decide which approach we take and then do changes accordingly.

Copy link
Copy Markdown
Member Author

@Siedlerchr Siedlerchr Feb 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From an architectural standpoint, updating the model should not have direct influences of the UI. The UI should take care that it is updating things in the right thread. This is more or less our current architecture. (2nd approach).

The key problem is more that we have SynchronizedList which is bound to the UI.
It has a mutex on all methods which will be held when elements are inserted. So while Elements are inserted a lock is held. Firing Changes on the FX Thread will always access the underlying list.
I will see if this work with the forUI for groups as well

bibDatabase.insertEntries(dbmsProcessor.getSharedEntries(entriesToInsertIntoLocalDatabase), EntriesEventSource.SHARED));
}
}

Expand Down Expand Up @@ -255,7 +266,9 @@ public void synchronizeSharedEntry(BibEntry bibEntry) {
BibDatabaseWriter.applySaveActions(bibEntry, metaData); // perform possibly existing save actions
dbmsProcessor.updateEntry(bibEntry);
} catch (OfflineLockException exception) {
eventBus.post(new UpdateRefusedEvent(bibDatabaseContext, exception.getLocalBibEntry(), exception.getSharedBibEntry()));
taskExecutor.runInFXThread(() -> {
eventBus.post(new UpdateRefusedEvent(bibDatabaseContext, exception.getLocalBibEntry(), exception.getSharedBibEntry()));
});
} catch (SQLException e) {
LOGGER.error("SQL Error", e);
}
Expand Down Expand Up @@ -354,7 +367,7 @@ private void pullWithLastEntry() {
*
* @return <code>true</code> if the connection is valid, else <code>false</code>.
*/
public boolean checkCurrentConnection() {
public synchronized boolean checkCurrentConnection() {
try {
boolean isValid = currentConnection.isValid(0);
if (!isValid) {
Expand Down
3 changes: 2 additions & 1 deletion src/test/java/org/jabref/logic/shared/DBMSSynchronizerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import java.util.List;
import java.util.Map;

import org.jabref.gui.util.CurrentThreadTaskExecutor;
import org.jabref.logic.citationkeypattern.GlobalCitationKeyPattern;
import org.jabref.logic.cleanup.FieldFormatterCleanup;
import org.jabref.logic.cleanup.FieldFormatterCleanups;
Expand Down Expand Up @@ -61,7 +62,7 @@ public void setup() throws Exception {
bibDatabase = new BibDatabase();
BibDatabaseContext context = new BibDatabaseContext(bibDatabase);

dbmsSynchronizer = new DBMSSynchronizer(context, ',', pattern, new DummyFileUpdateMonitor());
dbmsSynchronizer = new DBMSSynchronizer(context, ',', pattern, new DummyFileUpdateMonitor(), new CurrentThreadTaskExecutor());
bibDatabase.registerListener(dbmsSynchronizer);

dbmsSynchronizer.openSharedDatabase(dbmsConnection);
Expand Down
5 changes: 3 additions & 2 deletions src/test/java/org/jabref/logic/shared/SynchronizationTestSimulator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import java.sql.SQLException;
import java.util.List;

import org.jabref.gui.util.CurrentThreadTaskExecutor;
import org.jabref.logic.citationkeypattern.GlobalCitationKeyPattern;
import org.jabref.model.database.BibDatabaseContext;
import org.jabref.model.entry.BibEntry;
Expand Down Expand Up @@ -48,12 +49,12 @@ public void setup() throws Exception {
TestManager.clearTables(dbmsConnection);

clientContextA = new BibDatabaseContext();
DBMSSynchronizer synchronizerA = new DBMSSynchronizer(clientContextA, ',', pattern, new DummyFileUpdateMonitor());
DBMSSynchronizer synchronizerA = new DBMSSynchronizer(clientContextA, ',', pattern, new DummyFileUpdateMonitor(), new CurrentThreadTaskExecutor());
clientContextA.convertToSharedDatabase(synchronizerA);
clientContextA.getDBMSSynchronizer().openSharedDatabase(dbmsConnection);

clientContextB = new BibDatabaseContext();
DBMSSynchronizer synchronizerB = new DBMSSynchronizer(clientContextB, ',', pattern, new DummyFileUpdateMonitor());
DBMSSynchronizer synchronizerB = new DBMSSynchronizer(clientContextB, ',', pattern, new DummyFileUpdateMonitor(), new CurrentThreadTaskExecutor());
clientContextB.convertToSharedDatabase(synchronizerB);
// use a second connection, because this is another client (typically on another machine)
clientContextB.getDBMSSynchronizer().openSharedDatabase(TestConnector.getTestDBMSConnection(TestManager.getDBMSTypeTestParameter()));
Expand Down