Open Kai-Franke-DLR opened 2 years ago
Thanks for the report and your detailed analysis. This is very helpful.
pullWithLastEntry and synchronizeSharedEntry seem to sync the same entry twice, so one modification I did in pullWithLastEntry was to check if the last changed entry is equal to the parameter provided by pullWithLastEntry. This seems like a good first step. Can you provide a PR with the changes or a patch?
I think the bottleneck is probably the synchronizeLocalDatabase
which could be executed in a background thread. Can you please test with this commit as well? I added some simple background threading
https://github.com/JabRef/jabref/commit/2b78ac7cb20713885c9ab3b9ce4cee0790b8cd22
Regarding threading: This is difficult, however, I guess some things can be run using a BackgroundThread. However, there are certain actions that need to be run on the GUI/JavaFX Thread, things like inserting/updating/setting Fields in the bib database need to be on the fx thread, mainly because they are triggering changes for the GUI-
Many thanks for your fast reply!
I think the bottleneck is probably the synchronizeLocalDatabase which could be executed in a background thread. Can you please test with this commit as well? I added some simple background threading https://github.com/JabRef/jabref/commit/2b78ac7cb20713885c9ab3b9ce4cee0790b8cd22
I tried your commit, but unfortunatly the issue still persists. This may be because the performance issue arises mostly in synchronizeSharedEntry and not in synchronizeLocalDatabase, which does not yet execute DBMSProcessor.updateEntry in a BackgroundTask. Nevertheless, this commit is very useful, because I could maybe wrap DBMSProcessor.updateEntry exactly the same way as already done with synchronizeLocalDatabase in the DBMSSynchronizer.initializeDatabases method and handle GUI reactions (eg. popup merge dialog) via DefaultTaskExecutor.runInJavaFXThread as done within DBMSSynchronizer.synchronizeLocalDatabase. I will try to implement this and come back to you shortly.
The changes within #8494 fix the delay caused by database syncs. The only thing I found to be an issue with the JavaFX thread was the UpdateRefusedEvent posted by DBMSSynchronizer.synchronizeSharedEntry when entries should be merged (creates a popup dialog). This event is therefore posted within the JavaFX thread.
Are there any other GUI reactions in correlation to the listeners reaction I should be aware of? The build works correctly as far as I'm aware.
thanks a lot for your fixes! This is really great! I will test this later as well I guess you also need to wrap this in the FXTask (see my commit): in synchronizeLocalDatabase
bibDatabase.insertEntries(dbmsProcessor.getSharedEntries(entriesToInsertIntoLocalDatabase),
EntriesEventSource.SHARED);
});
I noticed a bug in your code, the code wrapped in the BackgroundTask was never executed. I tested a bit around and also added some synchronized to prevent concurrent thread modificaitons
@Kai-Franke-DLR: Thank you for this great hotfix (manual save button)! 👍
With the official JabRef release, we encountered the same slow response of the GUI making it almost impossible to use for daily work (OS Windows 10, PostgreSQL server, database used by multiple users). Therefore, I would be happy to see an improvement.
~~Both pull requests (#8494 and #8496) that seem to have fixed this issue were merged after Jabref 5.5 (the latest official stable version) was published. @saralajew, I take it, you have not tried the latest development version yet? If so, you might want to try it first: https://builds.jabref.org/main/ Only, if you still can reproduce significant slowness with the development version, i guess we can reopen this issue here.~~
Edit: The above mentioned info was faulty. I apologize.
@ThiloteE #8496 was NOT merged (it was closed because I ran into deadlocks on several other ocassions)
Uff. You are right. I see it now. Sorry for providing wrong info.
There were substantial changes and fixes to auto-save (6a7139566b555903daafa2913d10431a5f02d704) and the backup-manager (f7648e82168e180f635b0e783262117d75c87fa5). Not sure though, if performance got better, as autosave still runs in the GUI thread.
@ThiloteE I tested the version 5.7 with a shared database. The performance improved a lot! It is still a bit unresponsive but at a level that is acceptable. IMO you can close this issue.
@ThiloteE I tested the version 5.7 with a shared database. The performance improved a lot! It is still a bit unresponsive but at a level that is acceptable. IMO you can close this issue.
Do you still observe this? I just started using a shared database (on a server, not local) with jabref 5.9, and typing in fields comes with a 1-3 second delay.
JabRef version
Latest development branch build (please note build date below)
Operating system
Windows
Details on version and operating system
Windows 10 Pro Build 19041.1415
Checked with the latest development build
Steps to reproduce the behaviour
The behavior has been tested with the newest release as well as the current development version:
Appendix
I forked the current main branch for testing purposes:
https://github.com/Kai-Franke-DLR/jabref
. The modifications I made can be found in this repository. The tested shared database (PostgresSQL) is the following:I tracked down the cause of the unresponsiveness to the DBMSSynchronizer.listen(FieldChangedEvent event) method. The operations within this method take the following times to complete:
2022-02-07 08:21:31 [JavaFX Application Thread] org.jabref.logic.shared.DBMSSynchronizer.listen() INFO: Time (ms) for synchronizeLocalMetaData: 59 2022-02-07 08:21:31 [JavaFX Application Thread] org.jabref.logic.shared.DBMSSynchronizer.listen() INFO: Time (ms) for pullWithLastEntry: 746 2022-02-07 08:21:31 [JavaFX Application Thread] org.jabref.logic.shared.DBMSSynchronizer.listen() INFO: Time (ms) for synchronizeSharedEntry: 728 2022-02-07 08:21:31 [JavaFX Application Thread] org.jabref.logic.shared.DBMSSynchronizer.listen() INFO: Time (ms) for synchronizeLocalDatabase: 128
pullWithLastEntry and synchronizeSharedEntry seem to sync the same entry twice, so one modification I did in pullWithLastEntry was to check if the last changed entry is equal to the parameter provided by pullWithLastEntry.
Since the entry editor was still too unresponsive to be used correctly, I implemented a manual save button for modified entries and the ability to disable autosave to shared libraries as a quick&dirty "fix".
This seems to work for single entry field changes, but not, if you want to modify multiple entries in batch (eg. generate citation keys). In addition, the autosave feature is needed by our company.
Are there any plans on handling updates by the DBMSProcessor in a seperate thread to not block the GUI or to speed up the sync process? Can I support you somewhere?