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.

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

Performance improvements for GUI start up #5423

Merged
merged 8 commits into from
Oct 13, 2019
Merged

Performance improvements for GUI start up #5423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
08c5141
Delegate auto completion initialization to background thread
Brainsucker92 Oct 10, 2019
7471e17
Load bib files concurrently on start up
Brainsucker92 Oct 11, 2019
c0ccee9
Delegate load of bib files to OpenDatabaseAction class
Brainsucker92 Oct 11, 2019
bc8bce6
Formatting correction
Brainsucker92 Oct 12, 2019
3f2dd7d
Remove registering listener from parallel execution
Brainsucker92 Oct 12, 2019
90e4a92
Mark ExecutorService field as final
Brainsucker92 Oct 12, 2019
2edec2a
Code rearrange
Brainsucker92 Oct 12, 2019
588dca5
Move execution of database loading from JavaFX thread to executor thread
Brainsucker92 Oct 13, 2019
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
5 changes: 3 additions & 2 deletions src/main/java/org/jabref/JabRefGUI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import java.util.Iterator;
import java.util.List;

import javafx.application.Platform;
import javafx.scene.Scene;
import javafx.stage.Stage;

Expand Down Expand Up @@ -42,10 +41,12 @@ public class JabRefGUI {
private final boolean isBlank;
private final List<ParserResult> failed = new ArrayList<>();
private final List<ParserResult> toOpenTab = new ArrayList<>();
private final JabRefExecutorService executorService;

public JabRefGUI(Stage mainStage, List<ParserResult> databases, boolean isBlank) {
this.bibDatabases = databases;
this.isBlank = isBlank;
executorService = JabRefExecutorService.INSTANCE;
mainFrame = new JabRefFrame(mainStage);

openWindow(mainStage);
Expand Down Expand Up @@ -91,7 +92,7 @@ private void openWindow(Stage mainStage) {
}
});

Platform.runLater(this::openDatabases);
executorService.execute(this::openDatabases);
Copy link
Member

Choose a reason for hiding this comment

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

Did you tried if this works (i.e. if JabRef still starts and loads the databases)? That would be cool, but I fear that there might be an exception thrown as things are not initialized correctly on the javafx thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, both lines work pretty similar, which is to open the databases at some point in the future. The only difference is, that it's no longer executed by the JavaFX thread. That prevents any GUI freeze on startup. In my opinion the GUI has to be able to start up completely empty and get the data fed in as it's loaded. So in case that you are right and the program does not start correctly anymore due to this change, then we got a clear indicator for some bad code smell somewhere else in the project. Anyway - I'll test it to see how the program behaves. Just to make sure what's going on exactly.

Copy link
Member

Choose a reason for hiding this comment

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

You are of course completely right. It should work in principle. However, the GUI code is smelly at some points and the database handling is some of the extra smelly parts. This is why I'm a bit afraid it doesn't work. If it does, perfect and we can merge! Otherwise I would leave the current solution for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I finally managed to test it and it seems working to me. I imported several (smaller) databases and restarted the program a few times. Nothing too concerning happend. Therefore I think this is good to go.

Copy link
Member

Choose a reason for hiding this comment

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

"Nothing too concerning happend. “

Sounds convincing 😂

Copy link
Member

@tobiasdiez tobiasdiez Oct 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

If @matthiasgeiger is convinced, then I'm too! As already a few devs had a look at this PR, I'll merge.

}

private void openDatabases() {
Expand Down
19 changes: 11 additions & 8 deletions src/main/java/org/jabref/gui/BasePanel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,10 +143,12 @@ public class BasePanel extends StackPane {
// the query the user searches when this BasePanel is active
private Optional<SearchQuery> currentSearchQuery = Optional.empty();
private Optional<DatabaseChangeMonitor> changeMonitor = Optional.empty();
private JabRefExecutorService executorService;

public BasePanel(JabRefFrame frame, BasePanelPreferences preferences, BibDatabaseContext bibDatabaseContext, ExternalFileTypes externalFileTypes) {
this.preferences = Objects.requireNonNull(preferences);
this.frame = Objects.requireNonNull(frame);
this.executorService = JabRefExecutorService.INSTANCE;
this.bibDatabaseContext = Objects.requireNonNull(bibDatabaseContext);
this.externalFileTypes = Objects.requireNonNull(externalFileTypes);
this.undoManager = frame.getUndoManager();
Expand Down Expand Up @@ -674,7 +676,7 @@ private void createMainTable() {
String clearSearch = "clearSearch";
mainTable.getInputMap().put(Globals.getKeyPrefs().getKey(KeyBinding.CLEAR_SEARCH), clearSearch);
mainTable.getActionMap().put(clearSearch, new AbstractAction() {

@Override
public void actionPerformed(ActionEvent e) {
// need to close these here, b/c this action overshadows the responsible actions when the main table is selected
Expand All @@ -695,9 +697,9 @@ public void actionPerformed(ActionEvent e) {
}
}
});

mainTable.getActionMap().put(Actions.CUT, new AbstractAction() {

@Override
public void actionPerformed(ActionEvent e) {
try {
Expand All @@ -708,7 +710,7 @@ public void actionPerformed(ActionEvent e) {
}
});
mainTable.getActionMap().put(Actions.COPY, new AbstractAction() {

Copy link
Member

Choose a reason for hiding this comment

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

Please double check your IDE config. We recommend IntelliJ. See https://github.com/jabref/jabref/wiki/Guidelines-for-setting-up-a-local-workspace for detailed steps. Please let @Siedlerchr know if you use Eclipse.

Reason: We do not want to have code changes only because of a style.

Copy link
Contributor Author

@Brainsucker92 Brainsucker92 Oct 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I am using IntelliJ IDE and followed the IntelliJ IDEA Code Style Configuration (https://github.com/JabRef/jabref/tree/master/config) before posting any commits to this project.

config

Therefore the malformed code style has been in the file before I made any changes and has been corrected by the IDE automatically.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. Did you format all code? - At the diff of all the files (button on GitHub) there were spaces introduced to an empty line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I always format the whole file after any changes (for example to remove unused imports). But actually I noticed something odd going on here. Eventhough I had the JabRef code style configured, it was still overridden by the default editor config (for some reason). This should be fixed by now and we got proper formatting as desired.

@Override
public void actionPerformed(ActionEvent e) {
try {
Expand All @@ -719,7 +721,7 @@ public void actionPerformed(ActionEvent e) {
}
});
mainTable.getActionMap().put(Actions.PASTE, new AbstractAction() {

@Override
public void actionPerformed(ActionEvent e) {
try {
Expand Down Expand Up @@ -747,11 +749,12 @@ public void setupMainPanel() {
splitPane.getItems().add(pane);

// Set up name autocompleter for search:
instantiateSearchAutoCompleter();
executorService.execute(() -> {
Copy link
Member

Choose a reason for hiding this comment

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

I would leave the registerListener call on the current thread (just because I'm not sure how listeners interact with multithreading in detail - and this is no expensive operation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

instantiateSearchAutoCompleter();
setupAutoCompletion();
});
this.getDatabase().registerListener(new SearchAutoCompleteListener());

setupAutoCompletion();

// Saves the divider position as soon as it changes
// We need to keep a reference to the subscription, otherwise the binding gets garbage collected
dividerPositionSubscription = EasyBind.monadic(Bindings.valueAt(splitPane.getDividers(), 0))
Expand Down
81 changes: 37 additions & 44 deletions src/main/java/org/jabref/gui/JabRefFrame.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@
import org.jabref.logic.autosaveandbackup.AutosaveManager;
import org.jabref.logic.autosaveandbackup.BackupManager;
import org.jabref.logic.importer.IdFetcher;
import org.jabref.logic.importer.OpenDatabase;
import org.jabref.logic.importer.ParserResult;
import org.jabref.logic.importer.WebFetchers;
import org.jabref.logic.l10n.Localization;
Expand Down Expand Up @@ -147,10 +146,11 @@ public class JabRefFrame extends BorderPane {
private final Stage mainStage;
private final StateManager stateManager;
private final CountingUndoManager undoManager;
private SidePaneManager sidePaneManager;
private TabPane tabbedPane;
private final PushToApplicationsManager pushToApplicationsManager;
private final DialogService dialogService;
private final JabRefExecutorService executorService;
private SidePaneManager sidePaneManager;
private TabPane tabbedPane;
private SidePane sidePane;

public JabRefFrame(Stage mainStage) {
Expand All @@ -160,13 +160,14 @@ public JabRefFrame(Stage mainStage) {
this.pushToApplicationsManager = new PushToApplicationsManager(dialogService, stateManager);
this.undoManager = Globals.undoManager;
this.fileHistory = new FileHistoryMenu(prefs, dialogService, getOpenDatabaseAction());
this.executorService = JabRefExecutorService.INSTANCE;
}

private static BasePanel getBasePanel(Tab tab) {
return (BasePanel) tab.getContent();
}

public void initDragAndDrop() {
private void initDragAndDrop() {
Tab dndIndicator = new Tab(Localization.lang("Open files..."), null);
dndIndicator.getStyleClass().add("drop");

Expand Down Expand Up @@ -200,18 +201,10 @@ public void initDragAndDrop() {
tabHeaderArea.setOnDragDropped(event -> {
tabbedPane.getTabs().remove(dndIndicator);

boolean success = false;

List<Path> bibFiles = DragAndDropHelper.getBibFiles(event.getDragboard());
if (!bibFiles.isEmpty()) {
for (Path file : bibFiles) {
ParserResult pr = OpenDatabase.loadDatabase(file.toString(), Globals.prefs.getImportFormatPreferences(), Globals.getFileUpdateMonitor());
addParserResult(pr, true);
}
success = true;
}

event.setDropCompleted(success);
OpenDatabaseAction openDatabaseAction = this.getOpenDatabaseAction();
openDatabaseAction.openFiles(bibFiles, true);
event.setDropCompleted(true);
event.consume();
});
});
Expand Down Expand Up @@ -261,7 +254,7 @@ private void initShowTrackingNotification() {

@Override
public void run() {
DefaultTaskExecutor.runInJavaFXThread(JabRefFrame.this::showTrackingNotification);
DefaultTaskExecutor.runInJavaFXThread(JabRefFrame.this::showTrackingNotification);
}
}, 60000); // run in one minute
}
Expand Down Expand Up @@ -1205,6 +1198,34 @@ public DialogService getDialogService() {
return dialogService;
}

private void setDefaultTableFontSize() {
GUIGlobals.setFont(Globals.prefs.getIntDefault(JabRefPreferences.FONT_SIZE));
for (BasePanel basePanel : getBasePanelList()) {
basePanel.updateTableFont();
}
dialogService.notify(Localization.lang("Table font size is %0", String.valueOf(GUIGlobals.currentFont.getSize())));
}

private void increaseTableFontSize() {
GUIGlobals.setFont(GUIGlobals.currentFont.getSize() + 1);
for (BasePanel basePanel : getBasePanelList()) {
basePanel.updateTableFont();
}
dialogService.notify(Localization.lang("Table font size is %0", String.valueOf(GUIGlobals.currentFont.getSize())));
}

private void decreaseTableFontSize() {
double currentSize = GUIGlobals.currentFont.getSize();
if (currentSize < 2) {
return;
}
GUIGlobals.setFont(currentSize - 1);
for (BasePanel basePanel : getBasePanelList()) {
basePanel.updateTableFont();
}
dialogService.notify(Localization.lang("Table font size is %0", String.valueOf(GUIGlobals.currentFont.getSize())));
}

/**
* The action concerned with closing the window.
*/
Expand Down Expand Up @@ -1273,34 +1294,6 @@ public void execute() {
}
}

private void setDefaultTableFontSize() {
GUIGlobals.setFont(Globals.prefs.getIntDefault(JabRefPreferences.FONT_SIZE));
for (BasePanel basePanel : getBasePanelList()) {
basePanel.updateTableFont();
}
dialogService.notify(Localization.lang("Table font size is %0", String.valueOf(GUIGlobals.currentFont.getSize())));
}

private void increaseTableFontSize() {
GUIGlobals.setFont(GUIGlobals.currentFont.getSize() + 1);
for (BasePanel basePanel : getBasePanelList()) {
basePanel.updateTableFont();
}
dialogService.notify(Localization.lang("Table font size is %0", String.valueOf(GUIGlobals.currentFont.getSize())));
}

private void decreaseTableFontSize() {
double currentSize = GUIGlobals.currentFont.getSize();
if (currentSize < 2) {
return;
}
GUIGlobals.setFont(currentSize - 1);
for (BasePanel basePanel : getBasePanelList()) {
basePanel.updateTableFont();
}
dialogService.notify(Localization.lang("Table font size is %0", String.valueOf(GUIGlobals.currentFont.getSize())));
}

private class CloseDatabaseAction extends SimpleCommand {

@Override
Expand Down