ChangeNotesState: Store lists before defensive copying
List overhead is much lower than map overhead, and we now defensively
copy these in ChangeNotes.
Change-Id: I297aef345445c37094ce901618662d5a3c3a25d8
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
index edb5b4e..4967b0f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -25,12 +25,11 @@
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
-import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSortedMap;
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.ListMultimap;
-import com.google.common.collect.Maps;
import com.google.common.collect.Multimap;
import com.google.common.collect.Multimaps;
import com.google.common.collect.Ordering;
@@ -349,7 +348,7 @@
// Lazy defensive copies of mutable ReviewDb types, to avoid polluting the
// ChangeNotesCache from handlers.
- private ImmutableMap<PatchSet.Id, PatchSet> patchSets;
+ private ImmutableSortedMap<PatchSet.Id, PatchSet> patchSets;
private ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals;
@VisibleForTesting
@@ -368,18 +367,26 @@
return change;
}
- public ImmutableMap<PatchSet.Id, PatchSet> getPatchSets() {
+ public ImmutableSortedMap<PatchSet.Id, PatchSet> getPatchSets() {
if (patchSets == null) {
- patchSets = ImmutableMap.copyOf(
- Maps.transformValues(state.patchSets(), PatchSet::new));
+ ImmutableSortedMap.Builder<PatchSet.Id, PatchSet> b =
+ ImmutableSortedMap.orderedBy(comparing(PatchSet.Id::get));
+ for (Map.Entry<PatchSet.Id, PatchSet> e : state.patchSets()) {
+ b.put(e.getKey(), new PatchSet(e.getValue()));
+ }
+ patchSets = b.build();
}
return patchSets;
}
public ImmutableListMultimap<PatchSet.Id, PatchSetApproval> getApprovals() {
if (approvals == null) {
- approvals = ImmutableListMultimap.copyOf(
- Multimaps.transformValues(state.approvals(), PatchSetApproval::new));
+ ImmutableListMultimap.Builder<PatchSet.Id, PatchSetApproval> b =
+ ImmutableListMultimap.builder();
+ for (Map.Entry<PatchSet.Id, PatchSetApproval> e : state.approvals()) {
+ b.put(e.getKey(), new PatchSetApproval(e.getValue()));
+ }
+ approvals = b.build();
}
return approvals;
}
@@ -526,7 +533,7 @@
public PatchSet getCurrentPatchSet() {
PatchSet.Id psId = change.currentPatchSetId();
- return checkNotNull(state.patchSets().get(psId),
+ return checkNotNull(getPatchSets().get(psId),
"missing current patch set %s", psId.get());
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
index 198eb2f..73b7aec 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
@@ -101,7 +101,8 @@
+ P // status
+ P + set(state.pastAssignees(), K)
+ P + set(state.hashtags(), str(10))
- + P + map(state.patchSets(), patchSet())
+ + P + list(state.patchSets(), patchSet())
+ + P + list(state.allPastReviewers(), approval())
+ P + list(state.reviewerUpdates(), 4 * O + K + K + P)
+ P + list(state.submitRecords(), P + list(2, str(4) + P + K) + P)
+ P + list(state.allChangeMessages(), changeMessage())
@@ -172,6 +173,15 @@
+ P; // pushCertificate
}
+ private static int approval() {
+ return O
+ + P + patchSetId() + P + K + P + O + str(10)
+ + 2 // value
+ + P + T // granted
+ + P // tag
+ + P; // realAccountId
+ }
+
private static int changeMessage() {
int key = K + str(20);
return O
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesState.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesState.java
index 67eb328..e0f7920 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesState.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesState.java
@@ -15,14 +15,12 @@
package com.google.gerrit.server.notedb;
import static com.google.common.base.Preconditions.checkNotNull;
-import static java.util.Comparator.comparing;
import com.google.auto.value.AutoValue;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.ImmutableSortedMap;
import com.google.common.collect.Multimap;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.SubmitRecord;
@@ -59,17 +57,17 @@
return new AutoValue_ChangeNotesState(
change.getId(),
null,
- ImmutableSet.<Account.Id>of(),
- ImmutableSet.<String>of(),
- ImmutableSortedMap.<PatchSet.Id, PatchSet>of(),
- ImmutableListMultimap.<PatchSet.Id, PatchSetApproval>of(),
+ ImmutableSet.of(),
+ ImmutableSet.of(),
+ ImmutableList.of(),
+ ImmutableList.of(),
ReviewerSet.empty(),
- ImmutableList.<Account.Id>of(),
- ImmutableList.<ReviewerStatusUpdate>of(),
- ImmutableList.<SubmitRecord>of(),
- ImmutableList.<ChangeMessage>of(),
- ImmutableListMultimap.<PatchSet.Id, ChangeMessage>of(),
- ImmutableListMultimap.<RevId, Comment>of());
+ ImmutableList.of(),
+ ImmutableList.of(),
+ ImmutableList.of(),
+ ImmutableList.of(),
+ ImmutableListMultimap.of(),
+ ImmutableListMultimap.of());
}
static ChangeNotesState create(
@@ -117,8 +115,8 @@
status),
ImmutableSet.copyOf(pastAssignees),
ImmutableSet.copyOf(hashtags),
- ImmutableSortedMap.copyOf(patchSets, comparing(PatchSet.Id::get)),
- ImmutableListMultimap.copyOf(approvals),
+ ImmutableList.copyOf(patchSets.entrySet()),
+ ImmutableList.copyOf(approvals.entries()),
reviewers,
ImmutableList.copyOf(allPastReviewers),
ImmutableList.copyOf(reviewerUpdates),
@@ -161,8 +159,8 @@
// Other related to this Change.
abstract ImmutableSet<Account.Id> pastAssignees();
abstract ImmutableSet<String> hashtags();
- abstract ImmutableSortedMap<PatchSet.Id, PatchSet> patchSets();
- abstract ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals();
+ abstract ImmutableList<Map.Entry<PatchSet.Id, PatchSet>> patchSets();
+ abstract ImmutableList<Map.Entry<PatchSet.Id, PatchSetApproval>> approvals();
abstract ReviewerSet reviewers();
abstract ImmutableList<Account.Id> allPastReviewers();
