-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Avoid recreation of the EntryEditor #3187
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me but I havn't tried if the entry editor still works 😄.
The biggest improvement probably comes from the fact that the JavaFX scene
is reused (creating a new scene is very costly and time consuming). Good job!
typeLabel = new TypeLabel(""); | ||
setEntry(entry); | ||
setupToolBar(); | ||
container.setScene(new Scene(tabbed)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure but I think setScene
should be called in the JavaFX thread (DefaultTaskExecutor.runInJavaFXThread
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
public EntryEditor(JabRefFrame frame, BasePanel panel, BibEntry entry, String lastTabName) { | ||
this.frame = frame; | ||
public EntryEditor(BasePanel panel, BibEntry entry) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove the entry
from the constructor to make it clearer that the entry editor can be reused and is not specific to the entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
DefaultTaskExecutor.runInJavaFXThread(() -> { | ||
addTabs(lastTabName); | ||
addTabs(this.getVisibleTabName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is getVisibleTabName
used somewhere outside of this class? if not then it can be converted to just a field and don't need to be exposed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used in the BasePanel
and the main table, so it needs to be visible.
If I have "Other fields" and switch to an entry without "Other fields", the FIRST tab "Required field" is focused, not "Deprecated Fields", which is the tab BEFORE the tab "Other fields". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have looked into this and would say that we should merge this, especially after the discussion in #3314.
@lenhard would you mind going over the comments @tobiasdiez made in his review?
I used YourKit tonight to profile the current master and looked over this PR. I tested it by using the infinite scroll with the entry editor open so that JabRef was pushed to its limits. Be aware that the things below might be wrong but I hope they give some helpful insight: Localization memoryFirst, I was stunned that the objects that are purely connected to getting the translation strings are so large. In the current master branch, they accounted for 6% of the memory of the complete thread. I marked the line: I looked into the implementation and saw that the list of keys is always recreated. I was wondering why that is. When I change the language in the preferences, I get notified, that I have to restart JabRef. So why don't we hold one
|
@halirutan First of all: Wow, just wow. It is a very rare sight to see a new contributor put in as much effort and diligence as you do. You have my kudos. Your work is excellent. Regarding the localizationYour suggestion is a valid improvement which I fully support. It's not directly linked to the topic of this PR, though. So, could you turn this into a separate pull request? I hope that it does not take too much effort to turn this into a fully correct implementation, but from glancing at the code I don't think it would. Regarding the entry editorThe lazy initialization has my upvote as well. I wonder why this hasn't been changed to support lazy initialization before, and my guess is that it is just an oversight by several people including myself. What you list under "going one step further" is also what I had in mind. The "problem" that this faces is that the content of a tab varies depending on what kind of entry is displayed. For example, an How to proceed from hereThe performance improvements you show are just awesome. We should get all this into master and I suggest the following:
When we have integrated all this, we can have a go at a proper one-time creation of the tabs. If you are interested in trying this, be my guest. Once again, thank you a lot! |
I have double checked #2064 and #2075 manually and the optional fields are recreated as desired. @koppor I don't really see why the focus should jump to adjacent tab when switching to an entry that does not have optional fields (also why to the one before and not the one after). So unless I have a good reason to jump to a particular tab, I am against implementing special handling for that. From my point of view, this PR is ready to merge and then we can go for the further improvements discussed above. |
Okay, then I'll be brave and merge this now. |
* upstream/master: Update gradle to 4.2.1 (#3322) Avoid recreation of the EntryEditor (#3187) Fix #3133 telemetry by locking azure to 1.0.9 German translation for missing properties (#3312) Improvement for Java FX font rendering on Linux (#3305) Add also conversion for em dash Add \textendash to the html conversion table Update latex2tunicode from 0.2.1 -> 0.2.2 Added note about updating controlsfx Fix exception/freezing on EntryChangedEvent in Entry Editor (#3299) Update libs (#3300) update guava from 23.0 -> 23.2 update mvvmfx-validation from 1.6.0 -> 1.7.0 Resolves #3255 file open dialog should have "supported formates" filetype Fix #3235: remote metadata is updated instead of delete + insert (#3282) Change OO paths to Libre Office in preferences (#3287) Fix #2471: remove line breaks from abstracts in ADS fetcher (#3285) Resolves #3280 Empty String instead of N/A (#3288)
This is one baby step towards fixing the performance and memory issues we are having with the entry editor, e.g. #3175 #3113
Previously, we've created a new EntryEditor instance every time a new entry was selected. This PR avoids that. Instead of recreating a new entry editor, the existing one is reused and just all its contents, i.e. the tabs, are recreated. This doesn't help significantly and we still have the peaks when cycling through the main table with key pressed, but it's a step in the right direction. Here's an overview
The peaks are from cycling through the main table with down key pressed, but as we can see garbage collection still reduces the used heap massively.
The next step (in a new PR) would be to avoid a recreation of the tabs and to implement a proper lazy initialization of them, so that they only consume memory and processing power when they are actually visible.
I've tested it manually and hope that this doesn't add hidden bugs. After all, the tabs are still created anew so there shouldn't be update problems. Nevertheless, this should receive some manual testing from someone else before it is merged. Lastly, since the tabs are created anew there's now a visible effect when selecting a new entry: The new tabs sort of "fly in". I have no problems with that, but if others find that annoying then this PR should not yet be merged before for 4.0 or alternatively only when the tabs are not recreated.
gradle localizationUpdate
?