Skip to content

Commit

Permalink
Fix preview performance (#3533)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
Patrick Scheibe authored and tobiasdiez committed Dec 19, 2017
1 parent 07652a9 commit 6d2ec48
Show file tree
Hide file tree
Showing 7 changed files with 196 additions and 79 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
7 changes: 4 additions & 3 deletions src/main/java/org/jabref/gui/preftabs/PreviewPrefsTab.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
Expand All @@ -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");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,17 @@ public CitationStyleToClipboardWorker(BasePanel basePanel, CitationStyleOutputFo

@Override
protected List<String> 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);
Expand All @@ -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;
Expand Down Expand Up @@ -181,5 +189,4 @@ protected static HtmlTransferable processHtml(List<String> citations) {

return new HtmlTransferable(result);
}

}
116 changes: 116 additions & 0 deletions src/main/java/org/jabref/logic/citationstyle/CSLAdapter.java
Original file line number Diff line number Diff line change
@@ -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<String> makeBibliography(List<BibEntry> 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<BibEntry> data) {
List<CSLItemData> 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<String> latexFreeField = bibEntry.getLatexFreeField(key);
latexFreeField.ifPresent(value -> bibTeXEntry.addField(new Key(key), new DigitStringValue(value)));
}
return BIBTEX_CONVERTER.toItemData(bibTeXEntry);
}
}
}
83 changes: 51 additions & 32 deletions src/main/java/org/jabref/logic/citationstyle/CitationStyle.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<CitationStyle> 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);
}
Expand All @@ -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();
Expand Down Expand Up @@ -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<CitationStyle> discoverCitationStyles() {
if (!STYLES.isEmpty()) {
return STYLES;
}
try {
final List<CitationStyle> citationStyles = new ArrayList<>();
String path = CitationStyle.class.getProtectionDomain().getCodeSource().getLocation().toURI().getPath();

try (JarFile file = new JarFile(path)) {
Enumeration<JarEntry> 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<String> 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<Path> 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();
}
Expand All @@ -164,8 +184,8 @@ public String getSource() {
return source;
}

public String getFilepath() {
return filepath;
public String getFilePath() {
return filePath;
}

@Override
Expand All @@ -187,8 +207,7 @@ public boolean equals(Object o) {
}

@Override
public int hashCode()
{
public int hashCode() {
return Objects.hash(source);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public class CitationStyleCache {
private static final int CACHE_SIZE = 1024;

private CitationStyle citationStyle;
private LoadingCache<BibEntry, String> citationStyleCache;
private final LoadingCache<BibEntry, String> citationStyleCache;


public CitationStyleCache(BibDatabaseContext bibDatabaseContext) {
Expand All @@ -33,7 +33,7 @@ public CitationStyleCache(BibDatabaseContext bibDatabaseContext, CitationStyle c
this.citationStyle = citationStyle;
citationStyleCache = CacheBuilder.newBuilder().maximumSize(CACHE_SIZE).build(new CacheLoader<BibEntry, String>() {
@Override
public String load(BibEntry entry) throws Exception {
public String load(BibEntry entry) {
return CitationStyleGenerator.generateCitation(entry, getCitationStyle().getSource(), CitationStyleOutputFormat.HTML);
}
});
Expand Down
Loading

0 comments on commit 6d2ec48

Please sign in to comment.