From 6d2ec481cdad6029b29a3b3d217f23ab47e92588 Mon Sep 17 00:00:00 2001 From: Patrick Scheibe Date: Tue, 19 Dec 2017 18:36:00 +0100 Subject: [PATCH] Fix preview performance (#3533) * Fix searching for the citation styles in dev environment 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. * Improve performance of entry preview; Fix searching for citation styles 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. * Fix style format to prevent CSL re-initialization 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. * Refactor CSLAdapter and change minor things 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. * Rename static field to match style * Make access-level explicit to satisfy codacy * Correct path handling for library Use FileSystem to iterate the content of the jar files * Remove println and rename variable --- CHANGELOG.md | 1 + .../jabref/gui/preftabs/PreviewPrefsTab.java | 7 +- .../CitationStyleToClipboardWorker.java | 13 +- .../logic/citationstyle/CSLAdapter.java | 116 ++++++++++++++++++ .../logic/citationstyle/CitationStyle.java | 83 ++++++++----- .../citationstyle/CitationStyleCache.java | 4 +- .../citationstyle/CitationStyleGenerator.java | 51 ++------ 7 files changed, 196 insertions(+), 79 deletions(-) create mode 100644 src/main/java/org/jabref/logic/citationstyle/CSLAdapter.java diff --git a/CHANGELOG.md b/CHANGELOG.md index ebff550c998..19b8bf79814 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `# - In the preference, all installed java Look and Feels are now listed and selectable - We added an ID fetcher for [IACR eprints](https://eprint.iacr.org/). [#3473](https://github.com/JabRef/jabref/pull/3473) - We added a clear option to the right-click menu of the text field in the entry editor. [koppor#198](https://github.com/koppor/jabref/issues/198) +- We improved the performance and memory footprint of the citation preview when CSL styles are used. [#2533](https://github.com/JabRef/jabref/issues/2533) ### Fixed - We fixed the translation of \textendash and \textquotesingle in the entry preview [#3307](https://github.com/JabRef/jabref/issues/3307) diff --git a/src/main/java/org/jabref/gui/preftabs/PreviewPrefsTab.java b/src/main/java/org/jabref/gui/preftabs/PreviewPrefsTab.java index b1ef3193fe0..599eb9f14bf 100644 --- a/src/main/java/org/jabref/gui/preftabs/PreviewPrefsTab.java +++ b/src/main/java/org/jabref/gui/preftabs/PreviewPrefsTab.java @@ -3,6 +3,7 @@ import java.awt.BorderLayout; import java.awt.Dimension; import java.util.ArrayList; +import java.util.Comparator; import java.util.Enumeration; import java.util.List; import java.util.concurrent.ExecutionException; @@ -208,8 +209,8 @@ public void done() { } try { get().stream() - .filter(style -> !previewPreferences.getPreviewCycle().contains(style.getFilepath())) - .sorted((style0, style1) -> style0.getTitle().compareTo(style1.getTitle())) + .filter(style -> !previewPreferences.getPreviewCycle().contains(style.getFilePath())) + .sorted(Comparator.comparing(CitationStyle::getTitle)) .forEach(availableModel::addElement); btnRight.setEnabled(!availableModel.isEmpty()); @@ -230,7 +231,7 @@ public void storeSettings() { while (elements.hasMoreElements()) { Object obj = elements.nextElement(); if (obj instanceof CitationStyle) { - styles.add(((CitationStyle) obj).getFilepath()); + styles.add(((CitationStyle) obj).getFilePath()); } else if (obj instanceof String) { styles.add("Preview"); } diff --git a/src/main/java/org/jabref/gui/worker/CitationStyleToClipboardWorker.java b/src/main/java/org/jabref/gui/worker/CitationStyleToClipboardWorker.java index 9b7773ebd03..b50cb1df482 100644 --- a/src/main/java/org/jabref/gui/worker/CitationStyleToClipboardWorker.java +++ b/src/main/java/org/jabref/gui/worker/CitationStyleToClipboardWorker.java @@ -57,8 +57,17 @@ public CitationStyleToClipboardWorker(BasePanel basePanel, CitationStyleOutputFo @Override protected List doInBackground() throws Exception { + // This worker stored the style as filename. The CSLAdapter and the CitationStyleCache store the source of the + // style. Therefore, we extract the style source from the file. + String styleSource = null; if (CitationStyle.isCitationStyleFile(style)) { - return CitationStyleGenerator.generateCitations(selectedEntries, style, outputFormat); + final CitationStyle citationStyleFromFile = CitationStyle.createCitationStyleFromFile(style); + if (citationStyleFromFile != null && !citationStyleFromFile.getSource().isEmpty()) { + styleSource = citationStyleFromFile.getSource(); + } + } + if (styleSource != null) { + return CitationStyleGenerator.generateCitations(selectedEntries, styleSource, outputFormat); } else { StringReader sr = new StringReader(previewStyle.replace("__NEWLINE__", "\n")); LayoutFormatterPreferences layoutFormatterPreferences = Globals.prefs.getLayoutFormatterPreferences(Globals.journalAbbreviationLoader); @@ -80,7 +89,6 @@ public void done() { // if it's not a citation style take care of the preview if (!CitationStyle.isCitationStyleFile(style)) { new ClipBoardManager().setTransferableClipboardContents(processPreview(citations)); - } else { // if it's generated by a citation style take care of each output format Transferable transferable; @@ -181,5 +189,4 @@ protected static HtmlTransferable processHtml(List citations) { return new HtmlTransferable(result); } - } diff --git a/src/main/java/org/jabref/logic/citationstyle/CSLAdapter.java b/src/main/java/org/jabref/logic/citationstyle/CSLAdapter.java new file mode 100644 index 00000000000..631e0c030d9 --- /dev/null +++ b/src/main/java/org/jabref/logic/citationstyle/CSLAdapter.java @@ -0,0 +1,116 @@ +package org.jabref.logic.citationstyle; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Objects; +import java.util.Optional; + +import org.jabref.model.entry.BibEntry; + +import de.undercouch.citeproc.CSL; +import de.undercouch.citeproc.ItemDataProvider; +import de.undercouch.citeproc.ListItemDataProvider; +import de.undercouch.citeproc.bibtex.BibTeXConverter; +import de.undercouch.citeproc.csl.CSLItemData; +import de.undercouch.citeproc.output.Bibliography; +import org.jbibtex.BibTeXEntry; +import org.jbibtex.DigitStringValue; +import org.jbibtex.Key; + +/** + * Provides an adapter class to CSL. It holds a CSL instance under the hood that is only recreated when + * the style changes. + * + * @apiNote The first call to {@link #makeBibliography} is expensive since the + * CSL instance will be created. As long as the style stays the same, we can reuse this instance. On style-change, the + * engine is re-instantiated. Therefore, the use-case of this class is many calls to {@link #makeBibliography} with the + * same style. Changing the output format is cheap. + * @implNote The main function {@link #makeBibliography} will enforce + * synchronized calling. The main CSL engine under the hood is not thread-safe. Since this class is usually called from + * a SwingWorker, the only other option would be to create several CSL instances which is wasting a lot of resources and very slow. + * In the current scheme, {@link #makeBibliography} can be called as usual + * SwingWorker task and to the best of my knowledge, concurrent calls will pile up and processed sequentially. + */ +public class CSLAdapter { + + private static final BibTeXConverter BIBTEX_CONVERTER = new BibTeXConverter(); + private final JabRefItemDataProvider dataProvider = new JabRefItemDataProvider(); + private String style; + private CitationStyleOutputFormat format; + private CSL cslInstance; + + /** + * Creates the bibliography of the provided items. This method needs to run synchronized because the underlying + * CSL engine is not thread-safe. + */ + public synchronized List makeBibliography(List bibEntries, String style, CitationStyleOutputFormat outputFormat) throws IOException, IllegalArgumentException { + dataProvider.setData(bibEntries); + initialize(style, outputFormat); + cslInstance.registerCitationItems(dataProvider.getIds()); + final Bibliography bibliography = cslInstance.makeBibliography(); + return Arrays.asList(bibliography.getEntries()); + } + + /** + * Initialized the static CSL instance if needed. + * + * @param newStyle journal style of the output + * @param newFormat usually HTML or RTF. + * @throws IOException An error occurred in the underlying JavaScript framework + */ + private void initialize(String newStyle, CitationStyleOutputFormat newFormat) throws IOException { + if (cslInstance == null || !Objects.equals(newStyle, style)) { + cslInstance = new CSL(dataProvider, newStyle); + style = newStyle; + } + + if (!Objects.equals(newFormat, format)) { + cslInstance.setOutputFormat(newFormat.getFormat()); + format = newFormat; + } + } + + /** + * Custom ItemDataProvider that allows to set the data so that we don't have to instantiate a new CSL object + * every time. + */ + private static class JabRefItemDataProvider implements ItemDataProvider { + + private ItemDataProvider dataProvider = new ListItemDataProvider(); + + public void setData(List data) { + List cslItemData = new ArrayList<>(data.size()); + for (BibEntry bibEntry : data) { + cslItemData.add(bibEntryToCSLItemData(bibEntry)); + } + dataProvider = new ListItemDataProvider(cslItemData); + } + + @Override + public CSLItemData retrieveItem(String id) { + return dataProvider.retrieveItem(id); + } + + @Override + public String[] getIds() { + return dataProvider.getIds(); + } + + /** + * Converts the {@link BibEntry} into {@link CSLItemData}. + */ + private CSLItemData bibEntryToCSLItemData(BibEntry bibEntry) { + String citeKey = bibEntry.getCiteKeyOptional().orElse(""); + BibTeXEntry bibTeXEntry = new BibTeXEntry(new Key(bibEntry.getType()), new Key(citeKey)); + + // Not every field is already generated into latex free fields + for (String key : bibEntry.getFieldMap().keySet()) { + Optional latexFreeField = bibEntry.getLatexFreeField(key); + latexFreeField.ifPresent(value -> bibTeXEntry.addField(new Key(key), new DigitStringValue(value))); + } + return BIBTEX_CONVERTER.toItemData(bibTeXEntry); + } + } +} diff --git a/src/main/java/org/jabref/logic/citationstyle/CitationStyle.java b/src/main/java/org/jabref/logic/citationstyle/CitationStyle.java index 6e49bae2472..e6cd17fb137 100644 --- a/src/main/java/org/jabref/logic/citationstyle/CitationStyle.java +++ b/src/main/java/org/jabref/logic/citationstyle/CitationStyle.java @@ -5,17 +5,20 @@ import java.net.URISyntaxException; import java.net.URL; import java.nio.charset.StandardCharsets; +import java.nio.file.FileSystem; +import java.nio.file.FileSystems; import java.nio.file.Files; import java.nio.file.NoSuchFileException; +import java.nio.file.Path; import java.nio.file.Paths; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; -import java.util.Enumeration; import java.util.List; import java.util.Objects; -import java.util.jar.JarEntry; -import java.util.jar.JarFile; +import java.util.Optional; +import java.util.regex.Pattern; +import java.util.stream.Collectors; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; @@ -33,23 +36,25 @@ import org.xml.sax.InputSource; import org.xml.sax.SAXException; - /** - * Representation of a CitationStyle - * Stores its name, the filepath and the style itself + * Representation of a CitationStyle. + * Stores its name, the file path and the style itself */ public class CitationStyle { public static final String DEFAULT = "/ieee.csl"; private static final Log LOGGER = LogFactory.getLog(CitationStyle.class); - private final String filepath; + private static final Pattern SNAPSHOT_NAME = Pattern.compile(".*styles-1\\.0\\.1-SNAPSHOT\\.jar"); + + private static final List STYLES = new ArrayList<>(); + + private final String filePath; private final String title; private final String source; - private CitationStyle(final String filename, final String title, final String source) { - this.filepath = Objects.requireNonNull(filename); + this.filePath = Objects.requireNonNull(filename); this.title = Objects.requireNonNull(title); this.source = Objects.requireNonNull(source); } @@ -58,7 +63,7 @@ private CitationStyle(final String filename, final String title, final String so * Creates an CitationStyle instance out of the style string */ private static CitationStyle createCitationStyleFromSource(final String source, final String filename) { - if (filename != null && !filename.isEmpty() && source != null && !source.isEmpty()) { + if ((filename != null) && !filename.isEmpty() && (source != null) && !source.isEmpty()) { try { DocumentBuilder db = DocumentBuilderFactory.newInstance().newDocumentBuilder(); InputSource is = new InputSource(); @@ -115,36 +120,51 @@ public static CitationStyle createCitationStyleFromFile(final String styleFile) return null; } + /** + * Provides the default citation style which is currently IEEE + * @return default citation style + */ public static CitationStyle getDefault() { return createCitationStyleFromFile(DEFAULT); } /** - * THIS ONLY WORKS WHEN JabRef IS STARTED AS AN APPLICATION (JAR) - * - * Reads all available CitationStyle in the Jar + * Provides the citation styles that come with JabRef. + * @return list of available citation styles */ public static List discoverCitationStyles() { + if (!STYLES.isEmpty()) { + return STYLES; + } try { - final List citationStyles = new ArrayList<>(); - String path = CitationStyle.class.getProtectionDomain().getCodeSource().getLocation().toURI().getPath(); - - try (JarFile file = new JarFile(path)) { - Enumeration entries = file.entries(); - while (entries.hasMoreElements()) { - String filename = entries.nextElement().getName(); - if (!filename.startsWith("dependent") && filename.endsWith("csl")) { - CitationStyle citationStyle = CitationStyle.createCitationStyleFromFile(filename); - if (citationStyle != null) { - citationStyles.add(citationStyle); - } + + Path filePath = Paths.get(CitationStyle.class.getProtectionDomain().getCodeSource().getLocation().toURI()); + String path = filePath.toString(); + + // 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 (Files.isDirectory(filePath)) { + final String cp = System.getProperty("java.class.path"); + final String[] entries = cp.split(System.getProperty("path.separator")); + + Optional foundStyle = Arrays.stream(entries).filter(entry -> SNAPSHOT_NAME.matcher(entry).matches()).findFirst(); + path = foundStyle.orElse(path); + } + + try (FileSystem jarFs = FileSystems.newFileSystem(Paths.get(path), null)) { + + List allStyles = Files.find(jarFs.getRootDirectories().iterator().next(), 1, (file, attr) -> file.toString().endsWith("csl")).collect(Collectors.toList()); + + for (Path style : allStyles) { + CitationStyle citationStyle = CitationStyle.createCitationStyleFromFile(style.getFileName().toString()); + if (citationStyle != null) { + STYLES.add(citationStyle); } } } - return citationStyles; + return STYLES; } catch (IOException | URISyntaxException ex) { - LOGGER.error("something went wrong while searching available CitationStyles. " + - "Are you running directly from source code?", ex); + LOGGER.error("something went wrong while searching available CitationStyles. Are you running directly from source code?", ex); } return Collections.emptyList(); } @@ -164,8 +184,8 @@ public String getSource() { return source; } - public String getFilepath() { - return filepath; + public String getFilePath() { + return filePath; } @Override @@ -187,8 +207,7 @@ public boolean equals(Object o) { } @Override - public int hashCode() - { + public int hashCode() { return Objects.hash(source); } diff --git a/src/main/java/org/jabref/logic/citationstyle/CitationStyleCache.java b/src/main/java/org/jabref/logic/citationstyle/CitationStyleCache.java index e1fa873a9e0..df2f16659a4 100644 --- a/src/main/java/org/jabref/logic/citationstyle/CitationStyleCache.java +++ b/src/main/java/org/jabref/logic/citationstyle/CitationStyleCache.java @@ -22,7 +22,7 @@ public class CitationStyleCache { private static final int CACHE_SIZE = 1024; private CitationStyle citationStyle; - private LoadingCache citationStyleCache; + private final LoadingCache citationStyleCache; public CitationStyleCache(BibDatabaseContext bibDatabaseContext) { @@ -33,7 +33,7 @@ public CitationStyleCache(BibDatabaseContext bibDatabaseContext, CitationStyle c this.citationStyle = citationStyle; citationStyleCache = CacheBuilder.newBuilder().maximumSize(CACHE_SIZE).build(new CacheLoader() { @Override - public String load(BibEntry entry) throws Exception { + public String load(BibEntry entry) { return CitationStyleGenerator.generateCitation(entry, getCitationStyle().getSource(), CitationStyleOutputFormat.HTML); } }); diff --git a/src/main/java/org/jabref/logic/citationstyle/CitationStyleGenerator.java b/src/main/java/org/jabref/logic/citationstyle/CitationStyleGenerator.java index 064b947d447..3910f71c559 100644 --- a/src/main/java/org/jabref/logic/citationstyle/CitationStyleGenerator.java +++ b/src/main/java/org/jabref/logic/citationstyle/CitationStyleGenerator.java @@ -1,74 +1,63 @@ package org.jabref.logic.citationstyle; import java.io.IOException; -import java.util.Arrays; import java.util.Collections; import java.util.List; -import java.util.Optional; import org.jabref.logic.l10n.Localization; import org.jabref.model.entry.BibEntry; -import de.undercouch.citeproc.CSL; -import de.undercouch.citeproc.bibtex.BibTeXConverter; -import de.undercouch.citeproc.csl.CSLItemData; -import de.undercouch.citeproc.output.Bibliography; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; -import org.jbibtex.BibTeXEntry; -import org.jbibtex.DigitStringValue; -import org.jbibtex.Key; import org.jbibtex.TokenMgrException; /** - * WARNING: the citation is generated using JavaScript which may take some time, better call it from outside the main Thread + * Facade to unify the access to the citation style engine. Use these methods if you need rendered BibTeX item(s) in a + * given journal style. This class uses {@link CSLAdapter} to create output. */ public class CitationStyleGenerator { private static final Log LOGGER = LogFactory.getLog(CitationStyleGenerator.class); - private static final BibTeXConverter BIBTEX_CONVERTER = new BibTeXConverter(); + private static final CSLAdapter CSL_ADAPTER = new CSLAdapter(); private CitationStyleGenerator() { } /** - * WARNING: the citation is generated using JavaScript which may take some time, better call it from outside the main Thread * Generates a Citation based on the given entry and style + * @implNote the citation is generated using JavaScript which may take some time, better call it from outside the main Thread */ protected static String generateCitation(BibEntry entry, CitationStyle style) { return generateCitation(entry, style.getSource(), CitationStyleOutputFormat.HTML); } /** - * WARNING: the citation is generated using JavaScript which may take some time, better call it from outside the main Thread * Generates a Citation based on the given entry and style + * @implNote the citation is generated using JavaScript which may take some time, better call it from outside the main Thread */ protected static String generateCitation(BibEntry entry, String style) { return generateCitation(entry, style, CitationStyleOutputFormat.HTML); } /** - * WARNING: the citation is generated using JavaScript which may take some time, better call it from outside the main Thread * Generates a Citation based on the given entry, style, and output format + * @implNote the citation is generated using JavaScript which may take some time, better call it from outside the main Thread */ protected static String generateCitation(BibEntry entry, String style, CitationStyleOutputFormat outputFormat) { return generateCitations(Collections.singletonList(entry), style, outputFormat).get(0); } /** - * WARNING: the citation is generated using JavaScript which may take some time, better call it from outside the main Thread - * Generates the citation for multiple entries at once. This is useful when the Citation Style has an increasing number + * Generates the citation for multiple entries at once. + * @implNote The citations are generated using JavaScript which may take some time, better call it from outside the main thread. */ public static List generateCitations(List bibEntries, String style, CitationStyleOutputFormat outputFormat) { try { - CSLItemData[] cslItemData = new CSLItemData[bibEntries.size()]; - for (int i = 0; i < bibEntries.size(); i++) { - cslItemData[i] = bibEntryToCSLItemData(bibEntries.get(i)); - } - Bibliography bibliography = CSL.makeAdhocBibliography(style, outputFormat.getFormat(), cslItemData); - return Arrays.asList(bibliography.getEntries()); - + return CSL_ADAPTER.makeBibliography(bibEntries, style, outputFormat); + } catch (IllegalArgumentException ignored) { + LOGGER.error("Could not generate BibEntry citation. The CSL engine could not create a preview for your item."); + return Collections.singletonList(Localization.lang("Cannot generate preview based on selected citation style.")); } catch (IOException | ArrayIndexOutOfBoundsException e) { LOGGER.error("Could not generate BibEntry citation", e); return Collections.singletonList(Localization.lang("Cannot generate preview based on selected citation style.")); @@ -84,20 +73,4 @@ public static List generateCitations(List bibEntries, String s .toString()); } } - - /** - * Converts the {@link BibEntry} into {@link CSLItemData}. - */ - private static CSLItemData bibEntryToCSLItemData(BibEntry bibEntry) { - String citeKey = bibEntry.getCiteKeyOptional().orElse(""); - BibTeXEntry bibTeXEntry = new BibTeXEntry(new Key(bibEntry.getType()), new Key(citeKey)); - - // Not every field is already generated into latex free fields - for (String key : bibEntry.getFieldMap().keySet()) { - Optional latexFreeField = bibEntry.getLatexFreeField(key); - latexFreeField.ifPresent(value -> bibTeXEntry.addField(new Key(key), new DigitStringValue(value))); - } - return BIBTEX_CONVERTER.toItemData(bibTeXEntry); - } - }