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

Fix preview performance #3533

Merged
merged 8 commits into from
Dec 19, 2017
Merged

Fix preview performance #3533

merged 8 commits into from
Dec 19, 2017

Conversation

halirutan
Copy link
Collaborator

@halirutan halirutan commented Dec 15, 2017

This unifies the access to the CSL engine. The adapter class will hold one instance of it and only recreate it when the style changes. This improves the performance of from the second call to the Preview drastically and should provide an acceptable speed to fix #2533.

In addition, I extended the search for citation styles when JabRef is run during development.

I will add changelog and friends, but for the moment my biggest problem is that the unit tests are broken on master (at least for me).


  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • 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?

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
When JabRef is not run as application, the citation styles are provided
in the classpath as jar. This case is now included so that citation styles
will be available during development.
The access to the CSL engine was extracted to a separate adapter class
that takes care of handling the creation of the preview strings. The layout
should be a bit clearer now.
@halirutan halirutan added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 15, 2017
Copy link
Member

@lenhard lenhard left a comment

Choose a reason for hiding this comment

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

That you so much for fixing the annoying exception in dev mode :)

Overall the code looks fine and I just have minor remarks. But I am (sometimes) getting Exceptions when using the preview. First of all, it happens to me (even when running the fat jar from command line) that citationstyles are not found:

10:25:22.452 [AWT-EventQueue-0] ERROR org.jabref.gui.preftabs.PreviewPrefsTab - something went wrong while adding the discovered CitationStyles to the list 

In that case, the configured citation style can be used, but I cannot select a new one in the preferences dialog. There, the list is empty.

Then, when trying to preview items I get exceptions. But these exceptions happen reliably for certain bib items, so it seems just like a bad combination of the randomly selected style I used for testing (ACM SIG 3+ authors) and the bib items.

For the record, here's a stack trace:

[AWT-EventQueue-0] ERROR org.jabref.gui.worker.CitationStyleWorker - Error while generating citation style
java.util.concurrent.ExecutionException: com.google.common.util.concurrent.UncheckedExecutionException: java.lang.IllegalArgumentException: Could not update items
	at java.util.concurrent.FutureTask.report(FutureTask.java:122) ~[?:1.8.0_121]
	at java.util.concurrent.FutureTask.get(FutureTask.java:192) ~[?:1.8.0_121]
	at javax.swing.SwingWorker.get(SwingWorker.java:602) ~[?:1.8.0_121]
	at org.jabref.gui.worker.CitationStyleWorker.done(CitationStyleWorker.java:63) [classes/:?]
	at javax.swing.SwingWorker$5.run(SwingWorker.java:737) [?:1.8.0_121]
	at javax.swing.SwingWorker$DoSubmitAccumulativeRunnable.run(SwingWorker.java:832) [?:1.8.0_121]
	at sun.swing.AccumulativeRunnable.run(AccumulativeRunnable.java:112) [?:1.8.0_121]
	at javax.swing.SwingWorker$DoSubmitAccumulativeRunnable.actionPerformed(SwingWorker.java:842) [?:1.8.0_121]
	at javax.swing.Timer.fireActionPerformed(Timer.java:313) [?:1.8.0_121]
	at javax.swing.Timer$DoPostEvent.run(Timer.java:245) [?:1.8.0_121]
	at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:311) [?:1.8.0_121]
	at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:756) [?:1.8.0_121]
	at java.awt.EventQueue.access$500(EventQueue.java:97) [?:1.8.0_121]
	at java.awt.EventQueue$3.run(EventQueue.java:709) [?:1.8.0_121]
	at java.awt.EventQueue$3.run(EventQueue.java:703) [?:1.8.0_121]
	at java.security.AccessController.doPrivileged(Native Method) [?:1.8.0_121]
	at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:80) [?:1.8.0_121]
	at java.awt.EventQueue.dispatchEvent(EventQueue.java:726) [?:1.8.0_121]
	at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:201) [?:1.8.0_121]
	at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:116) [?:1.8.0_121]
	at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:105) [?:1.8.0_121]
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101) [?:1.8.0_121]
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:93) [?:1.8.0_121]
	at java.awt.EventDispatchThread.run(EventDispatchThread.java:82) [?:1.8.0_121]
Caused by: com.google.common.util.concurrent.UncheckedExecutionException: java.lang.IllegalArgumentException: Could not update items
	at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2218) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache.get(LocalCache.java:4147) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:4151) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:5140) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$LocalLoadingCache.getUnchecked(LocalCache.java:5146) ~[guava-23.4-jre.jar:?]
	at org.jabref.logic.citationstyle.CitationStyleCache.getCitationFor(CitationStyleCache.java:47) ~[classes/:?]
	at org.jabref.gui.worker.CitationStyleWorker.doInBackground(CitationStyleWorker.java:50) ~[classes/:?]
	at org.jabref.gui.worker.CitationStyleWorker.doInBackground(CitationStyleWorker.java:23) ~[classes/:?]
	at javax.swing.SwingWorker$1.call(SwingWorker.java:295) ~[?:1.8.0_121]
	at java.util.concurrent.FutureTask.run(FutureTask.java:266) ~[?:1.8.0_121]
	at javax.swing.SwingWorker.run(SwingWorker.java:334) ~[?:1.8.0_121]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) ~[?:1.8.0_121]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) ~[?:1.8.0_121]
	at java.lang.Thread.run(Thread.java:745) ~[?:1.8.0_121]
Caused by: java.lang.IllegalArgumentException: Could not update items
	at de.undercouch.citeproc.CSL.registerCitationItems(CSL.java:508) ~[citeproc-java-1.0.1.jar:?]
	at org.jabref.logic.citationstyle.CSLAdapter.makeBibliography(CSLAdapter.java:55) ~[classes/:?]
	at org.jabref.logic.citationstyle.CitationStyleGenerator.generateCitations(CitationStyleGenerator.java:56) ~[classes/:?]
	at org.jabref.logic.citationstyle.CitationStyleGenerator.generateCitation(CitationStyleGenerator.java:47) ~[classes/:?]
	at org.jabref.logic.citationstyle.CitationStyleCache$1.load(CitationStyleCache.java:37) ~[classes/:?]
	at org.jabref.logic.citationstyle.CitationStyleCache$1.load(CitationStyleCache.java:34) ~[classes/:?]
	at com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3708) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2416) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2299) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2212) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache.get(LocalCache.java:4147) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:4151) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:5140) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$LocalLoadingCache.getUnchecked(LocalCache.java:5146) ~[guava-23.4-jre.jar:?]
	at org.jabref.logic.citationstyle.CitationStyleCache.getCitationFor(CitationStyleCache.java:47) ~[classes/:?]
	at org.jabref.gui.worker.CitationStyleWorker.doInBackground(CitationStyleWorker.java:50) ~[classes/:?]
	at org.jabref.gui.worker.CitationStyleWorker.doInBackground(CitationStyleWorker.java:23) ~[classes/:?]
	at javax.swing.SwingWorker$1.call(SwingWorker.java:295) ~[?:1.8.0_121]
	at java.util.concurrent.FutureTask.run(FutureTask.java:266) ~[?:1.8.0_121]
	at javax.swing.SwingWorker.run(SwingWorker.java:334) ~[?:1.8.0_121]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) ~[?:1.8.0_121]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) ~[?:1.8.0_121]
	at java.lang.Thread.run(Thread.java:745) ~[?:1.8.0_121]
Caused by: de.undercouch.citeproc.script.ScriptRunnerException: Could not call method
	at de.undercouch.citeproc.script.JREScriptRunner.callMethod(JREScriptRunner.java:121) ~[citeproc-java-1.0.1.jar:?]
	at de.undercouch.citeproc.CSL.registerCitationItems(CSL.java:506) ~[citeproc-java-1.0.1.jar:?]
	at org.jabref.logic.citationstyle.CSLAdapter.makeBibliography(CSLAdapter.java:55) ~[classes/:?]
	at org.jabref.logic.citationstyle.CitationStyleGenerator.generateCitations(CitationStyleGenerator.java:56) ~[classes/:?]
	at org.jabref.logic.citationstyle.CitationStyleGenerator.generateCitation(CitationStyleGenerator.java:47) ~[classes/:?]
	at org.jabref.logic.citationstyle.CitationStyleCache$1.load(CitationStyleCache.java:37) ~[classes/:?]
	at org.jabref.logic.citationstyle.CitationStyleCache$1.load(CitationStyleCache.java:34) ~[classes/:?]
	at com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3708) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2416) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2299) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2212) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache.get(LocalCache.java:4147) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:4151) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:5140) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$LocalLoadingCache.getUnchecked(LocalCache.java:5146) ~[guava-23.4-jre.jar:?]
	at org.jabref.logic.citationstyle.CitationStyleCache.getCitationFor(CitationStyleCache.java:47) ~[classes/:?]
	at org.jabref.gui.worker.CitationStyleWorker.doInBackground(CitationStyleWorker.java:50) ~[classes/:?]
	at org.jabref.gui.worker.CitationStyleWorker.doInBackground(CitationStyleWorker.java:23) ~[classes/:?]
	at javax.swing.SwingWorker$1.call(SwingWorker.java:295) ~[?:1.8.0_121]
	at java.util.concurrent.FutureTask.run(FutureTask.java:266) ~[?:1.8.0_121]
	at javax.swing.SwingWorker.run(SwingWorker.java:334) ~[?:1.8.0_121]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) ~[?:1.8.0_121]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) ~[?:1.8.0_121]
	at java.lang.Thread.run(Thread.java:745) ~[?:1.8.0_121]
Caused by: javax.script.ScriptException: ReferenceError: "dump" is not defined in <eval> at line number 830
	at jdk.nashorn.api.scripting.NashornScriptEngine.throwAsScriptException(NashornScriptEngine.java:470) ~[nashorn.jar:?]
	at jdk.nashorn.api.scripting.NashornScriptEngine.invokeImpl(NashornScriptEngine.java:392) ~[nashorn.jar:?]
	at jdk.nashorn.api.scripting.NashornScriptEngine.invokeMethod(NashornScriptEngine.java:199) ~[nashorn.jar:?]
	at de.undercouch.citeproc.script.JREScriptRunner.callMethod(JREScriptRunner.java:117) ~[citeproc-java-1.0.1.jar:?]
	at de.undercouch.citeproc.CSL.registerCitationItems(CSL.java:506) ~[citeproc-java-1.0.1.jar:?]
	at org.jabref.logic.citationstyle.CSLAdapter.makeBibliography(CSLAdapter.java:55) ~[classes/:?]
	at org.jabref.logic.citationstyle.CitationStyleGenerator.generateCitations(CitationStyleGenerator.java:56) ~[classes/:?]
	at org.jabref.logic.citationstyle.CitationStyleGenerator.generateCitation(CitationStyleGenerator.java:47) ~[classes/:?]
	at org.jabref.logic.citationstyle.CitationStyleCache$1.load(CitationStyleCache.java:37) ~[classes/:?]
	at org.jabref.logic.citationstyle.CitationStyleCache$1.load(CitationStyleCache.java:34) ~[classes/:?]
	at com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3708) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2416) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2299) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2212) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache.get(LocalCache.java:4147) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:4151) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:5140) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$LocalLoadingCache.getUnchecked(LocalCache.java:5146) ~[guava-23.4-jre.jar:?]
	at org.jabref.logic.citationstyle.CitationStyleCache.getCitationFor(CitationStyleCache.java:47) ~[classes/:?]
	at org.jabref.gui.worker.CitationStyleWorker.doInBackground(CitationStyleWorker.java:50) ~[classes/:?]
	at org.jabref.gui.worker.CitationStyleWorker.doInBackground(CitationStyleWorker.java:23) ~[classes/:?]
	at javax.swing.SwingWorker$1.call(SwingWorker.java:295) ~[?:1.8.0_121]
	at java.util.concurrent.FutureTask.run(FutureTask.java:266) ~[?:1.8.0_121]
	at javax.swing.SwingWorker.run(SwingWorker.java:334) ~[?:1.8.0_121]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) ~[?:1.8.0_121]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) ~[?:1.8.0_121]
	at java.lang.Thread.run(Thread.java:745) ~[?:1.8.0_121]
Caused by: jdk.nashorn.internal.runtime.ECMAException: ReferenceError: "dump" is not defined
	at jdk.nashorn.internal.runtime.ECMAErrors.error(ECMAErrors.java:57) ~[nashorn.jar:?]
	at jdk.nashorn.internal.runtime.ECMAErrors.referenceError(ECMAErrors.java:319) ~[nashorn.jar:?]
	at jdk.nashorn.internal.runtime.ECMAErrors.referenceError(ECMAErrors.java:291) ~[nashorn.jar:?]
	at jdk.nashorn.internal.objects.Global.__noSuchProperty__(Global.java:1441) ~[nashorn.jar:?]
	at jdk.nashorn.internal.scripts.Script$Recompilation$195$30224A$\^eval\_.error(<eval>:830) ~[?:?]
	at jdk.nashorn.internal.scripts.Script$Recompilation$194$523263A$\^eval\_.dateParseArray(<eval>:12730) ~[?:?]
	at jdk.nashorn.internal.scripts.Script$Recompilation$191$100473A$\^eval\_.retrieveItem(<eval>:2870) ~[?:?]
	at jdk.nashorn.internal.scripts.Script$Recompilation$190$617052A$\^eval\_.doinserts(<eval>:15103) ~[?:?]
	at jdk.nashorn.internal.scripts.Script$Recompilation$187$235101AAAA$\^eval\_.updateItems(<eval>:6064) ~[?:?]
	at jdk.nashorn.internal.runtime.ScriptFunctionData.invoke(ScriptFunctionData.java:665) ~[nashorn.jar:?]
	at jdk.nashorn.internal.runtime.ScriptFunction.invoke(ScriptFunction.java:494) ~[nashorn.jar:?]
	at jdk.nashorn.internal.runtime.ScriptRuntime.apply(ScriptRuntime.java:393) ~[nashorn.jar:?]
	at jdk.nashorn.api.scripting.ScriptObjectMirror.callMember(ScriptObjectMirror.java:199) ~[nashorn.jar:?]
	at jdk.nashorn.api.scripting.NashornScriptEngine.invokeImpl(NashornScriptEngine.java:386) ~[nashorn.jar:?]
	at jdk.nashorn.api.scripting.NashornScriptEngine.invokeMethod(NashornScriptEngine.java:199) ~[nashorn.jar:?]
	at de.undercouch.citeproc.script.JREScriptRunner.callMethod(JREScriptRunner.java:117) ~[citeproc-java-1.0.1.jar:?]
	at de.undercouch.citeproc.CSL.registerCitationItems(CSL.java:506) ~[citeproc-java-1.0.1.jar:?]
	at org.jabref.logic.citationstyle.CSLAdapter.makeBibliography(CSLAdapter.java:55) ~[classes/:?]
	at org.jabref.logic.citationstyle.CitationStyleGenerator.generateCitations(CitationStyleGenerator.java:56) ~[classes/:?]
	at org.jabref.logic.citationstyle.CitationStyleGenerator.generateCitation(CitationStyleGenerator.java:47) ~[classes/:?]
	at org.jabref.logic.citationstyle.CitationStyleCache$1.load(CitationStyleCache.java:37) ~[classes/:?]
	at org.jabref.logic.citationstyle.CitationStyleCache$1.load(CitationStyleCache.java:34) ~[classes/:?]
	at com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3708) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2416) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2299) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2212) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache.get(LocalCache.java:4147) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:4151) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:5140) ~[guava-23.4-jre.jar:?]
	at com.google.common.cache.LocalCache$LocalLoadingCache.getUnchecked(LocalCache.java:5146) ~[guava-23.4-jre.jar:?]
	at org.jabref.logic.citationstyle.CitationStyleCache.getCitationFor(CitationStyleCache.java:47) ~[classes/:?]
	at org.jabref.gui.worker.CitationStyleWorker.doInBackground(CitationStyleWorker.java:50) ~[classes/:?]
	at org.jabref.gui.worker.CitationStyleWorker.doInBackground(CitationStyleWorker.java:23) ~[classes/:?]
	at javax.swing.SwingWorker$1.call(SwingWorker.java:295) ~[?:1.8.0_121]
	at java.util.concurrent.FutureTask.run(FutureTask.java:266) ~[?:1.8.0_121]
	at javax.swing.SwingWorker.run(SwingWorker.java:334) ~[?:1.8.0_121]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) ~[?:1.8.0_121]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) ~[?:1.8.0_121]
	at java.lang.Thread.run(Thread.java:745) ~[?:1.8.0_121]

And here's a corresponding item:

@InProceedings{harrer2012conformance,
  author    = {Simon Harrer and J\"org Lenhard and Guido Wirtz},
  title     = {{BPEL Conformance in Open Source Engines}},
  booktitle = {5th IEEE International Conference on Service-Oriented Computing and Applications},
  year      = {2012},
  pages     = {237--244},
  address   = {Taipei, Taiwan},
  month     = {December 17-19},
  doi       = {10.1109/soca.2012.6449467},
}

* Creates the bibliography of the provided items. This method needs to run synchronized because the underlying
* CSL engine is not thread-safe.
*/
static synchronized List<String> makeBibliography(List<BibEntry> bibEntries, String style, CitationStyleOutputFormat outputFormat) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

As far as I can see by looking at the code, this solution should be thread-safe. You could turn up parallelism by a tiny notch, but I cannot say if this has any tangible benefit. This could be done by not marking the whole method as synchronized, but just the part where the CSL engine is used. To do so, you would add a private lock instance variable, e.g. private static Object lock = new Object(); and then refactor the method like this:

        List<CSLItemData> cslItemData = new ArrayList<>(bibEntries.size());
        for (BibEntry bibEntry : bibEntries) {
            cslItemData.add(bibEntryToCSLItemData(bibEntry));
        }

        synchronized(lock) {
            initialize(style, outputFormat);
            dataProvider.setData(cslItemData);
            cslInstance.registerCitationItems(dataProvider.getIds());
            Bibliography bibliography = cslInstance.makeBibliography();
            return Arrays.asList(bibliography.getEntries());
        }

You can refactor the code in this way or not, I leave this up to you. It will only have an effect if there are many concurrent calls to the method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is an easy fix indeed. I will profile where the bottleneck is when you have many items selected and click on Copy Preview. It might be useful to check if the loop itself should be parallelized as well. Copying the preview is too slow for my taste and I'm unsure it really comes from CSL this time. I will check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I profiled this and it's having no significant impact on the performance. After refactoring the creation of the cslItemData was absorbed into our data provider and must be run synchronized as well or the data we provide to CSL will get out of synch.

String path = CitationStyle.class.getProtectionDomain().getCodeSource().getLocation().toURI().getPath();

// This is a quick fix to have the styles when running JabRef in a development environment.
// The styles.jar is not extracted into the JabRef.jar and therefore, we search the classpath for it.
if ((new File(path)).isDirectory()) {
Copy link
Member

Choose a reason for hiding this comment

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

Whenever we add/modify file IO code in JabRef, we try to use the new Java API for that java.nio, not java.io. So please replace the condition with Files.isDirectory(Paths.get(path)).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK. No problem.

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

Nice work! I have some ideas on how to minimally improve the code but I'm not sure if they can be implemented easily / if it they actually make sense. I'll leave this to your judgment.

private static String style;
private static CitationStyleOutputFormat format;
private static JabRefItemDataProvider dataProvider = new JabRefItemDataProvider();
private static CSL cslInstance;
Copy link
Member

Choose a reason for hiding this comment

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

Is it needed, that everything is static here? I actually would prefer a usual class with non-standard methods/instance variables.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. This would probably be better. We could then still instantiate several CSL adapters if we need one for whatever purpose.

}

@Override
public CSLItemData retrieveItem(String id) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is really an advantage but since you are already implementing a custom data provider, I would suggest the following:

  • setData accepts a list of BibEntry
  • retriveItem gets the BibEntry with the given key and then returns the result of bibEntryToCSLItemData. This solution might be a bit more memory effective since you don't have an additional list (but it only makes sense if the bibEntryToCSLItemData method is not expensive).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ha funny. It's the exact same thing I have thought about. I wanted to wait until we are sure it works in general. I too would prefer to make a real JabRefItemDataProvider that works on BibEntry.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I refactored this. retrieveItem cannot change its signature because it is called internally by CSL (that's why it is overridden). Still, the layout is much clearer now.

@halirutan
Copy link
Collaborator Author

@lenhard @tobiasdiez Thanks for the fast review. Here are to comments to Lenhard's first message:

In that case, the configured citation style can be used, but I cannot select a new one in the preferences dialog. There, the list is empty.

Yes. I did not consider this case. I assume the fat-jar contains the styles still packed as jar. I'm sure I can fix this.

Then, when trying to preview items I get exceptions. But these exceptions happen reliably for certain bib items, so it seems just like a bad combination of the randomly selected style I used for testing (ACM SIG 3+ authors) and the bib items.

Took a while, but I could pin it down. I'm not sure I caught the exception before but now you will see "Cannot create preview" and the exception appears in the log as usual. The error comes from your BibEntry. When you look at your BiBTeX code, you see

month     = {December 17-19},

If this is indeed valid BibTeX, then our CSL converter does not handle it correctly. You can find the Json format of what org.jabref.logic.citationstyle.CSLAdapter#bibEntryToCSLItemData makes from your code. It's so short that you spot the error easily. CSL is choking on the invalid date-parts.
As soon as you remove the days, the item is rendered correctly.

This error should have happened before as I did not touch that part. I'm not sure how this can reliably be fixed as we only extract the latex-free fields and turn them into strings. That should probably be a different PR.

@lenhard
Copy link
Member

lenhard commented Dec 15, 2017

@halirutan Thanks for the investigation and I hope you did not waste too much time on that. That is not correct bibtex (well bibtex is very open and relaxed and of course you can write it in this way, but this is not how the content of the month field is expected), I just opened an old bib file and clicked around, not caring too much about the actual data. You can ignore that error in your PR here.

I think a viable strategy would actually be to absorb the exception completely. Exceptions in JabRef's log should indicate problems in JabRef. This is not a problem of JabRef, but of user data and the citation styles library. Therefore, I think it would be sufficient to create a log entry stating that the attempt to parse an entry results in an error from the citation styles library. But this can be part of a different PR.

While the CitationStyleCache uses the complete source of the style definition,
this file used only the csl file name. To prevent useless re-initialization
of the CSLAdapter, this was unified to use the same approach as in the
preview worker.
CSLAdapter can now be instantiated several times. Each instance will hold
its own CSL instance and will be fast after the first call to makeBibliography.
CitationStyleGenerator has now its own static CSLAdapter instance.

JabRefItemDataProvider is now refactored to work on BibEntry.
@halirutan
Copy link
Collaborator Author

Things that need to work:

  1. Fast scrolling through the entry liest with preview open
  2. Edit -> Copy Preview for single and many items
  3. On a selection right click and Copy citation

@lenhard I cannot reproduce that the styles are not found with the fat jar. Can you try to delete and rebuild it and show exactly what you try? I see all styles and I can switch them as usual. The fat jar has the same structure as the normal jar with all styles included as files.

@lenhard
Copy link
Member

lenhard commented Dec 16, 2017

I am doing the following:

  • Execute gradlew build
  • cd to build/libs
  • execute java -jar JabRef-4.1-dev.jar
  • For some reason there is nothing available to select in the preview panel (see picture), although the CSL files are contained in the fat jar (not as a jar, but as the single .csl files right in the root of the jar). The log says
Opening: C:\Users\joerg\Desktop\jl-main\diss\tex\references.bib
something went wrong while adding the discovered CitationStyles to the list 

preview

Furthermore, I am getting several warnings on the console:

Dec 16, 2017 10:02:18 PM javafx.scene.CssStyleHelper calculateValue
WARNING: Could not resolve '-fx-info' while resolving lookups for '-fx-text-fill' from rule '*.log' in stylesheet jar:file:/C:/workspaces/jabref/build/libs/JabRef-4.1-dev-fat.jar!/org/jabref/gui/errorconsole/ErrorConsole.css

After these warnings I updated my current installation from Java 1.8.0_144 to Java 1.8.0_152 and the warnings were gone, but the rest stays the same (no citation styles in the list). My configuration is now

JabRef 4.1-dev
Windows 10 10.0 amd64 
Java 1.8.0_152

@Siedlerchr
Copy link
Member

I tested it and I found there is a problem with how you get the path. Will commit a fix.

Use FileSystem to iterate the content of the jar files
@Siedlerchr
Copy link
Member

@halirutan @lenhard I fixed the path handling and improved the code for searching inside the jar file.
For me it works now when I directly run JabRef Main in Eclipse 🙌 😃

@halirutan
Copy link
Collaborator Author

@Siedlerchr Can you point out what exactly went wrong with the path so that I don't make the mistake in the future? Otherwise nice clean up! I removed your println, I guess you simply forgot this and I renamed jarfs to jarFs to satisfy my spell check (I like to have absolutely no warning if possible 😃 ).

@lenhard Can you test it again. Your approach is basically what I did too and here it worked.

citationStyles.add(citationStyle);
}

Path filePath = Paths.get(CitationStyle.class.getProtectionDomain().getCodeSource().getLocation().toURI());
Copy link
Member

Choose a reason for hiding this comment

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

@halirutan The Paths.get method has an overload for accepting an URI.
The misleading method of the URI class (getPath) does not work as itdoesn't inlcude the "scheme" part of the URI object which is used to identify the underlying FileSytem implementation.
This tutorial has a nice overview of "what is a path" https://docs.oracle.com/javase/tutorial/essential/io/path.html

If you happen to use a Classpath url or sth else, the recommended way is the same Paths.get(Class.toUrl.toUri)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That wasn't my code :) When I see this right, it was coming from Christian Bartsch but it doesn't matter. Good that you spotted it.

Copy link
Member

@lenhard lenhard left a comment

Choose a reason for hiding this comment

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

@halirutan I've tested again, and now the citation styles are again listed in the preferences dialog, as they should be. Thanks!

@tobiasdiez tobiasdiez merged commit 6d2ec48 into master Dec 19, 2017
@tobiasdiez tobiasdiez deleted the fix_previewPerformance branch December 19, 2017 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Citation preview incrementally increases memory usage with each preview
4 participants