Used late initialization for context menus #3340

halirutan · 2017-10-24T05:46:10Z

I believe I could bring down the performance impact of the context menus to a bare minimum. I reported this in #3335 and @tobiasdiez provided me with the hint how this could work. Against his suggestion to not overshoot, I decided to prove my point that even those small context menus have a significant impact because things will sum up in the end and those menus are newly instantiated for every visible field in the editor.

The starting point was the current master-branch and profiling it by going through only 50 bib-entries. This means that the context menus are instantiated for the visible text fields. This has an impact on performance and memory and, unfortunately, it is not only a tiny fraction.

Memory

Notable are the columns "Objects", which are all objects that are created by this method and underlying sub calls and the overall "Size".

With almost 40% of the overall memory size, it's definitely something to look into.

Performance

The performance is with 8% not so large but still noticeable.

Fixing it

By using the Supplier interface, we can move the instantiation to the point where the user actually right-clicks into the field. I debugged this, and it seems to work nice. The outcome was as expected. Since no one actually triggers the context menu by just browsing through the list, it vanished altogether. Here is the used memory with this PR created by exactly scrolling through the 50 entries. First the memory footprint

and here the performance impact

In conclusion, this means as long as the user doesn't need a context menu, we have no memory or performance impact at all.

Note

I updated the checkstyle version in the Gradle built. I like to write comments, and now it doesn't prevent me any longer from using {@link ...} in doc-comments to classes that are otherwise not used.

Change in CHANGELOG.md described
Tests created for changes
Screenshots added (for bigger UI changes)
[x ] Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)
If you changed the localization: Did you run gradle localizationUpdate?

…upplier Updated checkstyle in gradle to 8.3 to be able to use {@link ..} in doc comments that refer to classes that would otherwise be "unused".

stefan-kolb · 2017-10-24T06:23:11Z

Nice work @halirutan Anytime your PRs are ready for review please add a 'ready-for-review' label.

halirutan · 2017-10-24T07:15:20Z

@stefan-kolb At the moment, I'm not sure I'm allowed to do this.

stefan-kolb · 2017-10-24T07:16:11Z

@halirutan Oops, ok I thought you already have access 😄

tobiasdiez

Perfect!

tobiasdiez · 2017-10-24T07:58:19Z

src/main/java/org/jabref/gui/fieldeditors/IdentifierEditor.java

-    @FXML private EditorTextArea textArea;
-    @FXML private Button fetchInformationByIdentifierButton;
-    @FXML private Button lookupIdentifierButton;
+    @FXML


Our convention is that these @FXML tags should be kept on the same line (I think this is also configured in the intellj style)

It's what IntelliJ did, therefore I thought it might be overlooked in the first place. I can change this

This is a fetish of @tobiasdiez :P For the sake of being consistent to what we have done before, please keep the @FXML in the same line a the instance variable.

Found it. We need to set this in our code style for IDEA

tobiasdiez · 2017-10-24T08:00:30Z

src/main/java/org/jabref/gui/fieldeditors/contextmenu/EditorMenus.java

+     * @return default context menu available for most text fields
+     */
+    public static Supplier<List<MenuItem>> getDefaultMenu(TextArea textArea) {
+        return () -> {


I'm not sure what is more readable: Hiding the supplier interface here like you did or instead writing something like addToContextMenu(() -> getDefaultMenu(textArea)). @JabRef/developers any opinions on this?

I think I am in favor of the second solution, i.e. putting the supplier into EditorTextArea.addToContextMenu(). Reasoning: With the current solution, the lazy initialization is dispersed over two classes. Changes are required in EditorTextArea and EditorMenus. If I understand everything correctly, we could build the supplier inside addToContextMenu() only. That would be a very localized change that hides the lazy initialization as far as possible.

My perception might be wrong, though. Please correct me if this is the case.

lenhard

I have some remarks regarding the code, which I would like you to fix/comment upon. Apart from that, great job!

Here's my memory profile of the keydown behavior:

Indeed even better than before.

I won't ask about a Changelog entry this time :-)

lenhard · 2017-10-24T08:42:49Z

src/main/java/org/jabref/gui/fieldeditors/IdentifierEditor.java

-    @FXML private EditorTextArea textArea;
-    @FXML private Button fetchInformationByIdentifierButton;
-    @FXML private Button lookupIdentifierButton;
+    @FXML


This is a fetish of @tobiasdiez :P For the sake of being consistent to what we have done before, please keep the @FXML in the same line a the instance variable.

lenhard · 2017-10-24T08:50:07Z

src/main/java/org/jabref/gui/fieldeditors/contextmenu/EditorMenus.java

+     * @return default context menu available for most text fields
+     */
+    public static Supplier<List<MenuItem>> getDefaultMenu(TextArea textArea) {
+        return () -> {


I think I am in favor of the second solution, i.e. putting the supplier into EditorTextArea.addToContextMenu(). Reasoning: With the current solution, the lazy initialization is dispersed over two classes. Changes are required in EditorTextArea and EditorMenus. If I understand everything correctly, we could build the supplier inside addToContextMenu() only. That would be a very localized change that hides the lazy initialization as far as possible.

My perception might be wrong, though. Please correct me if this is the case.

lenhard · 2017-10-24T08:51:12Z

src/main/java/org/jabref/gui/fieldeditors/EditorTextArea.java

        TextAreaSkin customContextSkin = new TextAreaSkin(this) {
            @Override
            public void populateContextMenu(ContextMenu contextMenu) {
                super.populateContextMenu(contextMenu);
-                contextMenu.getItems().addAll(0, items);
+                contextMenu.getItems().addAll(0, items.get());


If I understand everything correctly, we could build the supplier right here and still accept a List<MenuItem> as method parameter. That would keep the changes very localized.

More discussion see below.

Hmm, then I misunderstood the whole thing. Every text-field editor needs to attach its own context menu. Als long as I have wrapped it in a supplier, the code is not instantiated. So how is for instance IdentifierEditor supposed to attach a non-instantiated context menu?

textArea.addToContextMenu(EditorMenus.getDefaultMenu(textArea));

Assume we don't do the wrapping inside EditorMenus, then the menu would be instantiated exactly there. Except when I put the Supplier wrapper at this place. But then we have the suppliers scattered over even more classes. What do I miss?

No, Jörg, the supplier has to be built outside of this method since otherwise the context menu items are already created. With the help of a supplier, we are able to delay the creation until the user opens the context menu (since only then the get method is called).

Ok, then I was wrong. Thanks for clarifying! In that case, please disregard my comment and keep the solution.

halirutan · 2017-10-24T11:45:25Z

@lenhard I wanted to comment on this

Here's my memory profile of the keydown behavior:

The keydown behavior is a good general indicator, but there are some drawbacks. The main issue with it is that it can be heavily misleading. Assume for a moment that we ignore everything else and look only at the impact of showing the entry editor. When we have fixed an issue like the one in this PR that does not only affect memory but performance as well, then the speed of creating the entry editor will improve. That directly impacts how many entries you can visit on holding the key down.

Now assuming that we could speed up things by 50% but couldn't reduce the memory impact, then your memory monitor will make the impression if things had gone worse because it will fill the memory faster. That was the main reason why I reported both the memory and the performance separately under controlled circumstances (visiting exactly 50 entries).

What we would need (maybe we already have this) are what we call model level functional tests in IDEA plugin development. So, for instance, a test that visits n entries and simulates a click or starts JabRef and performs a search.

Those tests shouldn't be run by travis but they should be available for profiling to clearly pinpoint if we have a regression or a speedup.

lenhard · 2017-10-25T09:40:30Z

@halirutan You are right of course.

We have some GUI tests in jabref/src/test/java/org/jabref/gui/ but they are run on travis only, because they are rather annoying to run locally. Also, they are not focused on performance at all.

We also have a few performance tests in jabref/src/jmh/java/org/jabref/benchmarks/
but they are not related to the GUI and this is where our major performance problems are at the moment.

It would be good to have the tests you describe and, as usual, it depends on someone being willing to put the effort into. Most people (including myself) are a little shy when it comes to UI testing in JabRef, because the user interface has been heavily changed in the last year, meaning you have to re-write the tests all the time. If you are interested in doing something here, I would suggest you have a look at the current GUI tests, but if you want to use a better framework for building such tests, be my guest!

A new minor release of JabRef is imminent. Since I definitely want to have this in, I'll merge it right now. It is complete as far as I can see (the other refactoring I proposed does not work, as we have discussed).

Thank you for your contribution.

koppor · 2017-10-26T07:49:25Z

@halirutan - Did you sign up for https://hacktoberfest.digitalocean.com/?

halirutan · 2017-10-26T07:54:50Z

@koppor I've seen it but I thought it was a JabRef internal thing. After all, many of the contributors are located in Germany and I thought it was a fun way of dedicating a month to pushing things forward.

* 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) ...

* upstream/master: Rework AutoSetFileLinks (#3368) revert wrongly commited changes Refactorings (#3370) Fix freezing when running cleanup file operations like rename (#3315) This additional style setting for IDEA (#3355) Fix checkstyle Add proper equals to content selector 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) Delete unused code Extract difference finder from gui to logic Used late initialization for context menus (#3340) Remove unnecessary EntrySorter Move existing code to gui and rename change classes to view model Small code improvements

* upstream/master: (79 commits) Update richtext and flowless (#3386) Source tab: hide notification pane when code is correct again (#3387) Titles are optional at crossref (#3378) Update entry for every change in the source panel (#3366) Rework AutoSetFileLinks (#3368) revert wrongly commited changes Refactorings (#3370) Fix freezing when running cleanup file operations like rename (#3315) This additional style setting for IDEA (#3355) Fix checkstyle Add proper equals to content selector 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) Delete unused code Extract difference finder from gui to logic Used late initialization for context menus (#3340) Fix NPE when calling with bib file as cmd argument (#3343) ...

Used late initialization for context menus in the entry editor with S…

2efc483

…upplier Updated checkstyle in gradle to 8.3 to be able to use {@link ..} in doc comments that refer to classes that would otherwise be "unused".

stefan-kolb added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 24, 2017

tobiasdiez approved these changes Oct 24, 2017

View reviewed changes

Siedlerchr approved these changes Oct 24, 2017

View reviewed changes

lenhard reviewed Oct 24, 2017

View reviewed changes

Annotation formatting and entry into changelog.

62f6489

lenhard merged commit f6651cf into JabRef:master Oct 25, 2017

lenhard mentioned this pull request Oct 25, 2017

Can the context-menus for the field editor be reused? #3335

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Used late initialization for context menus #3340

Used late initialization for context menus #3340

halirutan commented Oct 24, 2017

stefan-kolb commented Oct 24, 2017

halirutan commented Oct 24, 2017

stefan-kolb commented Oct 24, 2017 •

edited

Loading

tobiasdiez left a comment

tobiasdiez Oct 24, 2017

halirutan Oct 24, 2017

lenhard Oct 24, 2017

halirutan Oct 24, 2017

tobiasdiez Oct 24, 2017

lenhard Oct 24, 2017

lenhard left a comment

lenhard Oct 24, 2017

lenhard Oct 24, 2017

lenhard Oct 24, 2017

halirutan Oct 24, 2017

tobiasdiez Oct 24, 2017

lenhard Oct 24, 2017

halirutan commented Oct 24, 2017

lenhard commented Oct 25, 2017

koppor commented Oct 26, 2017

halirutan commented Oct 26, 2017

Used late initialization for context menus #3340

Used late initialization for context menus #3340

Conversation

halirutan commented Oct 24, 2017

Memory

Performance

Fixing it

Note

stefan-kolb commented Oct 24, 2017

halirutan commented Oct 24, 2017

stefan-kolb commented Oct 24, 2017 • edited Loading

tobiasdiez left a 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

lenhard left a 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

halirutan commented Oct 24, 2017

lenhard commented Oct 25, 2017

koppor commented Oct 26, 2017

halirutan commented Oct 26, 2017

stefan-kolb commented Oct 24, 2017 •

edited

Loading