On Mon, 1 Jul 2024 05:35:37 GMT, Chen Liang <li...@openjdk.org> wrote:
>> Issue [JDK-8164908](https://bugs.openjdk.org/browse/JDK-8164908) added >> support for functionality required to continue to support IIOP and custom >> serializers in light of additional module-based restrictions on reflection. >> It was expected that these libraries would use `sun.misc.Unsafe` in order to >> access fields of serializable classes. However, with JEP 471, the methods >> necessary to do this are being removed. >> >> To allow these libraries to continue to function, it is proposed to add two >> methods to `sun.reflect.ReflectionFactory` which will allow serialization >> libraries to acquire a method handle to generated `readObject`/`writeObject` >> methods which set or get the fields of the serializable class using the >> serialization `GetField`/`PutField` mechanism. These generated methods >> should be used by serialization libraries to serialize and deserialize >> classes which do not have a `readObject`/`writeObject` method or which use >> `ObjectInputStream.defaultReadObject`/`ObjectOutputStream.defaultWriteObject` >> to supplement default serialization. >> >> It is also proposed to add methods which allow for the reading of >> serialization-specific private static final fields from classes which have >> them. >> >> With the addition of these methods, serialization libraries no longer need >> to rely on `Unsafe` for serialization/deserialization activities. >> cc: @AlanBateman > > src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line > 441: > >> 439: } >> 440: >> 441: return >> SerializationBytecodeGenerator.defaultReadObjectForSerialization(cl); > > How likely is it for multiple libraries to request readObject for the same > class? > > If it happens that many clients request factories for the same class, we can > consider computing and leaving the MHs in a `ClassValue` instead (so the MHs > can be GC'd when `cl` is unreachable, but are otherwise cached and shared) My hope is that the method handle would be more easily inlined when each one is a separate constant. I'd feel less confident that this would be the case if I indirected through `List`. I think it would also increase the size of the generated method as well, though I'm not sure if there actually is any practical consequence to this. Also, this was easier. ;-) > src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line > 464: > >> 462: } >> 463: field.setAccessible(true); >> 464: return (ObjectStreamField[]) field.get(null); > > Should we clone the array to prevent user modification? Sure, that's a good idea. > src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line > 488: > >> 486: } >> 487: >> 488: private static boolean isValidSerializable(Class<?> cl) { > > Should we filter `Class`, `ObjectStreamClass`, and `String` > (https://docs.oracle.com/en/java/javase/22/docs/specs/serialization/serial-arch.html#writing-to-an-object-stream) > too? Hmm, good idea. > src/java.base/share/classes/jdk/internal/reflect/SerializationBytecodeGenerator.java > line 53: > >> 51: >> 52: static MethodHandle defaultReadObjectForSerialization(Class<?> cl) { >> 53: // build an anonymous+hidden nestmate to perform the read >> operation > > Maybe note that we generate erased types and use MH setters/getters because > the generated bytecode is defined under the loader of > SerializationBytecodeGenerator, which cannot access `cl`. Otherwise, the > first thing coming to my mind is that we can just use direct field access and > explicit `cl` type in the bytecode, or we are supporting hidden classes for > `cl` so we have to stay erased (which is not the case here) I added a more detailed explanation. > src/java.base/share/classes/jdk/internal/reflect/SerializationBytecodeGenerator.java > line 88: > >> 86: } >> 87: cb.ldc(DynamicConstantDesc.ofNamed( >> 88: ConstantDescs.BSM_CLASS_DATA_AT, > > One side remark: I think `classDataAt` is more useful for bootstrap method > arguments, like `nullConstant`; in actual bytecode, `classData` + `List.get` > invokeinterface should be more constant-pool friendly, as you just need one > CP entry for the whole list as opposed to one CP entry for each list item. > > But either way, this is correct, and we only care about correctness in legacy > support code. I accidentally replied in the wrong thread here: https://github.com/openjdk/jdk/pull/19702#discussion_r1661000439 > src/java.base/share/classes/jdk/internal/reflect/SerializationBytecodeGenerator.java > line 100: > >> 98: ClassDesc fieldDesc = >> fieldType.describeConstable().orElseThrow(InternalError::new); >> 99: >> 100: switch (fieldDesc.descriptorString()) { > > If you just switch over primitives + reference, consider something like > `switch (TypeKind.from(fieldType)) {` for simplicity. Ah, nice idea. This way we can be sure no case was missed. > test/jdk/sun/reflect/ReflectionFactory/ReflectionFactoryTest.java line 732: > >> 730: final String final_str; >> 731: >> 732: Ser2(final byte final_byte, final short final_short, final char >> final_char, final int final_int, > > I wonder if you can add a serializable user-defined class in the test; > `String` is a reference but it's a primitive data type in serialization > specification. Sure. The test subclasses the `Object*Stream` classes so the special type handling never comes into play, but we can be clearer about it by using a different type. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1661000439 PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1660971581 PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1661000877 PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1661014468 PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1661020564 PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1660996224 PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1661006149
