WIP: Dbsync performance improvements #8496

Siedlerchr · 2022-02-09T19:06:38Z

I could not push to the original one
Follow up to #8494

Introduced a dummy FXThread method for the CurrentTaskExecutor to fix the tests and added some synchronized to prevent errors with transactions. Need to check if we need this for other udpates as well

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

* Kai-Franke-DLR-dbsync: fix architecture violation Handling of field change events as background task

add javadoc

revert actions changes

tobiasdiez · 2022-02-11T09:15:30Z

src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java

@@ -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(() ->


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.

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.

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);

}

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

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?

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.

the BackgroundThread only indirectly pauses in the UIList because

When calling add() on the SynchronizedLIst, Background Thread acquires a mutex for the whole list.

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(); }

@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);

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.

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

* upstream/main: Update Gradle Wrapper from 7.3.3 to 7.4. (#8499) Bump flexmark-ext-gfm-strikethrough from 0.62.2 to 0.64.0 (#8500) Bump tika-core from 2.2.1 to 2.3.0 (#8501) Bump classgraph from 4.8.138 to 4.8.139 (#8502) Fix for opening console on active database (#8492)

Siedlerchr · 2022-02-26T18:57:16Z

This need some more thinking how to solve blocking threads when the EventBus fires. Therefore I close this for the moment and reopen when I have more insights

* upstream/main: (387 commits) Show a warning in the merge dialog when authors are the same but formatted differently (#9088) Fix subdatabase from aux on cli (#9117) Visual improvements to LinkedFilesEditor (#9114) SLR Remove "last-search-date" (#9116) Hide diffs when one of the field values is blank a.k.a no conflict (#9110) Squashed 'buildres/csl/csl-locales/' changes from e637746677..b2afeb4d87 Squashed 'buildres/csl/csl-styles/' changes from c750b6e..8d69f16 Fix title case capitalization after en-dash characters (#9102) Update journal abbrev list (#9109) Fix CSL rendering in case of article (#8607) [WIP][GSOC22] - C - Improve the external changes resolver dialog (#9021) Bump jsoup from 1.15.1 to 1.15.3 (#9103) Bump checkstyle from 10.3.2 to 10.3.3 (#9104) Bump postgresql from 42.4.2 to 42.5.0 (#9105) Bump unirest-java from 3.13.10 to 3.13.11 (#9106) Include check for TimeStamp (#9089) Close OO connection on JabRef exit (#9076) Bump slf4j-tinylog from 2.4.1 to 2.5.0 (#9085) Bump bcprov-jdk18on from 1.71 to 1.71.1 (#9079) Bump tinylog-impl from 2.4.1 to 2.5.0 (#9086) ... # Conflicts: # src/main/java/org/jabref/gui/shared/SharedDatabaseUIManager.java # src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java # src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java

github-actions · 2022-09-02T21:48:21Z

src/main/java/org/jabref/gui/groups/GroupNodeViewModel.java

+        Platform.runLater(()-> {
+           entriesList = databaseContext.getDatabase().getEntries();
+           entriesList.addListener(this::onDatabaseChanged);
+        });


🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.whitespace.EmptyLineSeparatorCheck> _{reported by reviewdog 🐶}
There is more than 1 empty line after this line.

github-actions · 2022-09-02T21:48:22Z

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

-                fireChange(change);
-                latch.countDown();
+                try {
+                    fireChange(change);


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

github-actions · 2022-09-02T21:48:22Z

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

+                try {
+                    fireChange(change);
+
+                }


🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.blocks.RightCurlyCheck> _{reported by reviewdog 🐶}
'}' at column 17 should be on the same line as the next part of a multi-block statement (one that directly contains multiple blocks: if/else-if/else, do/while or try/catch/finally).

github-actions · 2022-09-02T21:48:22Z

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

+
+                }
+                finally
+                {


🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.blocks.LeftCurlyCheck> _{reported by reviewdog 🐶}
'{' at column 17 should be on the previous line.

github-actions · 2022-09-02T21:48:22Z

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

+    /**
+     * 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


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

github-actions · 2022-09-02T21:48:22Z

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

+     * 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
+     */
+    void runInFXThread(Runnable runnable);


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

github-actions · 2022-09-02T21:48:22Z

src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java

                    if (idVersionEntry.getValue() > localEntry.getSharedBibEntryData().getVersion()) {
                        Optional<BibEntry> sharedEntry = dbmsProcessor.getSharedEntry(idVersionEntry.getKey());
                        if (sharedEntry.isPresent()) {
                            // update fields
                            localEntry.setType(sharedEntry.get().getType(), EntriesEventSource.SHARED);
                            localEntry.getSharedBibEntryData()
                                      .setVersion(sharedEntry.get().getSharedBibEntryData().getVersion());
+
+
+                            taskExecutor.runInFXThread(()-> {


🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.whitespace.EmptyLineSeparatorCheck> _{reported by reviewdog 🐶}
There is more than 1 empty line after this line.

github-actions · 2022-09-02T21:48:22Z

src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java

                    if (idVersionEntry.getValue() > localEntry.getSharedBibEntryData().getVersion()) {
                        Optional<BibEntry> sharedEntry = dbmsProcessor.getSharedEntry(idVersionEntry.getKey());
                        if (sharedEntry.isPresent()) {
                            // update fields
                            localEntry.setType(sharedEntry.get().getType(), EntriesEventSource.SHARED);
                            localEntry.getSharedBibEntryData()
                                      .setVersion(sharedEntry.get().getSharedBibEntryData().getVersion());
+
+
+                            taskExecutor.runInFXThread(()-> {


🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.whitespace.EmptyLineSeparatorCheck> _{reported by reviewdog 🐶}
There is more than 1 empty line after this line.

github-actions · 2022-09-02T21:48:22Z

src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java

            if (!remoteEntryMatchingOneLocalEntryFound) {
                entriesToInsertIntoLocalDatabase.add(idVersionEntry.getKey());
            }
        }

        if (!entriesToInsertIntoLocalDatabase.isEmpty()) {
+
+            taskExecutor.runInFXThread( () -> bibDatabase.insertEntries(dbmsProcessor.getSharedEntries(entriesToInsertIntoLocalDatabase), EntriesEventSource.SHARED));


🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.whitespace.ParenPadCheck> _{reported by reviewdog 🐶}
'(' is followed by whitespace.

github-actions · 2022-09-02T22:41:07Z

src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java

            if (!remoteEntryMatchingOneLocalEntryFound) {
                entriesToInsertIntoLocalDatabase.add(idVersionEntry.getKey());
            }
        }

+        // TODO: Update at once? or wrap whole method again in taskExecutor?


🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.whitespace.EmptyLineSeparatorCheck> _{reported by reviewdog 🐶}
There is more than 1 empty line after this line.

github-actions · 2022-09-02T22:41:08Z

src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java

        if (!entriesToInsertIntoLocalDatabase.isEmpty()) {
+
+            taskExecutor.runInFXThread( () -> bibDatabase.insertEntries(dbmsProcessor.partitionAndGetSharedEntries(entriesToInsertIntoLocalDatabase), EntriesEventSource.SHARED)  );


🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.whitespace.ParenPadCheck> _{reported by reviewdog 🐶}
'(' is followed by whitespace.

github-actions · 2022-09-02T22:41:08Z

src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java

        if (!entriesToInsertIntoLocalDatabase.isEmpty()) {
+
+            taskExecutor.runInFXThread( () -> bibDatabase.insertEntries(dbmsProcessor.partitionAndGetSharedEntries(entriesToInsertIntoLocalDatabase), EntriesEventSource.SHARED)  );


🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.whitespace.ParenPadCheck> _{reported by reviewdog 🐶}
')' is preceded with whitespace.

github-actions · 2022-09-02T22:41:08Z

src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java

        if (!entriesToInsertIntoLocalDatabase.isEmpty()) {
+
+            taskExecutor.runInFXThread( () -> bibDatabase.insertEntries(dbmsProcessor.partitionAndGetSharedEntries(entriesToInsertIntoLocalDatabase), EntriesEventSource.SHARED)  );


🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.whitespace.SingleSpaceSeparatorCheck> _{reported by reviewdog 🐶}
Use a single space to separate non-whitespace characters.

* upstream/main: Fix parsing of JabRef v5.7 study.yml files (#9124) Fix integrity check for tilde accents in author names (#9097) Rework the Define study parameters dialog (#9123) Fix parsing of save actions (#9122) Fix exception that occurs when saving name formatters (#9121) Refine code for BibEntry#replaceDownloadedFile (#9118)

* upstream/main: fix l10n Compile fix # Conflicts: # src/main/java/org/jabref/gui/slr/StudyDatabaseItem.java

fuuu

github-actions · 2022-09-03T16:47:57Z

src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java

-                    remoteEntryMatchingOneLocalEntryFound = true;
-                    if (idVersionEntry.getValue() > localEntry.getSharedBibEntryData().getVersion()) {
-                        Optional<BibEntry> sharedEntry = dbmsProcessor.getSharedEntry(idVersionEntry.getKey());
+                remoteEntryMatchingOneLocalEntryFound = true;


🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.whitespace.EmptyLineSeparatorCheck> _{reported by reviewdog 🐶}
There is more than 1 empty line after this line.

github-actions · 2022-09-03T16:47:57Z

src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java

+                remoteEntryMatchingOneLocalEntryFound = true;
+
+
+                if (idVersionEntry.getValue() > localEntry.getSharedBibEntryData().getVersion()) {


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

github-actions · 2022-09-03T16:47:57Z

src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java

+            BackgroundTask.wrap(() -> dbmsProcessor.partitionAndGetSharedEntries(entriesToInsertIntoLocalDatabase))
+                          .onSuccess(entries-> {
+                // in case entries should be added into the local database, insert them
+                bibDatabase.insertEntries(entries, EntriesEventSource.SHARED);


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

github-actions · 2022-09-03T16:47:57Z

src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java

+                // in case entries should be added into the local database, insert them
+                bibDatabase.insertEntries(entries, EntriesEventSource.SHARED);
+
+            }).executeWith(taskExecutor);


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

Siedlerchr and others added 7 commits February 7, 2022 19:46

improve db sync

2b78ac7

Handling of field change events as background task

25888f4

fix architecture violation

90e0969

Merge branch 'Kai-Franke-DLR-dbsync' into dbsync

fbaf907

* Kai-Franke-DLR-dbsync: fix architecture violation Handling of field change events as background task

try to fix tests

e608b1f

checkstyle

a2dd44e

use default task executor

04842c0

Siedlerchr changed the title ~~Dbsync~~ Dbsync performance improvements Feb 9, 2022

Siedlerchr added 7 commits February 9, 2022 20:42

fix missing task executor

30a12f9

fix some threding errors

4251dc0

implement dummy fx thread

6fc9ecb

use current thread task

51aa6a6

fix checkstyle

cf209ee

add javadoc

remove useless fx code

a35b4b0

revert actions changes

convert default task executor to execute dummy fx thread

033a57e

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 9, 2022

make checkstyle and olly happy

e7b8219

tobiasdiez reviewed Feb 11, 2022

View reviewed changes

Siedlerchr added 4 commits February 11, 2022 20:29

Fix UI Thread blocking

e73f8bc

Fix UI Thraddlist blocking

d8ba668

try to improve threading

c1f74a5

Siedlerchr removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 18, 2022

experiment with threading

4b936a3

Siedlerchr closed this Feb 26, 2022

ThiloteE mentioned this pull request Mar 15, 2022

GUI unresponsive when changing fields and being connected to shared database #8485

Open

2 tasks

Siedlerchr mentioned this pull request Mar 21, 2022

Shared database: Entries are additionally assigned to groups of another user logged in #8585

Closed

2 tasks

Siedlerchr reopened this Sep 2, 2022

Siedlerchr changed the title ~~Dbsync performance improvements~~ WIP: Dbsync performance improvements Sep 2, 2022

github-actions bot reviewed Sep 2, 2022

View reviewed changes

test further

d0e7237

github-actions bot reviewed Sep 2, 2022

View reviewed changes

Siedlerchr added 4 commits September 3, 2022 13:15

fix item

391a274

Merge remote-tracking branch 'upstream/main' into dbsync

311f81b

* upstream/main: fix l10n Compile fix # Conflicts: # src/main/java/org/jabref/gui/slr/StudyDatabaseItem.java

argh

8010a47

fuuu

github-actions bot reviewed Sep 3, 2022

View reviewed changes

ThiloteE added shared-database type: performance labels Sep 13, 2022

Siedlerchr mentioned this pull request Apr 24, 2023

shared SQL database - jabref updates one char and crashes after #9787

Open

2 tasks

ThiloteE added the status: changes required Pull requests that are not yet complete label May 1, 2023

koppor force-pushed the main branch from 0594cb2 to c444c73 Compare October 10, 2023 21:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Dbsync performance improvements #8496

WIP: Dbsync performance improvements #8496

Siedlerchr commented Feb 9, 2022 •

edited

tobiasdiez Feb 11, 2022

Siedlerchr Feb 11, 2022

tobiasdiez Feb 11, 2022

Siedlerchr Feb 11, 2022

tobiasdiez Feb 11, 2022

tobiasdiez Feb 13, 2022

Siedlerchr Feb 13, 2022

Siedlerchr Feb 13, 2022

tobiasdiez Feb 13, 2022

Siedlerchr Feb 14, 2022

Siedlerchr commented Feb 26, 2022

github-actions bot Sep 2, 2022

github-actions bot Sep 2, 2022

github-actions bot Sep 2, 2022

github-actions bot Sep 2, 2022

github-actions bot Sep 2, 2022

github-actions bot Sep 2, 2022

github-actions bot Sep 2, 2022

github-actions bot Sep 2, 2022

github-actions bot Sep 2, 2022

github-actions bot Sep 2, 2022

github-actions bot Sep 2, 2022

github-actions bot Sep 2, 2022

github-actions bot Sep 2, 2022

github-actions bot Sep 3, 2022

github-actions bot Sep 3, 2022

github-actions bot Sep 3, 2022

github-actions bot Sep 3, 2022

	/**
	* 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);
	}

		if (!entriesToInsertIntoLocalDatabase.isEmpty()) {

		taskExecutor.runInFXThread( () -> bibDatabase.insertEntries(dbmsProcessor.partitionAndGetSharedEntries(entriesToInsertIntoLocalDatabase), EntriesEventSource.SHARED) );

		remoteEntryMatchingOneLocalEntryFound = true;


		if (idVersionEntry.getValue() > localEntry.getSharedBibEntryData().getVersion()) {

WIP: Dbsync performance improvements #8496

Are you sure you want to change the base?

WIP: Dbsync performance improvements #8496

Conversation

Siedlerchr commented Feb 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr commented Feb 26, 2022

github-actions bot Sep 2, 2022

Choose a reason for hiding this comment

github-actions bot Sep 2, 2022

Choose a reason for hiding this comment

github-actions bot Sep 2, 2022

Choose a reason for hiding this comment

github-actions bot Sep 2, 2022

Choose a reason for hiding this comment

github-actions bot Sep 2, 2022

Choose a reason for hiding this comment

github-actions bot Sep 2, 2022

Choose a reason for hiding this comment

github-actions bot Sep 2, 2022

Choose a reason for hiding this comment

github-actions bot Sep 2, 2022

Choose a reason for hiding this comment

github-actions bot Sep 2, 2022

Choose a reason for hiding this comment

github-actions bot Sep 2, 2022

Choose a reason for hiding this comment

github-actions bot Sep 2, 2022

Choose a reason for hiding this comment

github-actions bot Sep 2, 2022

Choose a reason for hiding this comment

github-actions bot Sep 2, 2022

Choose a reason for hiding this comment

github-actions bot Sep 3, 2022

Choose a reason for hiding this comment

github-actions bot Sep 3, 2022

Choose a reason for hiding this comment

github-actions bot Sep 3, 2022

Choose a reason for hiding this comment

github-actions bot Sep 3, 2022

Choose a reason for hiding this comment

Siedlerchr commented Feb 9, 2022 •

edited