opendev/gerrit
Code Issues Proposed changes

Avoid lazy loading data during change query

Browse Source 
Gerrit can thrash the JGit block cache trying to read NoteDb while
formatting search results for user dashboards. This seems to happen
when the index is missing fields the JSON formatter is reading from
ChangeData.

Avoid thrashing by configuring the ChangeData accessors to return
empty values instead of computing them. This is used only for bulk
change search (/changes/ REST API). Detail requests will still run
the lazy load logic for any requested fields.

Change-Id: Ied6b2b9e1b154f41db2fe5501b8e245098917a46
This commit is contained in:
Shawn Pearce
2016-09-16 20:24:44 -07:00
committed by Edwin Kempin
parent 1f9fe25821
commit d465e508da
3 changed files with 94 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

34
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
View File

@@ -45,6 +45,7 @@ import com.google.common.collect.FluentIterable;
import com.google.common.collect.HashBasedTable;
import com.google.common.collect.HashMultimap;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.LinkedHashMultimap;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
@@ -141,6 +142,9 @@ public class ChangeJson {
public static final Set<ListChangesOption> NO_OPTIONS =
Collections.emptySet();
public static final ImmutableSet<ListChangesOption> REQUIRE_LAZY_LOAD =
ImmutableSet.of(ALL_REVISIONS, MESSAGES);
public interface Factory {
ChangeJson create(Set<ListChangesOption> options);
}
@@ -168,6 +172,7 @@ public class ChangeJson {
private final ChangeResource.Factory changeResourceFactory;
private final ChangeKindCache changeKindCache;
private boolean lazyLoad = true;
private AccountLoader accountLoader;
private boolean includeSubmittable;
private Map<Change.Id, List<SubmitRecord>> submitRecords;
@@ -228,6 +233,11 @@ public class ChangeJson {
return this;
}
public ChangeJson lazyLoad(boolean load) {
lazyLoad = load;
return this;
}
public ChangeJson fix(FixInput fix) {
this.fix = fix;
return this;
@@ -322,16 +332,22 @@ public class ChangeJson {
}
private void ensureLoaded(Iterable<ChangeData> all) throws OrmException {
ChangeData.ensureChangeLoaded(all);
if (has(ALL_REVISIONS)) {
ChangeData.ensureAllPatchSetsLoaded(all);
} else if (has(CURRENT_REVISION) || has(MESSAGES)) {
ChangeData.ensureCurrentPatchSetLoaded(all);
if (lazyLoad) {
ChangeData.ensureChangeLoaded(all);
if (has(ALL_REVISIONS)) {
ChangeData.ensureAllPatchSetsLoaded(all);
} else if (has(CURRENT_REVISION) || has(MESSAGES)) {
ChangeData.ensureCurrentPatchSetLoaded(all);
}
if (has(REVIEWED) && userProvider.get().isIdentifiedUser()) {
ChangeData.ensureReviewedByLoadedForOpenChanges(all);
}
ChangeData.ensureCurrentApprovalsLoaded(all);
} else {
for (ChangeData cd : all) {
cd.setLazyLoad(false);
}
}
if (has(REVIEWED) && userProvider.get().isIdentifiedUser()) {
ChangeData.ensureReviewedByLoadedForOpenChanges(all);
}
ChangeData.ensureCurrentApprovalsLoaded(all);
}
private boolean has(ListChangesOption option) {

66
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
View File

@@ -24,6 +24,7 @@ import com.google.common.base.Optional;
import com.google.common.base.Predicate;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.Iterables;
import com.google.common.collect.ListMultimap;
@@ -308,6 +309,7 @@ public class ChangeData {
return cd;
}
private boolean lazyLoad = true;
private final ReviewDb db;
private final GitRepositoryManager repoManager;
private final ChangeControl.GenericFactory changeControlFactory;
@@ -549,6 +551,11 @@ public class ChangeData {
this.project = null;
}
public ChangeData setLazyLoad(boolean load) {
lazyLoad = load;
return this;
}
public ReviewDb db() {
return db;
}
@@ -569,10 +576,7 @@ public class ChangeData {
public List<String> currentFilePaths() throws OrmException {
PatchSet ps = currentPatchSet();
if (ps == null) {
return null;
}
return filePaths(currentPatchSet);
return ps != null ? filePaths(ps) : null;
}
public List<String> filePaths(PatchSet ps) throws OrmException {
@@ -587,7 +591,9 @@ public class ChangeData {
Optional<PatchList> p = getPatchList(c, ps);
if (!p.isPresent()) {
List<String> emptyFileList = Collections.emptyList();
files.put(ps.getPatchSetId(), emptyFileList);
if (lazyLoad) {
files.put(ps.getPatchSetId(), emptyFileList);
}
return emptyFileList;
}
@@ -625,6 +631,9 @@ public class ChangeData {
}
Optional<PatchList> r = patchLists.get(psId);
if (r == null) {
if (!lazyLoad) {
return Optional.absent();
}
try {
r = Optional.of(patchListCache.get(c, ps));
} catch (PatchListNotAvailableException e) {
@@ -654,6 +663,9 @@ public class ChangeData {
public Optional<ChangedLines> changedLines() throws OrmException {
if (changedLines == null) {
if (!lazyLoad) {
return Optional.absent();
}
changedLines = computeChangedLines();
}
return changedLines;
@@ -742,7 +754,7 @@ public class ChangeData {
}
public Change change() throws OrmException {
if (change == null) {
if (change == null && lazyLoad) {
reloadChange();
}
return change;
@@ -768,6 +780,9 @@ public class ChangeData {
public ChangeNotes notes() throws OrmException {
if (notes == null) {
if (!lazyLoad) {
throw new OrmException("ChangeNotes not available, lazyLoad = false");
}
notes = notesFactory.create(db, project(), legacyId);
}
return notes;
@@ -792,6 +807,9 @@ public class ChangeData {
public List<PatchSetApproval> currentApprovals()
throws OrmException {
if (currentApprovals == null) {
if (!lazyLoad) {
return Collections.emptyList();
}
Change c = change();
if (c == null) {
currentApprovals = Collections.emptyList();
@@ -928,6 +946,9 @@ public class ChangeData {
public ListMultimap<PatchSet.Id, PatchSetApproval> approvals()
throws OrmException {
if (allApprovals == null) {
if (!lazyLoad) {
return ImmutableListMultimap.of();
}
allApprovals = approvalsUtil.byChange(db, notes());
}
return allApprovals;
@@ -949,6 +970,9 @@ public class ChangeData {
public ReviewerSet reviewers() throws OrmException {
if (reviewers == null) {
if (!lazyLoad) {
return ReviewerSet.empty();
}
reviewers = approvalsUtil.getReviewers(notes(), approvals().values());
}
return reviewers;
@@ -964,6 +988,9 @@ public class ChangeData {
public List<ReviewerStatusUpdate> reviewerUpdates() throws OrmException {
if (reviewerUpdates == null) {
if (!lazyLoad) {
return Collections.emptyList();
}
reviewerUpdates = approvalsUtil.getReviewerUpdates(notes());
}
return reviewerUpdates;
@@ -980,6 +1007,9 @@ public class ChangeData {
public Collection<PatchLineComment> publishedComments()
throws OrmException {
if (publishedComments == null) {
if (!lazyLoad) {
return Collections.emptyList();
}
publishedComments = plcUtil.publishedByChange(db, notes());
}
return publishedComments;
@@ -988,6 +1018,9 @@ public class ChangeData {
public List<ChangeMessage> messages()
throws OrmException {
if (messages == null) {
if (!lazyLoad) {
return Collections.emptyList();
}
messages = cmUtil.byChange(db, notes());
}
return messages;
@@ -1021,6 +1054,9 @@ public class ChangeData {
if (c.getStatus() == Change.Status.MERGED) {
mergeable = true;
} else {
if (!lazyLoad) {
return null;
}
PatchSet ps = currentPatchSet();
try {
if (ps == null || !changeControl().isPatchVisible(ps, db)) {
@@ -1057,6 +1093,9 @@ public class ChangeData {
public Set<Account.Id> editsByUser() throws OrmException {
if (editsByUser == null) {
if (!lazyLoad) {
return Collections.emptySet();
}
Change c = change();
if (c == null) {
return Collections.emptySet();
@@ -1079,6 +1118,9 @@ public class ChangeData {
public Set<Account.Id> draftsByUser() throws OrmException {
if (draftsByUser == null) {
if (!lazyLoad) {
return Collections.emptySet();
}
Change c = change();
if (c == null) {
return Collections.emptySet();
@@ -1093,6 +1135,9 @@ public class ChangeData {
public Set<Account.Id> reviewedBy() throws OrmException {
if (reviewedBy == null) {
if (!lazyLoad) {
return Collections.emptySet();
}
Change c = change();
if (c == null) {
return Collections.emptySet();
@@ -1122,6 +1167,9 @@ public class ChangeData {
public Set<String> hashtags() throws OrmException {
if (hashtags == null) {
if (!lazyLoad) {
return Collections.emptySet();
}
hashtags = notes().getHashtags();
}
return hashtags;
@@ -1134,6 +1182,9 @@ public class ChangeData {
@Deprecated
public Set<Account.Id> starredBy() throws OrmException {
if (starredByUser == null) {
if (!lazyLoad) {
return Collections.emptySet();
}
starredByUser = checkNotNull(starredChangesUtil).byChange(
legacyId, StarredChangesUtil.DEFAULT_LABEL);
}
@@ -1147,6 +1198,9 @@ public class ChangeData {
public ImmutableMultimap<Account.Id, String> stars() throws OrmException {
if (stars == null) {
if (!lazyLoad) {
return ImmutableMultimap.of();
}
stars = checkNotNull(starredChangesUtil).byChange(legacyId);
}
return stars;

9
gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java
View File

@@ -14,6 +14,8 @@
package com.google.gerrit.server.query.change;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.restapi.AuthException;
@@ -122,6 +124,7 @@ public class QueryChanges implements RestReadView<TopLevelResource> {
int cnt = queries.size();
List<QueryResult<ChangeData>> results = imp.query(qb.parse(queries));
List<List<ChangeInfo>> res = json.create(options)
.lazyLoad(containsAnyOf(options, ChangeJson.REQUIRE_LAZY_LOAD))
.formatQueryResults(results);
for (int n = 0; n < cnt; n++) {
List<ChangeInfo> info = res.get(n);
@@ -131,4 +134,10 @@ public class QueryChanges implements RestReadView<TopLevelResource> {
}
return res;
}
private static boolean containsAnyOf(
EnumSet<ListChangesOption> set,
ImmutableSet<ListChangesOption> toFind) {
return !Sets.intersection(toFind, set).isEmpty();
}
}