From 4757ad473f87f953edfd1d6d6590839d395ab390 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Sun, 29 May 2022 21:56:14 +0100 Subject: Lazy load change notes when submit by push Lazy load the change notes associated with the change to merge when one or more changes are submitted by a git push of the change onto its target branch. When a single change is merged through git push, we need to find which one is the corresponding change meta to be checked and merged. However, the building of the entire set of open changes against the target branch, indexed by their change key was having the side-effect of loading eagerly changes notes from their /meta ref. Loading all changes notes can be very slow, O(mins), on repositories with a large number of refs on slow storage and, also, it is completely unneeded in this use case. Loading all the change notes for all open changes had increased the time to submit a change from a few seconds (via the GUI) to a few minutes (via git push). Release-Notes: Performance improvement of change submit via push Change-Id: I646bc7b13f3359816b6be6354e15ddb5412bfa38 --- .../gerrit/server/git/receive/ReceiveCommits.java | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index 77f1e1ec5e..945eedc9f0 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -3105,7 +3105,7 @@ class ReceiveCommits { } ListMultimap byCommit = changeRefsById(); - Map byKey = null; + Map changeDataByKey = null; List replaceAndClose = new ArrayList<>(); int existingPatchSets = 0; @@ -3128,17 +3128,18 @@ class ReceiveCommits { } for (String changeId : c.getFooterLines(CHANGE_ID)) { - if (byKey == null) { - byKey = executeIndexQuery(() -> openChangesByKeyByBranch(branch)); + if (changeDataByKey == null) { + changeDataByKey = executeIndexQuery(() -> openChangesByKeyByBranch(branch)); } - ChangeNotes onto = byKey.get(new Change.Key(changeId.trim())); + ChangeData onto = changeDataByKey.get(new Change.Key(changeId.trim())); if (onto != null) { newPatchSets++; // Hold onto this until we're done with the walk, as the call to // req.validate below calls isMergedInto which resets the walk. - ReplaceRequest req = new ReplaceRequest(onto.getChangeId(), c, cmd, false); - req.notes = onto; + ChangeNotes ontoNotes = onto.notes(); + ReplaceRequest req = new ReplaceRequest(ontoNotes.getChangeId(), c, cmd, false); + req.notes = ontoNotes; replaceAndClose.add(req); continue COMMIT; } @@ -3207,12 +3208,15 @@ class ReceiveCommits { } } - private Map openChangesByKeyByBranch(Branch.NameKey branch) + private Map openChangesByKeyByBranch(Branch.NameKey branch) throws OrmException { - Map r = new HashMap<>(); + Map r = new HashMap<>(); for (ChangeData cd : queryProvider.get().byBranchOpen(branch)) { try { - r.put(cd.change().getKey(), cd.notes()); + // ChangeData is not materialised into a ChangeNotes for avoiding + // to load a potentially large number of changes meta-data into memory + // which would cause unnecessary disk I/O, CPU and heap utilisation. + r.put(cd.change().getKey(), cd); } catch (NoSuchChangeException e) { // Ignore deleted change } -- cgit v1.2.3 From 06f9ed20939c5b4c38044e4bbc7476b0c6700189 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Tue, 31 May 2022 01:04:57 -0700 Subject: Fix RepoRefCache stale checks during NoteDb rebuild When rebuilding NoteDb meta-data from ReviewDb we are actually mutating the /meta ref during a reindexing operation. This is an edge case that happens only once during the on-line NoteDb upgrade and the trial mode. Adapt the RepoRefCache use during NoteDb reindexing and manage the situation where a staleness check applies to a ref that has disappeared on disk. Bug: Issue 15961 Release-Notes: skip Change-Id: Iac642eb7a9b24882a0c77dbaee24853fb0b29827 --- java/com/google/gerrit/server/git/RepoRefCache.java | 5 +++-- java/com/google/gerrit/server/notedb/ChangeNotes.java | 10 +++++++--- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/java/com/google/gerrit/server/git/RepoRefCache.java b/java/com/google/gerrit/server/git/RepoRefCache.java index c813b8375b..4fa33118f5 100644 --- a/java/com/google/gerrit/server/git/RepoRefCache.java +++ b/java/com/google/gerrit/server/git/RepoRefCache.java @@ -136,8 +136,9 @@ public class RepoRefCache implements RefCache { } try { - ObjectId diskId = refdb.exactRef(refName).getObjectId(); - boolean isStale = !Optional.ofNullable(diskId).equals(id); + Optional diskId = + Optional.ofNullable(refdb.exactRef(refName)).map(Ref::getObjectId); + boolean isStale = !diskId.equals(id); if (isStale) { log.atSevere().log( "Repository " diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java index 3bceeeecda..e1f415d331 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotes.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java @@ -763,15 +763,19 @@ public class ChangeNotes extends AbstractChangeNotes { refs = newRefCache; } + boolean changeUpToDate = true; try { - if (!NoteDbChangeState.isChangeUpToDate(state, refs, getChangeId())) { - return rebuildAndOpen(repo, id); - } + changeUpToDate = NoteDbChangeState.isChangeUpToDate(state, refs, getChangeId()); } finally { if (newRefCache != null) { + // RepoRefCache needs to be closed for preventing caching before + // any potential mutation happens with rebuildAndOpen() newRefCache.close(); } } + if (!changeUpToDate) { + return rebuildAndOpen(repo, id); + } } return super.openHandle(repo); } -- cgit v1.2.3 From f38eb6de6c352d0de07ff4ddd71c55bc8aab6e3f Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Wed, 1 Jun 2022 22:56:17 -0700 Subject: Allow async receive-commits to have a thread-local cache Git receive-commits are executed in a background thread of the ReceiveCommits pool. The thread-local cache allocated on the client caller thread isn't used in the execution of the command making multiple operations (e.g. ACL evaluation, events propagation) slower because of the missed caching activity. Release-Notes: Improve caching when merging changes through git push Change-Id: I34a6e1485294f3156c7f35261fedfc280af1ed43 --- java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java b/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java index 882f2083a4..fa4ba4c5e0 100644 --- a/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java @@ -32,6 +32,7 @@ import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.UsedAt; +import com.google.gerrit.server.cache.PerThreadCache; import com.google.gerrit.server.config.ConfigUtil; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.ReceiveCommitsExecutor; @@ -125,7 +126,9 @@ public class AsyncReceiveCommits implements PreReceiveHook { @Override public void run() { - receiveCommits.processCommands(commands, progress); + try (PerThreadCache threadLocalCache = PerThreadCache.create(null)) { + receiveCommits.processCommands(commands, progress); + } } @Override -- cgit v1.2.3