Store LaTeX-free fields in BibEntry #2102

lenhard · 2016-09-30T09:47:14Z

Based on the discussion in #1993 and the work of @bartsch-dev in #2091, this PR aims to improve the speed of search by storing LaTeX-free versions of field values in a cache. It aims at a balance between better performance and an acceptable memory footprint. Basically, all ideas from #1993 are implemented: a cached store of LaTeX-free fields which is computed on demand, String internalization, and regex performance improvements in LatexToUnicode.

Here are measurements with the new branch:

Benchmark                             Mode  Cnt         Score        Error  Unit
Benchmarks.latexToUnicodeConversion  thrpt   20    119823.078   2063.425  ops/s
Benchmarks.parallelSearch            thrpt   20       735.890    113.685  ops/s
Benchmarks.search                    thrpt   20       397.807     28.557  ops/s

The memory footprint of a database with 6500 entries is ~ 912 MB on my machine

And on current master:

Benchmark                             Mode  Cnt         Score        Error  Unit
Benchmarks.latexToUnicodeConversion  thrpt   20     94367.718   2570.784  ops/s
Benchmarks.parallelSearch            thrpt   20        58.204       3.285  ops/s
Benchmarks.search                    thrpt   20        30.531       0.952  ops/s

The memory footprint of the same 6500 entry database is ~ 888 MB.

All in all, this looks like a hefty performance improvement at little memory cost.

Change in CHANGELOG.md described
Manually tested changed features in running JabRef

stefan-kolb · 2016-09-30T10:02:33Z

LGTM. Although the converter is probably not perfect and we might run into new problems caused by that fact.

lenhard · 2016-09-30T10:11:25Z

The functionality in the converter has not changed, it is just a rewording of String.replace calls into regular expressions that are compiled once instead of every time. Thus, there should be no new problems that weren't broken before anyway. You can never be sure, though...

I still think the converter should be written from scratch, but that should really be a separate PR.

matthiasgeiger

Some nitpicking 😉 and a question:

Why does the "GrammarBasedSearchRule" not use this?

matthiasgeiger · 2016-09-30T10:04:54Z

src/main/java/net/sf/jabref/model/entry/BibEntry.java

@@ -57,6 +58,16 @@
     */
    private final Map<String, Set<String>> fieldsAsWords = new HashMap<>();

+    /*
+     * Map that stores latex free versions of fields. Is populated as a cash


Ok, will change

matthiasgeiger · 2016-09-30T10:05:05Z

src/main/java/net/sf/jabref/model/entry/BibEntry.java

+    private final Map<String, String> latexFreeFields = new ConcurrentHashMap<>();
+
+    /*
+     * Used to cleanse field values for internal LaTeX-free storage


What is wrong with cleanse? My dictionary says this is a proper term :-)

matthiasgeiger · 2016-09-30T10:05:34Z

src/main/java/net/sf/jabref/model/entry/BibEntry.java

@@ -180,7 +191,7 @@ public String getId() {

    /**
     * Sets the cite key AKA citation key AKA BibTeX key.
-     *
+     * <p>


matthiasgeiger · 2016-09-30T10:05:41Z

src/main/java/net/sf/jabref/model/entry/BibEntry.java

@@ -191,7 +202,7 @@ public void setCiteKey(String newCiteKey) {

    /**
     * Returns the cite key AKA citation key AKA BibTeX key, or null if it is not set.
-     *
+     * <p>


Sure, weird autoformatting...

lenhard · 2016-09-30T10:29:03Z

Regarding GrammarBasedSearchRule: I am not 100% sure, but it looks like it on the one hand builds upon ContainBasedSearchRule. But it also has some self-searching code, which I modified to use the LaTeX-free version. This was probably a bug.

koppor · 2016-09-30T10:31:30Z

Implementation alternatives for the LaTeX-to-Unicode conversions are discussed at #1252

tobiasdiez

I join the nitpicking party. LGTM!

tobiasdiez · 2016-09-30T10:27:54Z

src/main/java/net/sf/jabref/model/entry/BibEntry.java

@@ -57,6 +58,16 @@
     */
    private final Map<String, Set<String>> fieldsAsWords = new HashMap<>();

+    /*
+     * Cache that stores latex free versions of fields. Is populated as a cash


Second sentence doesn't make sense?!

tobiasdiez · 2016-09-30T10:31:45Z

src/main/java/net/sf/jabref/model/entry/BibEntry.java

@@ -479,6 +491,8 @@ public void setField(Map<String, String> fields) {

        fields.remove(fieldName);
        fieldsAsWords.remove(fieldName);


maybe also move fieldsAsWords.remove(fieldName); to the invalidate cache method?

Good point! Done!

chriba · 2016-09-30T10:34:21Z

src/main/java/net/sf/jabref/model/entry/BibEntry.java

@@ -57,6 +58,16 @@
     */
    private final Map<String, Set<String>> fieldsAsWords = new HashMap<>();

+    /*


/**instead of /*

Good point. It's strange that Intellij does not distinguish between JavaDoc and Inline documentation in its color.

With the Darcula theme it does.

chriba · 2016-09-30T10:34:32Z

src/main/java/net/sf/jabref/model/entry/BibEntry.java

+
+    /*
+     * Used to cleanse field values for internal LaTeX-free storage
+     */


/**instead of /*

lenhard · 2016-10-03T07:37:42Z

As a few people have looked at the code and only criticized minor aspects (which I fixed), I'll merge this now. The codacy build seems to be broken somehow, but then it is also not that critical.

* Add VM args for string deduplication * Add cache of LaTeX-free fields * Intern Strings for fields * Exchange calls of replaceAll with compiled regular expressions * Transform string.replace() with compiled regular expressions * Performance improvements in RegexBasedSearchRule * Add changelog entry * Javadoc fixes * Use LaTeX-free fields in GrammarBasedSearchRule * More JavaDoc cleanup * You can always improve JavaDoc * Bundle cache invalidation

koppor · 2017-01-29T20:06:41Z

jabref.install4j

@@ -750,4 +750,9 @@ return true;</string>
  </mediaSets>
  <buildIds buildAll="true" />
  <buildOptions verbose="false" faster="false" disableSigning="false" disableJreBundling="false" debug="false" />
+  <jvmArguments>


Note that if these settings go missing, it may caused by a plain save of the config using the Install4J GUI. I'm trying to add it as "VM options file" at 53ffd02, which seems to work.

lenhard added 7 commits September 29, 2016 12:57

Add VM args for string deduplication

7a991b5

Add cache of LaTeX-free fields

040ab69

Intern Strings for fields

ef5f0e5

Exchange calls of replaceAll with compiled regular expressions

93e665e

Transform string.replace() with compiled regular expressions

f0d2722

Performance improvements in RegexBasedSearchRule

23bdf0c

Add changelog entry

ff35c63

lenhard added search bug Confirmed bugs or reports that are very likely to be bugs status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Sep 30, 2016

lenhard mentioned this pull request Sep 30, 2016

[WIP] Speed up search #2091

Closed

stefan-kolb approved these changes Sep 30, 2016

View reviewed changes

matthiasgeiger reviewed Sep 30, 2016

View reviewed changes

lenhard added 2 commits September 30, 2016 12:22

Javadoc fixes

85af153

Use LaTeX-free fields in GrammarBasedSearchRule

85f5af1

More JavaDoc cleanup

cfb3384

tobiasdiez approved these changes Sep 30, 2016

View reviewed changes

chriba reviewed Sep 30, 2016

View reviewed changes

lenhard added 2 commits September 30, 2016 13:03

You can always improve JavaDoc

9e39312

Bundle cache invalidation

0ee1d17

lenhard merged commit 2e5dd7e into master Oct 3, 2016

This was referenced Oct 3, 2016

Performance issue with new search #1993

Closed

Store LaTeX-free version inside each BibTeX entry #518

Closed

tobiasdiez deleted the latex-free-fields branch October 3, 2016 08:43

matthiasgeiger referenced this pull request Dec 19, 2016

Remove JVM arguments

fa16bc6

koppor reviewed Jan 29, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store LaTeX-free fields in BibEntry #2102

Store LaTeX-free fields in BibEntry #2102

lenhard commented Sep 30, 2016 •

edited

Loading

stefan-kolb commented Sep 30, 2016

lenhard commented Sep 30, 2016 •

edited

Loading

matthiasgeiger left a comment

matthiasgeiger Sep 30, 2016

lenhard Sep 30, 2016

matthiasgeiger Sep 30, 2016

lenhard Sep 30, 2016

matthiasgeiger Sep 30, 2016

matthiasgeiger Sep 30, 2016

lenhard Sep 30, 2016

lenhard commented Sep 30, 2016

koppor commented Sep 30, 2016

tobiasdiez left a comment

tobiasdiez Sep 30, 2016

lenhard Sep 30, 2016

tobiasdiez Sep 30, 2016

lenhard Sep 30, 2016

chriba Sep 30, 2016

lenhard Sep 30, 2016

chriba Sep 30, 2016

chriba Sep 30, 2016

lenhard commented Oct 3, 2016

koppor Jan 29, 2017

		@@ -479,6 +491,8 @@ public void setField(Map<String, String> fields) {

		fields.remove(fieldName);
		fieldsAsWords.remove(fieldName);

Store LaTeX-free fields in BibEntry #2102

Store LaTeX-free fields in BibEntry #2102

Conversation

lenhard commented Sep 30, 2016 • edited Loading

stefan-kolb commented Sep 30, 2016

lenhard commented Sep 30, 2016 • edited Loading

matthiasgeiger 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

Choose a reason for hiding this comment

lenhard commented Sep 30, 2016

koppor commented Sep 30, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lenhard commented Oct 3, 2016

Choose a reason for hiding this comment

lenhard commented Sep 30, 2016 •

edited

Loading

lenhard commented Sep 30, 2016 •

edited

Loading