Improve PatchSet.Id.fromRef performance and avoid double-parsing

If5a39210 optimized PatchSet.isRef() to improve the performance of
VisibleRefFilter during ref advertisement on large repositories.
However, typical large Gerrit repositories are dominated by patch set
refs, in which case the relatively slower PatchSet.Id.fromRef() would
be called after each call to isRef().

Teach fromRef() to return null for invalid refs, and switch to a
character-by-character implementation like the former isRef()
implementation, which also avoids any allocations until returning.

Add small tests for various ref formats; the new parser is somewhat
more strict.

On a test project with 50,000 changes (50,004 refs), this reduces
average ls-remote time on my MacBook Air by about 30%, 57653us to
40480us (3 warmup runs then average of 10).

Leave isRef(), now implemented as "fromRef() != null". This may be
marginally less efficient than before, since it involves allocating a
PatchSet.Id. This makes the code more maintainable; when writing
tests, I found several inconsistencies between the old
isRef()/fromRef() implementations. Rather than fix these, I stuck
with one implementation. In any case, remaining isRef() callers
are not called on every ref in the repo in a tight loop, so the
performance impact should be minimal.

Change-Id: I9dfb074a7c2312512960fd070aef323c33f80dbd

gerrit-reviewdb/BUCK

@@ -1,4 +1,5 @@
 SRC = 'src/main/java/com/google/gerrit/reviewdb/'
+TESTS = 'src/test/java/com/google/gerrit/reviewdb/'
 
 gwt_module(
   name = 'client',
@@ -22,3 +23,15 @@ java_library(
   ],
   visibility = ['PUBLIC'],
 )
+
+java_test(
+  name = 'client_tests',
+  srcs = glob([TESTS + 'client/**/*.java']),
+  deps = [
+    ':client',
+    '//lib:gwtorm',
+    '//lib:junit',
+  ],
+  source_under_test = [':client'],
+  visibility = ['//tools/eclipse:classpath'],
+)

gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchSet.java

@@ -24,28 +24,8 @@ import java.sql.Timestamp;
 /** A single revision of a {@link Change}. */
 public final class PatchSet {
   /** Is the reference name a change reference? */
-  public static boolean isRef(final String name) {
+  public static boolean isRef(String name) {
-    if (name == null || !name.startsWith(REFS_CHANGES)) {
+    return Id.fromRef(name) != null;
-      return false;
-    }
-    boolean accepted = false;
-    int numsFound = 0;
-    for (int i = name.length() - 1; i >= REFS_CHANGES.length() - 1; i--) {
-      char c = name.charAt(i);
-      if (c >= '0' && c <= '9') {
-        accepted = (c != '0');
-      } else if (c == '/') {
-        if (accepted) {
-          if (++numsFound == 2) {
-            return true;
-          }
-          accepted = false;
-        }
-      } else {
-        return false;
-      }
-    }
-    return false;
   }
 
   public static class Id extends IntKey<Change.Id> {
@@ -106,16 +86,63 @@ public final class PatchSet {
 
     /** Parse a PatchSet.Id from a {@link PatchSet#getRefName()} result. */
     public static Id fromRef(String name) {
-      if (!name.startsWith(REFS_CHANGES)) {
+      if (name == null || !name.startsWith(REFS_CHANGES)) {
-        throw new IllegalArgumentException("Not a PatchSet.Id: " + name);
+        return null;
       }
-      final String[] parts = name.substring(REFS_CHANGES.length()).split("/");
+
-      final int n = parts.length;
+      // Last 2 digits.
-      if (n < 2) {
+      int ls = REFS_CHANGES.length();
-        throw new IllegalArgumentException("Not a PatchSet.Id: " + name);
+      int le;
+      for (le = ls; le < name.length() && name.charAt(le) != '/'; le++) {
+        if (name.charAt(le) < '0' || name.charAt(le) > '9') {
+          return null;
+        }
       }
-      final int changeId = Integer.parseInt(parts[n - 2]);
+      if (le - ls != 2) {
-      final int patchSetId = Integer.parseInt(parts[n - 1]);
+        return null;
+      }
+
+      // Change ID.
+      int cs = le + 1;
+      if (cs >= name.length() || name.charAt(cs) == '0') {
+        return null;
+      }
+      int ce;
+      for (ce = cs; ce < name.length() && name.charAt(ce) != '/'; ce++) {
+        if (name.charAt(ce) < '0' || name.charAt(ce) > '9') {
+          return null;
+        }
+      }
+      switch (ce - cs) {
+        case 0:
+          return null;
+        case 1:
+          if (name.charAt(ls) != '0'
+              || name.charAt(ls + 1) != name.charAt(cs)) {
+            return null;
+          }
+          break;
+        default:
+          if (name.charAt(ls) != name.charAt(ce - 2)
+              || name.charAt(ls + 1) != name.charAt(ce - 1)) {
+            return null;
+          }
+          break;
+      }
+
+      // Patch set ID.
+      int ps = ce + 1;
+      if (ps >= name.length() || name.charAt(ps) == '0') {
+        return null;
+      }
+      for (int i = ps; i < name.length(); i++) {
+        if (name.charAt(i) < '0' || name.charAt(i) > '9') {
+          return null;
+        }
+      }
+
+      int changeId = Integer.parseInt(name.substring(cs, ce));
+      int patchSetId = Integer.parseInt(name.substring(ps));
       return new PatchSet.Id(new Change.Id(changeId), patchSetId);
     }
   }

gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/PatchSetTest.java

@@ -0,0 +1,69 @@
+// Copyright (C) 2014 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.reviewdb.client;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+import org.junit.Test;
+
+public class PatchSetTest {
+  @Test
+  public void parseRefNames() {
+    assertRef(1, 1, "refs/changes/01/1/1");
+    assertRef(1234, 56, "refs/changes/34/1234/56");
+
+    // Not even close.
+    assertNotRef(null);
+    assertNotRef("");
+    assertNotRef("01/1/1");
+    assertNotRef("HEAD");
+    assertNotRef("refs/tags/v1");
+
+    // Invalid characters.
+    assertNotRef("refs/changes/0x/1/1");
+    assertNotRef("refs/changes/01/x/1");
+    assertNotRef("refs/changes/01/1/x");
+
+    // Truncations.
+    assertNotRef("refs/changes/");
+    assertNotRef("refs/changes/1");
+    assertNotRef("refs/changes/01");
+    assertNotRef("refs/changes/01/");
+    assertNotRef("refs/changes/01/1/");
+    assertNotRef("refs/changes/01/1/1/");
+    assertNotRef("refs/changes/01//1/1");
+
+    // Leading zeroes.
+    assertNotRef("refs/changes/01/01/1");
+    assertNotRef("refs/changes/01/1/01");
+
+    // Mismatched last 2 digits.
+    assertNotRef("refs/changes/35/1234/56");
+  }
+
+  private static void assertRef(int changeId, int psId, String refName) {
+    assertTrue(PatchSet.isRef(refName));
+    assertEquals(new PatchSet.Id(new Change.Id(changeId), psId),
+        PatchSet.Id.fromRef(refName));
+  }
+
+  private static void assertNotRef(String refName) {
+    assertFalse(PatchSet.isRef(refName));
+    assertNull(PatchSet.Id.fromRef(refName));
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java

@@ -2131,8 +2131,11 @@ public class ReceiveCommits {
           allRefs.size() / estRefsPerChange,
           estRefsPerChange);
       for (Ref ref : allRefs.values()) {
-        if (ref.getObjectId() != null && PatchSet.isRef(ref.getName())) {
+        if (ref.getObjectId() != null) {
-          refsByChange.put(Change.Id.fromRef(ref.getName()), ref);
+          PatchSet.Id psId = PatchSet.Id.fromRef(ref.getName());
+          if (psId != null) {
+            refsByChange.put(psId.getParentKey(), ref);
+          }
         }
       }
     }

gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java

@@ -82,12 +82,13 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
     final List<Ref> deferredTags = new ArrayList<>();
 
     for (Ref ref : refs.values()) {
+      PatchSet.Id psId;
       if (ref.getName().startsWith(RefNames.REFS_CACHE_AUTOMERGE)) {
         continue;
-      } else if (PatchSet.isRef(ref.getName())) {
+      } else if ((psId = PatchSet.Id.fromRef(ref.getName())) != null) {
         // Reference to a patch set is visible if the change is visible.
         //
-        if (showChanges && visibleChanges.contains(Change.Id.fromRef(ref.getName()))) {
+        if (showChanges && visibleChanges.contains(psId.getParentKey())) {
           result.put(ref.getName(), ref);
         }
 

tools/eclipse/BUCK

@@ -11,6 +11,7 @@ java_library(
     '//gerrit-main:main_lib',
     '//gerrit-patch-jgit:jgit_patch_tests',
     '//gerrit-plugin-gwtui:gwtui-api-lib',
+    '//gerrit-reviewdb:client_tests',
     '//gerrit-server:server',
     '//gerrit-server:server_tests',
     '//lib/asciidoctor:asciidoc_lib',