From 382b187d4df664e555608c895716172d96e092c5 Mon Sep 17 00:00:00 2001 From: mschaller Date: Tue, 1 Feb 2022 12:03:41 -0800 Subject: [PATCH] Automated rollback of commit 9998c6361d3d7c22184ea07e363f38e88a345701. *** Reason for rollback *** Suspected cause of b/217386541 *** Original change description *** Minor DynamicCodec simplifications. Serialization order is now based on field offset instead of field name. It's possible to create dependencies on serialization order, but it seems to be accidental. PiperOrigin-RevId: 425683231 --- .../skyframe/serialization/DynamicCodec.java | 123 +++++++++++------- .../skyframe/serialization/EnumMapCodec.java | 10 +- .../autocodec/AutoCodecProcessor.java | 4 +- .../build/lib/unsafe/StringUnsafe.java | 15 ++- .../build/lib/unsafe/UnsafeProvider.java | 2 +- 5 files changed, 92 insertions(+), 62 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/DynamicCodec.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/DynamicCodec.java index d447e3e9df4a62..3ec0e3216876cd 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/DynamicCodec.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/DynamicCodec.java @@ -14,18 +14,20 @@ package com.google.devtools.build.lib.skyframe.serialization; -import static com.google.devtools.build.lib.unsafe.UnsafeProvider.unsafe; - import com.google.common.flogger.GoogleLogger; +import com.google.devtools.build.lib.unsafe.UnsafeProvider; import com.google.protobuf.CodedInputStream; import com.google.protobuf.CodedOutputStream; import java.io.IOException; import java.lang.reflect.Array; +import java.lang.reflect.Constructor; import java.lang.reflect.Field; import java.lang.reflect.Modifier; import java.nio.ByteBuffer; +import java.util.Comparator; import java.util.Map; import java.util.TreeMap; +import sun.reflect.ReflectionFactory; /** A codec that serializes arbitrary types. */ public final class DynamicCodec implements ObjectCodec { @@ -33,10 +35,12 @@ public final class DynamicCodec implements ObjectCodec { private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); private final Class type; + private final Constructor constructor; private final TypeAndOffset[] offsets; - public DynamicCodec(Class type) { + public DynamicCodec(Class type) throws ReflectiveOperationException { this.type = type; + this.constructor = getConstructor(type); this.offsets = getOffsets(type); } @@ -53,8 +57,8 @@ public MemoizationStrategy getStrategy() { @Override public void serialize(SerializationContext context, Object obj, CodedOutputStream codedOut) throws SerializationException, IOException { - for (TypeAndOffset fieldInfo : offsets) { - serializeField(context, codedOut, obj, fieldInfo.type, fieldInfo.offset); + for (int i = 0; i < offsets.length; ++i) { + serializeField(context, codedOut, obj, offsets[i].type, offsets[i].offset); } } @@ -75,30 +79,32 @@ private void serializeField( throws SerializationException, IOException { if (type.isPrimitive()) { if (type.equals(boolean.class)) { - codedOut.writeBoolNoTag(unsafe().getBoolean(obj, offset)); + codedOut.writeBoolNoTag(UnsafeProvider.getInstance().getBoolean(obj, offset)); } else if (type.equals(byte.class)) { - codedOut.writeRawByte(unsafe().getByte(obj, offset)); + codedOut.writeRawByte(UnsafeProvider.getInstance().getByte(obj, offset)); } else if (type.equals(short.class)) { - ByteBuffer buffer = ByteBuffer.allocate(2).putShort(unsafe().getShort(obj, offset)); + ByteBuffer buffer = + ByteBuffer.allocate(2).putShort(UnsafeProvider.getInstance().getShort(obj, offset)); codedOut.writeRawBytes(buffer); } else if (type.equals(char.class)) { - ByteBuffer buffer = ByteBuffer.allocate(2).putChar(unsafe().getChar(obj, offset)); + ByteBuffer buffer = + ByteBuffer.allocate(2).putChar(UnsafeProvider.getInstance().getChar(obj, offset)); codedOut.writeRawBytes(buffer); } else if (type.equals(int.class)) { - codedOut.writeInt32NoTag(unsafe().getInt(obj, offset)); + codedOut.writeInt32NoTag(UnsafeProvider.getInstance().getInt(obj, offset)); } else if (type.equals(long.class)) { - codedOut.writeInt64NoTag(unsafe().getLong(obj, offset)); + codedOut.writeInt64NoTag(UnsafeProvider.getInstance().getLong(obj, offset)); } else if (type.equals(float.class)) { - codedOut.writeFloatNoTag(unsafe().getFloat(obj, offset)); + codedOut.writeFloatNoTag(UnsafeProvider.getInstance().getFloat(obj, offset)); } else if (type.equals(double.class)) { - codedOut.writeDoubleNoTag(unsafe().getDouble(obj, offset)); + codedOut.writeDoubleNoTag(UnsafeProvider.getInstance().getDouble(obj, offset)); } else if (type.equals(void.class)) { // Does nothing for void type. } else { throw new UnsupportedOperationException("Unknown primitive type: " + type); } } else if (type.isArray()) { - Object arr = unsafe().getObject(obj, offset); + Object arr = UnsafeProvider.getInstance().getObject(obj, offset); if (type.getComponentType().equals(byte.class)) { if (arr == null) { codedOut.writeBoolNoTag(false); @@ -114,8 +120,8 @@ private void serializeField( } int length = Array.getLength(arr); codedOut.writeInt32NoTag(length); - int base = unsafe().arrayBaseOffset(type); - int scale = unsafe().arrayIndexScale(type); + int base = UnsafeProvider.getInstance().arrayBaseOffset(type); + int scale = UnsafeProvider.getInstance().arrayIndexScale(type); if (scale == 0) { throw new SerializationException("Failed to get index scale for type: " + type); } @@ -125,7 +131,7 @@ private void serializeField( } } else { try { - context.serialize(unsafe().getObject(obj, offset), codedOut); + context.serialize(UnsafeProvider.getInstance().getObject(obj, offset), codedOut); } catch (SerializationException e) { logger.atSevere().withCause(e).log( "Unserializable object and superclass: %s %s", obj, obj.getClass().getSuperclass()); @@ -140,13 +146,13 @@ public Object deserialize(DeserializationContext context, CodedInputStream coded throws SerializationException, IOException { Object instance; try { - instance = unsafe().allocateInstance(type); + instance = constructor.newInstance(); } catch (ReflectiveOperationException e) { throw new SerializationException("Could not instantiate object of type: " + type, e); } context.registerInitialValue(instance); - for (TypeAndOffset fieldInfo : offsets) { - deserializeField(context, codedIn, instance, fieldInfo.type, fieldInfo.offset); + for (int i = 0; i < offsets.length; ++i) { + deserializeField(context, codedIn, instance, offsets[i].type, offsets[i].offset); } return instance; } @@ -168,23 +174,23 @@ private void deserializeField( throws SerializationException, IOException { if (fieldType.isPrimitive()) { if (fieldType.equals(boolean.class)) { - unsafe().putBoolean(obj, offset, codedIn.readBool()); + UnsafeProvider.getInstance().putBoolean(obj, offset, codedIn.readBool()); } else if (fieldType.equals(byte.class)) { - unsafe().putByte(obj, offset, codedIn.readRawByte()); + UnsafeProvider.getInstance().putByte(obj, offset, codedIn.readRawByte()); } else if (fieldType.equals(short.class)) { ByteBuffer buffer = ByteBuffer.allocate(2).put(codedIn.readRawBytes(2)); - unsafe().putShort(obj, offset, buffer.getShort(0)); + UnsafeProvider.getInstance().putShort(obj, offset, buffer.getShort(0)); } else if (fieldType.equals(char.class)) { ByteBuffer buffer = ByteBuffer.allocate(2).put(codedIn.readRawBytes(2)); - unsafe().putChar(obj, offset, buffer.getChar(0)); + UnsafeProvider.getInstance().putChar(obj, offset, buffer.getChar(0)); } else if (fieldType.equals(int.class)) { - unsafe().putInt(obj, offset, codedIn.readInt32()); + UnsafeProvider.getInstance().putInt(obj, offset, codedIn.readInt32()); } else if (fieldType.equals(long.class)) { - unsafe().putLong(obj, offset, codedIn.readInt64()); + UnsafeProvider.getInstance().putLong(obj, offset, codedIn.readInt64()); } else if (fieldType.equals(float.class)) { - unsafe().putFloat(obj, offset, codedIn.readFloat()); + UnsafeProvider.getInstance().putFloat(obj, offset, codedIn.readFloat()); } else if (fieldType.equals(double.class)) { - unsafe().putDouble(obj, offset, codedIn.readDouble()); + UnsafeProvider.getInstance().putDouble(obj, offset, codedIn.readDouble()); } else if (fieldType.equals(void.class)) { // Does nothing for void type. } else { @@ -193,19 +199,20 @@ private void deserializeField( } } else if (fieldType.isArray()) { if (fieldType.getComponentType().equals(byte.class)) { - if (codedIn.readBool()) { - unsafe().putObject(obj, offset, codedIn.readByteArray()); - } // Otherwise, it was null. + boolean isNonNull = codedIn.readBool(); + UnsafeProvider.getInstance() + .putObject(obj, offset, isNonNull ? codedIn.readByteArray() : null); return; } int length = codedIn.readInt32(); if (length < 0) { + UnsafeProvider.getInstance().putObject(obj, offset, null); return; } Object arr = Array.newInstance(fieldType.getComponentType(), length); - unsafe().putObject(obj, offset, arr); - int base = unsafe().arrayBaseOffset(fieldType); - int scale = unsafe().arrayIndexScale(fieldType); + UnsafeProvider.getInstance().putObject(obj, offset, arr); + int base = UnsafeProvider.getInstance().arrayBaseOffset(fieldType); + int scale = UnsafeProvider.getInstance().arrayIndexScale(fieldType); if (scale == 0) { throw new SerializationException( "Failed to get index scale for field type " + fieldType + " for " + type); @@ -225,10 +232,7 @@ private void deserializeField( e.addTrail(this.type); throw e; } - if (fieldValue == null) { - return; - } - if (!fieldType.isInstance(fieldValue)) { + if (fieldValue != null && !fieldType.isInstance(fieldValue)) { throw new SerializationException( "Field " + fieldValue @@ -239,38 +243,59 @@ private void deserializeField( + ") for " + type); } - unsafe().putObject(obj, offset, fieldValue); + UnsafeProvider.getInstance().putObject(obj, offset, fieldValue); } } private static TypeAndOffset[] getOffsets(Class type) { - TreeMap> offsets = new TreeMap<>(); + TreeMap offsets = new TreeMap<>(new FieldComparator()); for (Class next = type; next != null; next = next.getSuperclass()) { for (Field field : next.getDeclaredFields()) { if ((field.getModifiers() & (Modifier.STATIC | Modifier.TRANSIENT)) != 0) { continue; // Skips static or transient fields. } - if (offsets.put(unsafe().objectFieldOffset(field), field.getType()) != null) { - throw new IllegalStateException("Field number collision in: " + type); - } + field.setAccessible(true); + offsets.put(field, UnsafeProvider.getInstance().objectFieldOffset(field)); } } - // Converts to an array to avoid iterator allocation when iterating. + // Converts to an array to make it easy to avoid the use of iterators. TypeAndOffset[] offsetsArr = new TypeAndOffset[offsets.size()]; int i = 0; - for (Map.Entry> entry : offsets.entrySet()) { - offsetsArr[i++] = new TypeAndOffset(entry.getValue(), entry.getKey()); + for (Map.Entry entry : offsets.entrySet()) { + offsetsArr[i] = new TypeAndOffset(entry.getKey().getType(), entry.getValue()); + ++i; } return offsetsArr; } private static final class TypeAndOffset { - private final Class type; - private final long offset; + public final Class type; + public final long offset; - private TypeAndOffset(Class type, long offset) { + TypeAndOffset(Class type, long offset) { this.type = type; this.offset = offset; } } + + private static Constructor getConstructor(Class type) throws ReflectiveOperationException { + Constructor constructor = + ReflectionFactory.getReflectionFactory() + .newConstructorForSerialization(type, Object.class.getDeclaredConstructor()); + constructor.setAccessible(true); + return constructor; + } + + private static final class FieldComparator implements Comparator { + + @Override + public int compare(Field f1, Field f2) { + int classCompare = + f1.getDeclaringClass().getName().compareTo(f2.getDeclaringClass().getName()); + if (classCompare != 0) { + return classCompare; + } + return f1.getName().compareTo(f2.getName()); + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/EnumMapCodec.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/EnumMapCodec.java index c441a076b4e42e..fbc340668f28a8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/EnumMapCodec.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/EnumMapCodec.java @@ -14,13 +14,13 @@ package com.google.devtools.build.lib.skyframe.serialization; -import static com.google.devtools.build.lib.unsafe.UnsafeProvider.unsafe; - +import com.google.devtools.build.lib.unsafe.UnsafeProvider; import com.google.protobuf.CodedInputStream; import com.google.protobuf.CodedOutputStream; import java.io.IOException; import java.util.EnumMap; import java.util.Map; +import sun.misc.Unsafe; /** * Serialize {@link EnumMap}. Subclasses of {@link EnumMap} will crash at runtime because currently @@ -33,7 +33,8 @@ class EnumMapCodec, V> implements ObjectCodec> { EnumMapCodec() { try { - classTypeOffset = unsafe().objectFieldOffset(EnumMap.class.getDeclaredField("keyType")); + classTypeOffset = + UnsafeProvider.getInstance().objectFieldOffset(EnumMap.class.getDeclaredField("keyType")); } catch (NoSuchFieldException e) { throw new IllegalStateException("Couldn't get keyType field fron EnumMap", e); } @@ -55,7 +56,8 @@ public void serialize(SerializationContext context, EnumMap obj, CodedOutp codedOut.writeInt32NoTag(obj.size()); if (obj.isEmpty()) { // Do gross hack to get key type of map, since we have no concrete element to examine. - context.serialize(unsafe().getObject(obj, classTypeOffset), codedOut); + Unsafe unsafe = UnsafeProvider.getInstance(); + context.serialize(unsafe.getObject(obj, classTypeOffset), codedOut); return; } context.serialize(obj.keySet().iterator().next().getDeclaringClass(), codedOut); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec/AutoCodecProcessor.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec/AutoCodecProcessor.java index 7e002046e1d031..e10d9e5c50938c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec/AutoCodecProcessor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec/AutoCodecProcessor.java @@ -508,7 +508,7 @@ private MethodSpec buildSerializeMethodWithInstantiator( } TypeKind typeKind = parameter.asType().getKind(); serializeBuilder.addStatement( - "$T unsafe_$L = ($T) $T.unsafe().get$L(input, $L_offset)", + "$T unsafe_$L = ($T) $T.getInstance().get$L(input, $L_offset)", sanitizeTypeParameter(parameter.asType(), env), parameter.getSimpleName(), sanitizeTypeParameter(parameter.asType(), env), @@ -676,7 +676,7 @@ private void initializeUnsafeOffsets( TypeName.LONG, param.getSimpleName() + "_offset", Modifier.PRIVATE, Modifier.FINAL); constructor.beginControlFlow("try"); constructor.addStatement( - "this.$L_offset = $T.unsafe().objectFieldOffset($T.class.getDeclaredField(\"$L\"))", + "this.$L_offset = $T.getInstance().objectFieldOffset($T.class.getDeclaredField(\"$L\"))", param.getSimpleName(), UnsafeProvider.class, ClassName.get(field.get().declaringClassType), diff --git a/src/main/java/com/google/devtools/build/lib/unsafe/StringUnsafe.java b/src/main/java/com/google/devtools/build/lib/unsafe/StringUnsafe.java index 2db411d53d55d8..f4153988159fbd 100644 --- a/src/main/java/com/google/devtools/build/lib/unsafe/StringUnsafe.java +++ b/src/main/java/com/google/devtools/build/lib/unsafe/StringUnsafe.java @@ -13,11 +13,10 @@ // limitations under the License. package com.google.devtools.build.lib.unsafe; -import static com.google.devtools.build.lib.unsafe.UnsafeProvider.unsafe; - import java.lang.reflect.Constructor; import java.lang.reflect.Field; import java.util.Arrays; +import sun.misc.Unsafe; /** * Provides direct access to the string implementation used by JDK9. @@ -35,6 +34,7 @@ public final class StringUnsafe { private static final StringUnsafe INSTANCE = new StringUnsafe(); + private final Unsafe unsafe; private final Constructor constructor; private final long valueOffset; private final long coderOffset; @@ -44,6 +44,7 @@ public static StringUnsafe getInstance() { } private StringUnsafe() { + unsafe = UnsafeProvider.getInstance(); Field valueField; Field coderField; try { @@ -59,13 +60,15 @@ private StringUnsafe() { e); } this.constructor.setAccessible(true); - valueOffset = unsafe().objectFieldOffset(valueField); - coderOffset = unsafe().objectFieldOffset(coderField); + valueField.setAccessible(true); + valueOffset = UnsafeProvider.getInstance().objectFieldOffset(valueField); + coderField.setAccessible(true); + coderOffset = UnsafeProvider.getInstance().objectFieldOffset(coderField); } /** Returns the coder used for this string. See {@link #LATIN1} and {@link #UTF16}. */ public byte getCoder(String obj) { - return unsafe().getByte(obj, coderOffset); + return unsafe.getByte(obj, coderOffset); } /** @@ -75,7 +78,7 @@ public byte getCoder(String obj) { * Ensure you do not mutate this byte array in any way. */ public byte[] getByteArray(String obj) { - return (byte[]) unsafe().getObject(obj, valueOffset); + return (byte[]) unsafe.getObject(obj, valueOffset); } /** Constructs a new string from a byte array and coder. */ diff --git a/src/main/java/com/google/devtools/build/lib/unsafe/UnsafeProvider.java b/src/main/java/com/google/devtools/build/lib/unsafe/UnsafeProvider.java index 4baecd5444ea27..c1e3f3b87fa77b 100644 --- a/src/main/java/com/google/devtools/build/lib/unsafe/UnsafeProvider.java +++ b/src/main/java/com/google/devtools/build/lib/unsafe/UnsafeProvider.java @@ -30,7 +30,7 @@ public class UnsafeProvider { private static final Unsafe UNSAFE = getUnsafe(); - public static Unsafe unsafe() { + public static Unsafe getInstance() { return UNSAFE; }