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.

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

Initializing EntryEditor Tabs on focus #3331

Merged
merged 2 commits into from
Oct 21, 2017

Conversation

halirutan
Copy link
Collaborator

This is a very small fix with big implications for performance and memory. The reasoning is simple: Currently, all editor tabs (extending from FieldsEditorTab) are initialized when a new entry is selected. So even if the user is only browsing through the entry table with the editor open, on each new BibEntry we create all the tabs for each entry from scratch. This can in the case of FieldsEditorTab simply prevented by moving this initialization to initialize which is called when the tab gets focus. This PR addresses this issue.

I want to give a detailed analysis because there is much more potential when we slightly rework how the EntryEditor and its tabs work. First of all, this is the memory increase when I scroll the 209 entry example jabref-authors.bib with the current master branch.

img

In the middle I start after running the GC with about 140MB and after scrolling the list, I ended up with 1.9GB. 99% of this comes directly from the EntryEditor. After the patch I propose, the memory situation is vastly improved

img

We need less than 400MB at the peaks and the GC seems to be able to do a much better job. It's not only an improvement in memory, but the performance increases dramatically.

Where to go from here?

These things should be fixed in a next step:

  1. org.jabref.gui.entryeditor.EntryEditor#addTabs is the major hotspot now because it recreates all tabs on a new entry.
  2. ensure that all tabs get this late initialization on focus.
  3. tabs should not be recreated at all, they should be kept as objects in EntryEditor and only get notified when a new entry is selected.

Unfortunately, I don't see that I can do this on my own without breaking some of the various information flows on updates in the tabs (esp. the source tab). But if someone started to work on this I would join. These are the things I would change first:

  1. almost all tabs get frame, panel, entryType, this, entry on construction. This needs to go away. EntryEditor is an instance of EntryContainer and tabs can get the current entry. We need similar things for the other parameters. In essences, it should be new EntryEditorTab(entryEditorInstance).
  2. On initialize the tab should check if it is initialized and if the entry has changed
  3. The entryEditor has a list of all tabs that are created on construction and it only notifies them on entry change.

The "sliding tab problem"

Especially with this faster version, the problem of the tabs sliding in from the left on recreation becomes more visible. I have no simple solution to this especially since I'm not sure how many tabs there theoretically can be. It's these line in addTabs

        // General fields from preferences
        EntryEditorTabList tabList = Globals.prefs.getEntryEditorTabList();
        for (int i = 0; i < tabList.getTabCount(); i++) {
            FieldsEditorTab newFieldsEditorTab = new FieldsEditorTab(frame, panel, tabList.getTabFields(i), this, false,
                    false, entry);
            newFieldsEditorTab.setText(tabList.getTabName(i));
            tabs.add(newFieldsEditorTab);
        }

If we have a reasonable number of tabs, the best and clearest solution would be to just disable the ones that should not be shown. This will give a fixed EntryEditor layout that works nice, but I'm not sure it would piss user of if they have to endure disabled tabs.

The second solution would be to maintain a list of all tabs and one with the currently visible tabs (tabbed.getTabs()). This will again lead to a wiggling tabs header, but not as bad as before. On entry change, we just modify tabbed to display correct tabs.

The third solution would be to simply close tabs programmatically but that interferes with the current TabClosingPolicy.UNAVAILABLE

…ble (end from FieldsEditor) will cost no memory or CPU
@lenhard
Copy link
Member

lenhard commented Oct 21, 2017

As discussed in the prior PR, I am very much in favor of this. To me, this change alone and the implications it has on performance would justify the release of a 4.1. I have tested it locally and the reduced memory consumption is awesome. Could you add a changelog entry regarding lazy initialization in the Changed section?

The next step would be, as you say, to avoid the recreation of the tabs. I would like to work on this, but my schedule for the weeks to come is a bit crazy, so I cannot promise anything.

Regarding the sliding tab problem, we cannot really fix the number of tabs without annoying users. It would be a reduction in functionality and I would like to avoid that. Maintaining a list of tabs and selectively showing the ones that need to be shown seems like the most reasonable solution to me at the moment, but we will see when we proceed with the implementation.

@Siedlerchr
Copy link
Member

Looks good to me, just take a look at the codacy issue, is probably just about the region variable

@halirutan
Copy link
Collaborator Author

@lenhard I'm making a note in the changelog.

I would like to work on this, but my schedule for the weeks to come is a bit crazy,

No problem, I have a lot of different things I can/should/have to work on as well. You seemed to have written a great deal of the code there and that's why it would be nice if you would help with the basic restructuring so that I don't overlook some dependency on the global preferences or on other things and have to start over. Once we have the basic structure, I'm sure I can help.

@Siedlerchr codacy is just complaining

FieldsEditorTab.java is often changed in this project

Well, I hope it's going to complain a lot in the future.

@lenhard
Copy link
Member

lenhard commented Oct 21, 2017

Perfect, I'll merge now.

Actually, most of the code in the current entry editor was written by @tobiasdiez (who might also have some valuable hints on this). I just contributed with a number of bug fixes over the past months. Anyway, it would be cool to get all this done for 4.1. We'll see.

@lenhard lenhard merged commit f71c5a8 into JabRef:master Oct 21, 2017
Siedlerchr added a commit that referenced this pull request Oct 22, 2017
* upstream/master:
  Initializing EntryEditor Tabs on focus (#3331)
  Fix #3292: annotations are now automatically refreshed (#3325)
  Change integrity message for names depending on database mode (#3330)
  Fix location bundle with fast access (#3327)
  Small code cleanup
  Fix "path not found" exception
  Small fix in drag and drop handler of linked files (#3328)

# Conflicts:
#	src/main/java/org/jabref/gui/DefaultInjector.java
@halirutan halirutan deleted the fixPS-EntryEditorPerformance branch October 23, 2017 15:01
Siedlerchr added a commit that referenced this pull request Oct 25, 2017
* upstream/master:
  Initializing EntryEditor Tabs on focus (#3331)
  Fix #3292: annotations are now automatically refreshed (#3325)

# Conflicts:
#	CHANGELOG.md
Siedlerchr added a commit that referenced this pull request Oct 27, 2017
* upstream/master: (31 commits)
  Source tab entry duplication (#3360)
  Use CITE_COMMENT not only for external latex editors but also for cop… (#3351)
  Updating with new translations (#3348)
  Upgrade error-prone (#3350)
  Jabref_pt_BR partially updated (#3349)
  Used late initialization for context menus (#3340)
  Fix NPE when calling with bib file as cmd argument (#3343)
  update mockito-core from 2.10.0 -> 2.11.0 (#3338)
  Remove underscore in Localized messages (#3336)
  Localisation: French: new entries translated (#3337)
  Refactoring: Lazy init of all editor tabs (#3333)
  Initializing EntryEditor Tabs on focus (#3331)
  Fix #3292: annotations are now automatically refreshed (#3325)
  Change integrity message for names depending on database mode (#3330)
  Fix location bundle with fast access (#3327)
  Small code cleanup
  Fix "path not found" exception
  Small fix in drag and drop handler of linked files (#3328)
  Fix NPE in MainTable (#3318)
  Increase relative size of abstract field in editor (#3320)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants