use a open addressing map to store folding paths by arnaudroger · Pull Request #36 · google/re2j

arnaudroger · 2017-10-10T14:33:35Z

change simplefold logic to use an open addressing map with phi mix hashing.
- quick path check of value if < ORBIT_MIN_VALUE
- if there is a folding array just iterate over avoiding continuous lookup
- if not just got through toUpper toLower path
reduce binary search to exclude the element searched through the linear searched

This optimisation came from profiling my app with real payload, the simpleFold was taking around 15% cpu and is now negligible.

I added a few test to verify that the logic is still working as expected.

alandonovan · 2017-10-10T17:01:07Z

java/com/google/re2j/Inst.java

+      int[] folds = Unicode.optimizedFoldOrbit(r0);
+
+      if (folds == null) {
+        return (r == Unicode.optimizedFoldNonOrbit(r0));


Have you benchmarked this against an implementation based on Character.toLowerCase(r) == r? Ideally with a few changes like that, we could throw away all the Unicode tables and use the ones built into java.lang.

Be sure to use the int (code point) not char (UTF-16 code) variants of those functions.

I guess you are talking about the optimizedFoldNonOrbit? no I have not, was trying to be the least impactful as possible :) will have a look. From what I understand the Orbit exception will still need to be there ?

Yes, the orbit case will still need special handling. I suggest you do this experiment as a separate change.

alandonovan · 2017-10-10T17:27:50Z

java/com/google/re2j/Unicode.java

+  static int[] optimizedFoldOrbit(int r) {
+    if (r >= ORBIT_MIN_VALUE) {
+      return FOLD_MAP.get(r);
+    } else return null;


Use {...} around every block, or outdent the else block:

if (r < ORBIT_MIN_VALUE) { return null; } return FOLD_MAP.get(r);

alandonovan · 2017-10-10T17:30:32Z

java/com/google/re2j/Unicode.java

+  private static final int ORBIT_MIN_VALUE = UnicodeTables.CASE_ORBIT[0][0];
+
+  static {
+    for(int i = 0; i < UnicodeTables.CASE_ORBIT.length; i++) {


for (int[][] orbit : UnicodeTables.CASE_ORBIT)

alandonovan · 2017-10-10T17:33:01Z

java/com/google/re2j/Unicode.java

+      int[] folds = new int[0];
+      int r1 = r0;
+      while((r1 = simpleFold(r1)) != r0) {
+        folds = Arrays.copyOf(folds, folds.length + 1);


Isn't an orbit necessarily of length at least 3? I realize they are small and few, but it seems perverse to copy the entire array each time we add an element. Use a temporary large enough for the longest orbit (MAX_DISTANCE?) and copy it once before each call to FOLD_MAP.put.

alandonovan · 2017-10-10T17:35:27Z

java/com/google/re2j/Unicode.java

+  }
+
+  /*
+    associate a rune to it's unfolded case equivalent


"A FoldMap maps a rune to an array of runes that are equivalent to it in a case-insensitive pattern."

alandonovan · 2017-10-10T17:38:14Z

java/com/google/re2j/Unicode.java

+    return toUpper(r);
+  }
+
+  private static final FoldMap FOLD_MAP = new FoldMap(UnicodeTables.CASE_ORBIT.length * 4);


Whence 4? Is it a load factor based on MAX_DISTANCE?

I don't think this should be a parameter. FoldMap is instantiated exactly once; specialize it.

alandonovan · 2017-10-10T17:47:36Z

java/com/google/re2j/Inst.java

 */
 class Inst {

+  private static final int RUNES_LINER_SEARCH_SIZE = 10;


LINEAR?

Move this constant into the sole function that uses it.

alandonovan · 2017-10-10T17:51:37Z

java/com/google/re2j/Inst.java

-    // Otherwise binary search.
-    for (int lo = 0, hi = runes.length / 2; lo < hi; ) {
+    // Otherwise binary search on rest of the array
+    for (int lo = 0, hi = (runes.length - RUNES_LINER_SEARCH_SIZE) / 2; lo < hi; ) {


I'm not sure it's worth the fuss to subtract the portion initially scanned. It's a logarithmic algorithm and this is not the common case. I'm sure it's fast enough to search the complete array. Did you find evidence to the contrary?

no, will remove it

alandonovan · 2017-10-10T17:53:51Z

java/com/google/re2j/Inst.java


-    // Otherwise binary search.
-    for (int lo = 0, hi = runes.length / 2; lo < hi; ) {
+    // Otherwise binary search on rest of the array


While you're here, please document:

// Invariant: lo, hi, m are even.

alandonovan · 2017-10-10T17:59:08Z

java/com/google/re2j/Unicode.java

+    associate a rune to it's unfolded case equivalent
+   */
+  private static class FoldMap {
+    private static final int MAX_DISTANCE = 4;


Document:

// number of closed-hashing probes before giving up

alandonovan · 2017-10-10T18:01:52Z

java/com/google/re2j/Unicode.java

+    private final int mask;
+    private final int maxDistance;
+
+    FoldMap(int size) {


FoldMap serves exactly one purpose. It doesn't need a parameter, and it doesn't need to be separated from the logic that instantiates and populates it.

arnaudroger · 2017-10-11T15:10:02Z

I did check the Character.toLower/toUpper it does not affect the results, though I change the isUpper/isLower to use the Character one and it broke some test so I rolled it back.

use a open addressing map to store folding paths

294e8a7

googlebot added the cla: yes label Oct 10, 2017

alandonovan reviewed Oct 10, 2017

View reviewed changes

fix comments and use Character for toLowerCase

cecfbea

Merge branch 'master' into simplefold_opt

aaa59ef

martin-a mentioned this pull request Jan 4, 2018

What about a new release? #45

Closed

Comments

Conversation

arnaudroger commented Oct 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnaudroger commented Oct 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants