Optimize edit handling in VisibleRefFilter
Checking each ref through the RefNames.isRefsEditOf is still several
percent of the total garbage created by a busy Gerrit server filtering
references for clients... most of which don't have pending edits.
Rewrite this handling to reduce the StringBuilder allocations so that
the user prefix is built once and reused over the refs.
Fix the TODO about filtering edits to visible changes; we have the
Set<Change.Id> readily available and can parse the Change.Id out of
the edit ref name to test in the set once we know the ref is owned by
the current user.
Rework much of the VisibleRefFilter logic to simplify the
!showMetadata case to short-circuit through the loop more quickly.
This speeds up advertisment generation for pushes by a very small
amount, but also simplifies every single branch so its worth the code
churn either way.
Change-Id: I15d97ec7783e8bd6c7a042a020cdcf4352273cab
diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java
index 935f8fc..a1278fa 100644
--- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java
+++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java
@@ -164,29 +164,17 @@
* @return reference prefix for this change edit
*/
public static String refsEditPrefix(Account.Id accountId, Change.Id changeId) {
- return new StringBuilder(refsUsers(accountId))
- .append('/')
- .append(EDIT_PREFIX)
- .append(changeId.get())
- .append('/')
- .toString();
+ return refsEditPrefix(accountId) + changeId.get() + '/';
+ }
+
+ public static String refsEditPrefix(Account.Id accountId) {
+ return refsUsers(accountId) + '/' + EDIT_PREFIX;
}
public static boolean isRefsEdit(String ref) {
return ref.startsWith(REFS_USERS) && ref.contains(EDIT_PREFIX);
}
- public static boolean isRefsEditOf(String ref, Account.Id accountId) {
- if (accountId == null) {
- return false;
- }
- String prefix = new StringBuilder(refsUsers(accountId))
- .append('/')
- .append(EDIT_PREFIX)
- .toString();
- return ref.startsWith(prefix);
- }
-
static Integer parseShardedRefPart(String name) {
if (name == null) {
return null;
diff --git a/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java b/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java
index af1c074..40d8b53 100644
--- a/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java
+++ b/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java
@@ -81,30 +81,6 @@
}
@Test
- public void isRefsEditOf() throws Exception {
- assertThat(
- RefNames.isRefsEditOf("refs/users/23/1011123/edit-67473/42", accountId))
- .isTrue();
-
- // other user
- assertThat(
- RefNames.isRefsEditOf("refs/users/20/1078620/edit-67473/42", accountId))
- .isFalse();
-
- // user ref, but no edit ref
- assertThat(RefNames.isRefsEditOf("refs/users/23/1011123", accountId))
- .isFalse();
-
- // bad user shard
- assertThat(
- RefNames.isRefsEditOf("refs/users/77/1011123/edit-67473/42", accountId))
- .isFalse();
-
- // other ref
- assertThat(RefNames.isRefsEditOf("refs/heads/master", accountId)).isFalse();
- }
-
- @Test
public void testParseShardedRefsPart() throws Exception {
assertThat(parseShardedRefPart("01/1")).isEqualTo(1);
assertThat(parseShardedRefPart("01/1-drafts")).isEqualTo(1);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
index 6bfd151..f142649 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
@@ -14,6 +14,9 @@
package com.google.gerrit.server.git;
+import static com.google.gerrit.reviewdb.client.RefNames.REFS_CACHE_AUTOMERGE;
+import static com.google.gerrit.reviewdb.client.RefNames.REFS_CHANGES;
+
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Maps;
import com.google.gerrit.common.Nullable;
@@ -60,6 +63,7 @@
private final ProjectControl projectCtl;
private final ReviewDb reviewDb;
private final boolean showMetadata;
+ private String userEditPrefix;
private Set<Change.Id> visibleChanges;
public VisibleRefFilter(
@@ -101,66 +105,54 @@
return r;
}
- Account.Id currAccountId;
- boolean canViewMetadata;
+ Account.Id userId;
+ boolean viewMetadata;
if (projectCtl.getUser().isIdentifiedUser()) {
IdentifiedUser user = projectCtl.getUser().asIdentifiedUser();
- currAccountId = user.getAccountId();
- canViewMetadata = user.getCapabilities().canAccessDatabase();
+ userId = user.getAccountId();
+ viewMetadata = user.getCapabilities().canAccessDatabase();
+ userEditPrefix = RefNames.refsEditPrefix(userId);
} else {
- currAccountId = null;
- canViewMetadata = false;
+ userId = null;
+ viewMetadata = false;
}
Map<String, Ref> result = new HashMap<>();
List<Ref> deferredTags = new ArrayList<>();
for (Ref ref : refs.values()) {
+ String name = ref.getName();
Change.Id changeId;
Account.Id accountId;
- if (ref.getName().startsWith(RefNames.REFS_CACHE_AUTOMERGE)) {
+ if (name.startsWith(REFS_CACHE_AUTOMERGE)
+ || (!showMetadata && isMetadata(name))) {
continue;
- } else if (showMetadata
- && (RefNames.isRefsEditOf(ref.getLeaf().getName(), currAccountId)
- || (RefNames.isRefsEdit(ref.getLeaf().getName())
- && canViewMetadata))) {
- // Change edit reference related is visible to the account that owns the
- // change edit.
- //
- // TODO(dborowitz): Verify if change is visible (to exclude edits on
- // changes that the user has lost access to).
- result.put(ref.getName(), ref);
-
- } else if ((changeId = Change.Id.fromRef(ref.getName())) != null) {
- // Reference related to a change is visible if the change is visible.
- if (showMetadata && (canViewMetadata || visible(changeId))) {
- result.put(ref.getName(), ref);
+ } else if (RefNames.isRefsEdit(name)) {
+ // Edits are visible only to the owning user, if change is visible.
+ if (viewMetadata || visibleEdit(name)) {
+ result.put(name, ref);
}
-
+ } else if ((changeId = Change.Id.fromRef(name)) != null) {
+ // Change ref is visible only if the change is visible.
+ if (viewMetadata || visible(changeId)) {
+ result.put(name, ref);
+ }
+ } else if ((accountId = Account.Id.fromRef(name)) != null) {
+ // Account ref is visible only to corresponding account.
+ if (viewMetadata || (accountId.equals(userId)
+ && projectCtl.controlForRef(name).isVisible())) {
+ result.put(name, ref);
+ }
} else if (isTag(ref)) {
// If its a tag, consider it later.
- //
if (ref.getObjectId() != null) {
deferredTags.add(ref);
}
-
} else if (projectCtl.controlForRef(ref.getLeaf().getName()).isVisible()) {
// Use the leaf to lookup the control data. If the reference is
// symbolic we want the control around the final target. If its
// not symbolic then getLeaf() is a no-op returning ref itself.
- //
-
- if ((accountId =
- Account.Id.fromRef(ref.getLeaf().getName())) != null) {
- // Reference related to an account is visible only for the current
- // account.
- if (showMetadata
- && (canViewMetadata || accountId.equals(currAccountId))) {
- result.put(ref.getName(), ref);
- }
- } else {
- result.put(ref.getName(), ref);
- }
+ result.put(name, ref);
}
}
@@ -211,6 +203,16 @@
return visibleChanges.contains(changeId);
}
+ private boolean visibleEdit(String name) {
+ if (userEditPrefix != null && name.startsWith(userEditPrefix)) {
+ Change.Id id = Change.Id.fromEditRefPart(name);
+ if (id != null) {
+ return visible(id);
+ }
+ }
+ return false;
+ }
+
private Set<Change.Id> visibleChangesBySearch() {
Project project = projectCtl.getProject();
try {
@@ -247,6 +249,10 @@
}
}
+ private static boolean isMetadata(String name) {
+ return name.startsWith(REFS_CHANGES) || RefNames.isRefsEdit(name);
+ }
+
private static boolean isTag(Ref ref) {
return ref.getLeaf().getName().startsWith(Constants.R_TAGS);
}
