opendev/gerrit
Code Issues Proposed changes

Optimize edit handling in VisibleRefFilter

Browse Source 
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
This commit is contained in:
Shawn Pearce
2016-06-20 16:44:55 -07:00
parent ffe9a9f0cd
commit 98f448cd3e
3 changed files with 49 additions and 79 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java
View File

@@ -164,29 +164,17 @@ public class RefNames {
* @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;

24
gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java
View File

@@ -80,30 +80,6 @@ public class RefNamesTest {
assertThat(RefNames.isRefsEdit("refs/heads/master")).isFalse();
}
@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);

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

@@ -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 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
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 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
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 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
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 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
}
}
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);
}