Skip to content

Commit

Permalink
[Java] Fix Collection/Map jit/interpreter protocol inconsisitency for…
Browse files Browse the repository at this point in the history 
… generics instantiated subclass (#947)

* remove collection class auto element class infer

* remove map class auto kv class infer

* refine comments

* fix javadoc

* lint code
  • Loading branch information
@chaokunyang
chaokunyang authored Oct 5, 2023
1 parent 4b135c5 commit 71f2531
Show file tree
Hide file tree
Showing 7 changed files with 60 additions and 95 deletions.
72 changes: 28 additions & 44 deletions java/fury-core/src/main/java/io/fury/serializer/CollectionSerializers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,7 @@

package io.fury.serializer;

import static io.fury.type.TypeUtils.getRawType;

import com.google.common.base.Preconditions;
import com.google.common.reflect.TypeToken;
import io.fury.Fury;
import io.fury.annotation.CodegenInvoke;
import io.fury.config.Language;
Expand All @@ -31,7 +28,6 @@
import io.fury.resolver.RefResolver;
import io.fury.type.GenericType;
import io.fury.type.Type;
import io.fury.type.TypeUtils;
import io.fury.util.Platform;
import io.fury.util.ReflectionUtils;
import java.lang.reflect.Constructor;
Expand Down Expand Up @@ -85,46 +81,34 @@ public static class Flags {
public static int NOT_SAME_TYPE = 0b1000;
}

/** Serializer for {@link Collection}. All collection serializer should extend this class. */
public static class CollectionSerializer<T extends Collection> extends Serializer<T> {
private Constructor<?> constructor;
private final boolean supportCodegenHook;
// TODO remove elemSerializer, support generics in CompatibleSerializer.
private Serializer<?> elemSerializer;
protected final ClassInfoHolder elementClassInfoHolder;
// support subclass whose element type are instantiated already, such as
// `Subclass extends ArrayList<String>`.
// nested generics such as `Subclass extends ArrayList<List<Integer>>` can only be passed by
// `pushGenerics` instead of set element serializers.
private final GenericType collectionGenericType;
// For subclass whose element type are instantiated already, such as
// `Subclass extends ArrayList<String>`. If declared `Collection` doesn't specify
// instantiated element type, then the serialization will need to write this element
// type. Although we can extract this generics when creating the serializer,
// we can't do it when jit `Serializer` for some class which contains one of such collection
// field. So we will write this extra element class to keep protocol consistency between
// interpreter and jit mode although it seems unnecessary.
// With elements header, we can write this element class only once, the cost won't be too much.

public CollectionSerializer(Fury fury, Class<T> cls) {
this(fury, cls, !ReflectionUtils.isDynamicGeneratedCLass(cls), true);
this(fury, cls, !ReflectionUtils.isDynamicGeneratedCLass(cls));
}

public CollectionSerializer(
Fury fury, Class<T> cls, boolean supportCodegenHook, boolean inferGenerics) {
public CollectionSerializer(Fury fury, Class<T> cls, boolean supportCodegenHook) {
super(fury, cls);
this.supportCodegenHook = supportCodegenHook;
elementClassInfoHolder = fury.getClassResolver().nilClassInfoHolder();
if (inferGenerics) {
TypeToken<?> elementType = TypeUtils.getElementType(TypeToken.of(cls));
if (getRawType(elementType) != Object.class) {
collectionGenericType =
fury.getClassResolver()
.buildGenericType(TypeUtils.collectionOf(elementType).getType());
} else {
collectionGenericType = null;
}
} else {
collectionGenericType = null;
}
}

private GenericType getElementGenericType(Fury fury) {
GenericType genericType = fury.getGenerics().nextGenericType();
if (genericType == null || genericType.getTypeParametersCount() < 1) {
genericType = collectionGenericType;
}
GenericType elemGenericType = null;
if (genericType != null) {
elemGenericType = genericType.getTypeParameter0();
Expand Down Expand Up @@ -735,7 +719,7 @@ public void xreadElements(

public static final class ArrayListSerializer extends CollectionSerializer<ArrayList> {
public ArrayListSerializer(Fury fury) {
super(fury, ArrayList.class, true, false);
super(fury, ArrayList.class, true);
}

@Override
Expand All @@ -755,7 +739,7 @@ public static final class ArraysAsListSerializer extends CollectionSerializer<Li
private final long arrayFieldOffset;

public ArraysAsListSerializer(Fury fury, Class<List<?>> cls) {
super(fury, cls, false, false);
super(fury, cls, false);
try {
Field arrayField = Class.forName("java.util.Arrays$ArrayList").getDeclaredField("a");
arrayFieldOffset = ReflectionUtils.getFieldOffset(arrayField);
Expand Down Expand Up @@ -806,7 +790,7 @@ public List<?> xread(MemoryBuffer buffer) {

public static final class HashSetSerializer extends CollectionSerializer<HashSet> {
public HashSetSerializer(Fury fury) {
super(fury, HashSet.class, true, false);
super(fury, HashSet.class, true);
}

@Override
Expand All @@ -824,7 +808,7 @@ public HashSet newCollection(MemoryBuffer buffer, int numElements) {

public static final class LinkedHashSetSerializer extends CollectionSerializer<LinkedHashSet> {
public LinkedHashSetSerializer(Fury fury) {
super(fury, LinkedHashSet.class, true, false);
super(fury, LinkedHashSet.class, true);
}

@Override
Expand All @@ -844,7 +828,7 @@ public static class SortedSetSerializer<T extends SortedSet> extends CollectionS
private Constructor<?> constructor;

public SortedSetSerializer(Fury fury, Class<T> cls) {
super(fury, cls, true, false);
super(fury, cls, true);
if (cls != TreeSet.class) {
try {
this.constructor = cls.getConstructor(Comparator.class);
Expand Down Expand Up @@ -889,7 +873,7 @@ public T newCollection(MemoryBuffer buffer, int numElements) {
public static final class EmptyListSerializer extends CollectionSerializer<List<?>> {

public EmptyListSerializer(Fury fury, Class<List<?>> cls) {
super(fury, cls, false, false);
super(fury, cls, false);
}

@Override
Expand Down Expand Up @@ -921,7 +905,7 @@ public List<?> xread(MemoryBuffer buffer) {
public static final class EmptySetSerializer extends CollectionSerializer<Set<?>> {

public EmptySetSerializer(Fury fury, Class<Set<?>> cls) {
super(fury, cls, false, false);
super(fury, cls, false);
}

@Override
Expand Down Expand Up @@ -953,7 +937,7 @@ public Set<?> xread(MemoryBuffer buffer) {
public static final class EmptySortedSetSerializer extends CollectionSerializer<SortedSet<?>> {

public EmptySortedSetSerializer(Fury fury, Class<SortedSet<?>> cls) {
super(fury, cls, false, false);
super(fury, cls, false);
}

@Override
Expand All @@ -969,7 +953,7 @@ public static final class CollectionsSingletonListSerializer
extends CollectionSerializer<List<?>> {

public CollectionsSingletonListSerializer(Fury fury, Class<List<?>> cls) {
super(fury, cls, false, false);
super(fury, cls, false);
}

@Override
Expand Down Expand Up @@ -1003,7 +987,7 @@ public List<?> xread(MemoryBuffer buffer) {
public static final class CollectionsSingletonSetSerializer extends CollectionSerializer<Set<?>> {

public CollectionsSingletonSetSerializer(Fury fury, Class<Set<?>> cls) {
super(fury, cls, false, false);
super(fury, cls, false);
}

@Override
Expand Down Expand Up @@ -1053,7 +1037,7 @@ public ConcurrentSkipListSet newCollection(MemoryBuffer buffer, int numElements)
public static final class VectorSerializer extends CollectionSerializer<Vector> {

public VectorSerializer(Fury fury, Class<Vector> cls) {
super(fury, cls, true, false);
super(fury, cls, true);
}

@Override
Expand All @@ -1067,7 +1051,7 @@ public Vector newCollection(MemoryBuffer buffer, int numElements) {
public static final class ArrayDequeSerializer extends CollectionSerializer<ArrayDeque> {

public ArrayDequeSerializer(Fury fury, Class<ArrayDeque> cls) {
super(fury, cls, true, false);
super(fury, cls, true);
}

@Override
Expand All @@ -1082,7 +1066,7 @@ public static class EnumSetSerializer extends CollectionSerializer<EnumSet> {
public EnumSetSerializer(Fury fury, Class<EnumSet> type) {
// getElementType(EnumSet.class) will be `E` without Enum as bound.
// so no need to infer generics in init.
super(fury, type, false, false);
super(fury, type, false);
}

@Override
Expand Down Expand Up @@ -1139,7 +1123,7 @@ public BitSet read(MemoryBuffer buffer) {

public static class PriorityQueueSerializer extends CollectionSerializer<PriorityQueue> {
public PriorityQueueSerializer(Fury fury, Class<PriorityQueue> cls) {
super(fury, cls, true, false);
super(fury, cls, true);
}

public void writeHeader(MemoryBuffer buffer, PriorityQueue value) {
Expand All @@ -1165,7 +1149,7 @@ public static final class DefaultJavaCollectionSerializer<T extends Collection>
private Serializer<T> dataSerializer;

public DefaultJavaCollectionSerializer(Fury fury, Class<T> cls) {
super(fury, cls, false, false);
super(fury, cls, false);
Preconditions.checkArgument(
fury.getLanguage() == Language.JAVA,
"Python default collection serializer should use " + CollectionSerializer.class);
Expand Down Expand Up @@ -1196,7 +1180,7 @@ public static final class JDKCompatibleCollectionSerializer<T extends Collection
private final Serializer serializer;

public JDKCompatibleCollectionSerializer(Fury fury, Class<T> cls) {
super(fury, cls, false, false);
super(fury, cls, false);
// Collection which defined `writeReplace` may use this serializer, so check replace/resolve
// is necessary.
Class<? extends Serializer> serializerType =
Expand Down Expand Up @@ -1227,7 +1211,7 @@ public static void registerDefaultSerializers(Fury fury) {
Class arrayAsListClass = Arrays.asList(1, 2).getClass();
fury.registerSerializer(arrayAsListClass, new ArraysAsListSerializer(fury, arrayAsListClass));
fury.registerSerializer(
LinkedList.class, new CollectionSerializer(fury, LinkedList.class, true, false));
LinkedList.class, new CollectionSerializer(fury, LinkedList.class, true));
fury.registerSerializer(HashSet.class, new HashSetSerializer(fury));
fury.registerSerializer(LinkedHashSet.class, new LinkedHashSetSerializer(fury));
fury.registerSerializer(TreeSet.class, new SortedSetSerializer<>(fury, TreeSet.class));
Expand Down
6 changes: 3 additions & 3 deletions java/fury-core/src/main/java/io/fury/serializer/GuavaSerializers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public class GuavaSerializers {
abstract static class GuavaCollectionSerializer<T extends Collection>
extends CollectionSerializer<T> {
public GuavaCollectionSerializer(Fury fury, Class<T> cls) {
super(fury, cls, true, false);
super(fury, cls, true);
fury.getClassResolver().setSerializer(cls, this);
}

Expand Down Expand Up @@ -183,7 +183,7 @@ public static final class ImmutableSortedSetSerializer<T extends ImmutableSorted
extends CollectionSerializer<T> {

public ImmutableSortedSetSerializer(Fury fury, Class<T> cls) {
super(fury, cls, false, false);
super(fury, cls, false);
fury.getClassResolver().setSerializer(cls, this);
}

Expand Down Expand Up @@ -211,7 +211,7 @@ public T onCollectionRead(Collection collection) {
abstract static class GuavaMapSerializer<T extends Map> extends MapSerializer<T> {

public GuavaMapSerializer(Fury fury, Class<T> cls) {
super(fury, cls, true, false);
super(fury, cls, true);
fury.getClassResolver().setSerializer(cls, this);
}

Expand Down
6 changes: 3 additions & 3 deletions java/fury-core/src/main/java/io/fury/serializer/ImmutableCollectionSerializers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ class MapNStub {}

public static class ImmutableListSerializer extends CollectionSerializers.CollectionSerializer {
public ImmutableListSerializer(Fury fury, Class cls) {
super(fury, cls, true, false);
super(fury, cls, true);
}

@Override
Expand Down Expand Up @@ -136,7 +136,7 @@ public Collection onCollectionRead(Collection collection) {

public static class ImmutableSetSerializer extends CollectionSerializers.CollectionSerializer {
public ImmutableSetSerializer(Fury fury, Class cls) {
super(fury, cls, true, false);
super(fury, cls, true);
}

@Override
Expand Down Expand Up @@ -167,7 +167,7 @@ public Collection onCollectionRead(Collection collection) {

public static class ImmutableMapSerializer extends MapSerializers.MapSerializer {
public ImmutableMapSerializer(Fury fury, Class cls) {
super(fury, cls, true, false);
super(fury, cls, true);
}

@Override
Expand Down
Loading

0 comments on commit 71f2531

Please sign in to comment.