opendev/gerrit
Code Issues Proposed changes

Serialize TagSetHolder with protobuf

Browse Source 
TagSetHolder and TagSet have some slightly exotic properties:
  * A subclass of AtomicReference<ObjectId> with additional fields,
    CachedRef.
  * An ObjectIdOwnerMap, which requires a custom subclass of ObjectId
    with additional fields.
  * BitSets.
  * Volatile field references.

However, it still boils down to collections of value types, so the
resulting TagSetProto structure is pretty straightforward.

The most annoying thing is that the AtomicReference and ObjectId
subclasses can't reasonably implement equals(), so the tests need to
have more detailed assertions which reach into what would otherwise be
private fields.

While we're in there, eliminate the intermediate EntryVal class, which
was serving no purpose other than to hold the readObject/writeObject
methods for Java serialization.

It is quite possible that this change will be slower to deserialize than
using Java serialization, since it was previously able to directly
deserialize the internal data structures, whereas we now have to build
these structures piece by piece. However, as with the rest of the
serialization code, we assume that proto is good enough until proven
otherwise.

Beyond that, we don't attempt to further rework the tag cache types or
the cache as a whole. In particular:
  * Continue to use volatile types to handle incrementally updating
    specific cache entries.
  * Using composition instead of inheritance for CachedRef is out of
    scope. However, note that using protobuf for serialization means
    that we can make this change without flushing the cache.
  * Using a less exotic type than ObjectIdOwnerMap would probably
    require some benchmarking to prove that it's worth making the
    change.

Change-Id: I08623b3f51ef1a0541559bbb2360c0d06a9de9d4
This commit is contained in:
Dave Borowitz 2018-07-23 15:28:22 -07:00
parent 5c060d2769
commit b30a414171
6 changed files with 370 additions and 85 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

61
java/com/google/gerrit/server/git/TagCache.java
View File

@ -17,14 +17,11 @@ package com.google.gerrit.server.git;
import com.google.common.cache.Cache;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.cache.StringSerializer;
import com.google.inject.Inject;
import com.google.inject.Module;
import com.google.inject.Singleton;
import com.google.inject.name.Named;
import java.io.IOException;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.io.Serializable;
import java.util.concurrent.ExecutionException;
import org.eclipse.jgit.lib.ObjectId;
@ -36,16 +33,19 @@ public class TagCache {
return new CacheModule() {
@Override
protected void configure() {
persist(CACHE_NAME, String.class, EntryVal.class);
persist(CACHE_NAME, String.class, TagSetHolder.class)
.version(1)
.keySerializer(StringSerializer.INSTANCE)
.valueSerializer(TagSetHolder.Serializer.INSTANCE);
bind(TagCache.class);
}
};
}
private final Cache<String, EntryVal> cache;
private final Cache<String, TagSetHolder> cache;
@Inject
TagCache(@Named(CACHE_NAME) Cache<String, EntryVal> cache) {
TagCache(@Named(CACHE_NAME) Cache<String, TagSetHolder> cache) {
this.cache = cache;
}
@ -68,15 +68,12 @@ public class TagCache {
// never fail with an exception. Some of these references can be null
// (e.g. not all projects are cached, or the cache is not current).
//
EntryVal val = cache.getIfPresent(name.get());
if (val != null) {
TagSetHolder holder = val.holder;
if (holder != null) {
TagSet tags = holder.getTagSet();
if (tags != null) {
if (tags.updateFastForward(refName, oldValue, newValue)) {
cache.put(name.get(), val);
}
TagSetHolder holder = cache.getIfPresent(name.get());
if (holder != null) {
TagSet tags = holder.getTagSet();
if (tags != null) {
if (tags.updateFastForward(refName, oldValue, newValue)) {
cache.put(name.get(), holder);
}
}
}
@ -84,41 +81,13 @@ public class TagCache {
public TagSetHolder get(Project.NameKey name) {
try {
return cache.get(name.get(), () -> new EntryVal(new TagSetHolder(name))).holder;
return cache.get(name.get(), () -> new TagSetHolder(name));
} catch (ExecutionException e) {
throw new IllegalStateException(e);
}
}
void put(Project.NameKey name, TagSetHolder tags) {
cache.put(name.get(), new EntryVal(tags));
}
public static class EntryVal implements Serializable {
static final long serialVersionUID = 1L;
transient TagSetHolder holder;
private EntryVal(TagSetHolder holder) {
this.holder = holder;
}
private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException {
holder = new TagSetHolder(new Project.NameKey(in.readUTF()));
if (in.readBoolean()) {
TagSet tags = new TagSet(holder.getProjectName());
tags.readObject(in);
holder.setTagSet(tags);
}
}
private void writeObject(ObjectOutputStream out) throws IOException {
TagSet tags = holder.getTagSet();
out.writeUTF(holder.getProjectName().get());
out.writeBoolean(tags != null);
if (tags != null) {
tags.writeObject(out);
}
}
cache.put(name.get(), tags);
}
}

131
java/com/google/gerrit/server/git/TagSet.java
View File

@ -14,16 +14,19 @@
package com.google.gerrit.server.git;
import static org.eclipse.jgit.lib.ObjectIdSerializer.readWithoutMarker;
import static org.eclipse.jgit.lib.ObjectIdSerializer.writeWithoutMarker;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.MoreObjects;
import com.google.common.collect.Maps;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.cache.ProtoCacheSerializers.ObjectIdConverter;
import com.google.gerrit.server.cache.proto.Cache.TagSetHolderProto.TagSetProto;
import com.google.gerrit.server.cache.proto.Cache.TagSetHolderProto.TagSetProto.CachedRefProto;
import com.google.gerrit.server.cache.proto.Cache.TagSetHolderProto.TagSetProto.TagProto;
import com.google.protobuf.ByteString;
import java.io.IOException;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.util.BitSet;
import java.util.HashMap;
import java.util.Map;
@ -47,15 +50,35 @@ class TagSet {
private final ObjectIdOwnerMap<Tag> tags;
TagSet(Project.NameKey projectName) {
this(projectName, new HashMap<>(), new ObjectIdOwnerMap<>());
}
TagSet(Project.NameKey projectName, HashMap<String, CachedRef> refs, ObjectIdOwnerMap<Tag> tags) {
this.projectName = projectName;
this.refs = new HashMap<>();
this.tags = new ObjectIdOwnerMap<>();
this.refs = refs;
this.tags = tags;
}
Project.NameKey getProjectName() {
return projectName;
}
Tag lookupTag(AnyObjectId id) {
return tags.get(id);
}
// Test methods have obtuse names in addition to annotations, since they expose mutable state
// which would be easy to corrupt.
@VisibleForTesting
Map<String, CachedRef> getRefsForTesting() {
return refs;
}
@VisibleForTesting
ObjectIdOwnerMap<Tag> getTagsForTesting() {
return tags;
}
boolean updateFastForward(String refName, ObjectId oldValue, ObjectId newValue) {
CachedRef ref = refs.get(refName);
if (ref != null) {
@ -191,36 +214,46 @@ class TagSet {
}
}
void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException {
int refCnt = in.readInt();
for (int i = 0; i < refCnt; i++) {
String name = in.readUTF();
int flag = in.readInt();
ObjectId id = readWithoutMarker(in);
refs.put(name, new CachedRef(flag, id));
}
static TagSet fromProto(TagSetProto proto) {
ObjectIdConverter idConverter = ObjectIdConverter.create();
int tagCnt = in.readInt();
for (int i = 0; i < tagCnt; i++) {
ObjectId id = readWithoutMarker(in);
BitSet flags = (BitSet) in.readObject();
tags.add(new Tag(id, flags));
}
HashMap<String, CachedRef> refs = Maps.newHashMapWithExpectedSize(proto.getRefCount());
proto
.getRefMap()
.forEach(
(n, cr) ->
refs.put(n, new CachedRef(cr.getFlag(), idConverter.fromByteString(cr.getId()))));
ObjectIdOwnerMap<Tag> tags = new ObjectIdOwnerMap<>();
proto
.getTagList()
.forEach(
t ->
tags.add(
new Tag(
idConverter.fromByteString(t.getId()),
BitSet.valueOf(t.getFlags().asReadOnlyByteBuffer()))));
return new TagSet(new Project.NameKey(proto.getProjectName()), refs, tags);
}
void writeObject(ObjectOutputStream out) throws IOException {
out.writeInt(refs.size());
for (Map.Entry<String, CachedRef> e : refs.entrySet()) {
out.writeUTF(e.getKey());
out.writeInt(e.getValue().flag);
writeWithoutMarker(out, e.getValue().get());
}
out.writeInt(tags.size());
for (Tag tag : tags) {
writeWithoutMarker(out, tag);
out.writeObject(tag.refFlags);
}
TagSetProto toProto() {
ObjectIdConverter idConverter = ObjectIdConverter.create();
TagSetProto.Builder b = TagSetProto.newBuilder().setProjectName(projectName.get());
refs.forEach(
(n, cr) ->
b.putRef(
n,
CachedRefProto.newBuilder()
.setId(idConverter.toByteString(cr.get()))
.setFlag(cr.flag)
.build()));
tags.forEach(
t ->
b.addTag(
TagProto.newBuilder()
.setId(idConverter.toByteString(t))
.setFlags(ByteString.copyFrom(t.refFlags.toByteArray()))
.build()));
return b.build();
}
private boolean refresh(TagSet old, TagMatcher m) {
@ -341,8 +374,17 @@ class TagSet {
return ref.getName().startsWith(Constants.R_TAGS);
}
@Override
public String toString() {
return MoreObjects.toStringHelper(this)
.add("projectName", projectName)
.add("refs", refs)
.add("tags", tags)
.toString();
}
static final class Tag extends ObjectIdOwnerMap.Entry {
private final BitSet refFlags;
@VisibleForTesting final BitSet refFlags;
Tag(AnyObjectId id, BitSet flags) {
super(id);
@ -352,11 +394,15 @@ class TagSet {
boolean has(BitSet mask) {
return refFlags.intersects(mask);
}
@Override
public String toString() {
return MoreObjects.toStringHelper(this).addValue(name()).add("refFlags", refFlags).toString();
}
}
private static final class CachedRef extends AtomicReference<ObjectId> {
private static final long serialVersionUID = 1L;
@VisibleForTesting
static final class CachedRef extends AtomicReference<ObjectId> {
final int flag;
CachedRef(Ref ref, int flag) {
@ -367,6 +413,15 @@ class TagSet {
this.flag = flag;
set(id);
}
@Override
public String toString() {
ObjectId id = get();
return MoreObjects.toStringHelper(this)
.addValue(id != null ? id.name() : "null")
.add("flag", flag)
.toString();
}
}
private static final class TagWalk extends RevWalk {

33
java/com/google/gerrit/server/git/TagSetHolder.java
View File

@ -16,7 +16,11 @@ package com.google.gerrit.server.git;
import static java.util.stream.Collectors.toList;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.cache.CacheSerializer;
import com.google.gerrit.server.cache.ProtoCacheSerializers;
import com.google.gerrit.server.cache.proto.Cache.TagSetHolderProto;
import java.util.Collection;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
@ -24,7 +28,8 @@ import org.eclipse.jgit.lib.Repository;
public class TagSetHolder {
private final Object buildLock = new Object();
private final Project.NameKey projectName;
private volatile TagSet tags;
@Nullable private volatile TagSet tags;
TagSetHolder(Project.NameKey projectName) {
this.projectName = projectName;
@ -94,4 +99,30 @@ public class TagSetHolder {
return cur;
}
}
enum Serializer implements CacheSerializer<TagSetHolder> {
INSTANCE;
@Override
public byte[] serialize(TagSetHolder object) {
TagSetHolderProto.Builder b =
TagSetHolderProto.newBuilder().setProjectName(object.projectName.get());
TagSet tags = object.tags;
if (tags != null) {
b.setTags(tags.toProto());
}
return ProtoCacheSerializers.toByteArray(b.build());
}
@Override
public TagSetHolder deserialize(byte[] in) {
TagSetHolderProto proto =
ProtoCacheSerializers.parseUnchecked(TagSetHolderProto.parser(), in);
TagSetHolder holder = new TagSetHolder(new Project.NameKey(proto.getProjectName()));
if (proto.hasTags()) {
holder.tags = TagSet.fromProto(proto.getTags());
}
return holder;
}
}
}

57
javatests/com/google/gerrit/server/git/TagSetHolderTest.java Normal file
View File

@ -0,0 +1,57 @@
// Copyright (C) 2018 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.git;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.cache.proto.Cache.TagSetHolderProto;
import org.junit.Test;
public class TagSetHolderTest {
@Test
public void serializerWithTagSet() throws Exception {
TagSetHolder holder = new TagSetHolder(new Project.NameKey("project"));
holder.setTagSet(new TagSet(holder.getProjectName()));
byte[] serialized = TagSetHolder.Serializer.INSTANCE.serialize(holder);
assertThat(TagSetHolderProto.parseFrom(serialized))
.ignoringRepeatedFieldOrder()
.isEqualTo(
TagSetHolderProto.newBuilder()
.setProjectName("project")
.setTags(holder.getTagSet().toProto())
.build());
TagSetHolder deserialized = TagSetHolder.Serializer.INSTANCE.deserialize(serialized);
assertThat(deserialized.getProjectName()).isEqualTo(holder.getProjectName());
TagSetTest.assertEqual(holder.getTagSet(), deserialized.getTagSet());
}
@Test
public void serializerWithoutTagSet() throws Exception {
TagSetHolder holder = new TagSetHolder(new Project.NameKey("project"));
byte[] serialized = TagSetHolder.Serializer.INSTANCE.serialize(holder);
assertThat(TagSetHolderProto.parseFrom(serialized))
.ignoringRepeatedFieldOrder()
.isEqualTo(TagSetHolderProto.newBuilder().setProjectName("project").build());
TagSetHolder deserialized = TagSetHolder.Serializer.INSTANCE.deserialize(serialized);
assertThat(deserialized.getProjectName()).isEqualTo(holder.getProjectName());
TagSetTest.assertEqual(holder.getTagSet(), deserialized.getTagSet());
}
}

147
javatests/com/google/gerrit/server/git/TagSetTest.java Normal file
View File

@ -0,0 +1,147 @@
// Copyright (C) 2018 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.git;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat;
import static com.google.gerrit.server.cache.testing.CacheSerializerTestUtil.bytes;
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.collect.Streams;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.cache.proto.Cache.TagSetHolderProto.TagSetProto;
import com.google.gerrit.server.cache.proto.Cache.TagSetHolderProto.TagSetProto.CachedRefProto;
import com.google.gerrit.server.cache.proto.Cache.TagSetHolderProto.TagSetProto.TagProto;
import com.google.gerrit.server.git.TagSet.CachedRef;
import com.google.gerrit.server.git.TagSet.Tag;
import java.util.Arrays;
import java.util.BitSet;
import java.util.Comparator;
import java.util.HashMap;
import java.util.Map;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectIdOwnerMap;
import org.junit.Test;
public class TagSetTest {
@Test
public void roundTripToProto() {
HashMap<String, CachedRef> refs = new HashMap<>();
refs.put(
"refs/heads/master",
new CachedRef(1, ObjectId.fromString("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")));
refs.put(
"refs/heads/branch",
new CachedRef(2, ObjectId.fromString("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb")));
ObjectIdOwnerMap<Tag> tags = new ObjectIdOwnerMap<>();
tags.add(
new Tag(
ObjectId.fromString("cccccccccccccccccccccccccccccccccccccccc"), newBitSet(1, 3, 5)));
tags.add(
new Tag(
ObjectId.fromString("dddddddddddddddddddddddddddddddddddddddd"), newBitSet(2, 4, 6)));
TagSet tagSet = new TagSet(new Project.NameKey("project"), refs, tags);
TagSetProto proto = tagSet.toProto();
assertThat(proto)
.ignoringRepeatedFieldOrder()
.isEqualTo(
TagSetProto.newBuilder()
.setProjectName("project")
.putRef(
"refs/heads/master",
CachedRefProto.newBuilder()
.setId(
bytes(
0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa,
0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa))
.setFlag(1)
.build())
.putRef(
"refs/heads/branch",
CachedRefProto.newBuilder()
.setId(
bytes(
0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb,
0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb))
.setFlag(2)
.build())
.addTag(
TagProto.newBuilder()
.setId(
bytes(
0xcc, 0xcc, 0xcc, 0xcc, 0xcc, 0xcc, 0xcc, 0xcc, 0xcc, 0xcc, 0xcc,
0xcc, 0xcc, 0xcc, 0xcc, 0xcc, 0xcc, 0xcc, 0xcc, 0xcc))
.setFlags(bytes(0x2a))
.build())
.addTag(
TagProto.newBuilder()
.setId(
bytes(
0xdd, 0xdd, 0xdd, 0xdd, 0xdd, 0xdd, 0xdd, 0xdd, 0xdd, 0xdd, 0xdd,
0xdd, 0xdd, 0xdd, 0xdd, 0xdd, 0xdd, 0xdd, 0xdd, 0xdd))
.setFlags(bytes(0x54))
.build())
.build());
assertEqual(tagSet, TagSet.fromProto(proto));
}
// TODO(dborowitz): Find some more common place to put this method, which requires access to
// package-private TagSet details.
static void assertEqual(@Nullable TagSet a, @Nullable TagSet b) {
if (a == null || b == null) {
assertWithMessage("only one TagSet is null out of\n%s\n%s", a, b)
.that(a == null && b == null)
.isTrue();
return;
}
assertThat(a.getProjectName()).isEqualTo(b.getProjectName());
Map<String, CachedRef> aRefs = a.getRefsForTesting();
Map<String, CachedRef> bRefs = b.getRefsForTesting();
assertThat(ImmutableSortedSet.copyOf(aRefs.keySet()))
.named("ref name set")
.isEqualTo(ImmutableSortedSet.copyOf(bRefs.keySet()));
for (String name : aRefs.keySet()) {
CachedRef aRef = aRefs.get(name);
CachedRef bRef = bRefs.get(name);
assertThat(aRef.get()).named("value of ref %s", name).isEqualTo(bRef.get());
assertThat(aRef.flag).named("flag of ref %s", name).isEqualTo(bRef.flag);
}
ObjectIdOwnerMap<Tag> aTags = a.getTagsForTesting();
ObjectIdOwnerMap<Tag> bTags = b.getTagsForTesting();
assertThat(getTagIds(aTags)).named("tag ID set").isEqualTo(getTagIds(bTags));
for (Tag aTag : aTags) {
Tag bTag = bTags.get(aTag);
assertThat(aTag.refFlags).named("flags for tag %s", aTag.name()).isEqualTo(bTag.refFlags);
}
}
private static ImmutableSortedSet<String> getTagIds(ObjectIdOwnerMap<Tag> bTags) {
return Streams.stream(bTags)
.map(Tag::name)
.collect(ImmutableSortedSet.toImmutableSortedSet(Comparator.naturalOrder()));
}
private BitSet newBitSet(int... bits) {
BitSet result = new BitSet();
Arrays.stream(bits).forEach(result::set);
return result;
}
}

26
proto/cache.proto
View File

@ -193,3 +193,29 @@ message ConflictKeyProto {
string submit_type = 3;
bool content_merge = 4;
}
// Serialized form of com.google.gerrit.server.query.git.TagSetHolder.
// Next ID: 3
message TagSetHolderProto {
string project_name = 1;
// Next ID: 4
message TagSetProto {
string project_name = 1;
// Next ID: 3
message CachedRefProto {
bytes id = 1;
int32 flag = 2;
}
map<string, CachedRefProto> ref = 2;
// Next ID: 3
message TagProto {
bytes id = 1;
bytes flags = 2;
}
repeated TagProto tag = 3;
}
TagSetProto tags = 2;
}