Re: RFR: 8315810: Reimplement sun.reflect.ReflectionFactory::newConstructorForSerialization with method handles [v3]
On Wed, 13 Sep 2023 18:40:07 GMT, Mandy Chung wrote: >> This reimplements >> `sun.reflect.ReflectionFactory::newConstructorForSerialization` with method >> handles. >> >> This API currently generates the bytecode which fails the verification >> because `new C; invokespecial A()` where the given class `C` and invoke a >> no-arg constructor of `C`'s first non-`Serializable` superclass `A` is not a >> valid operation per the VM specification. VM special cases the classes >> generated for reflection to skip verification for the constructors generated >> for serialization and externalization. This change will allow such VM hack >> to be removed. >> >> A `jdk.reflect.useOldSerializableConstructor` system property can be set to >> use the old implementation in case if customers run into any compatibility >> issue. I expect this change has very low compatibility risk. This system >> property is undocumented and will be removed in a future release. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > review feedback LGTM - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15600#pullrequestreview-1625212661
Re: RFR: 8315810: Reimplement sun.reflect.ReflectionFactory::newConstructorForSerialization with method handles [v3]
> This reimplements > `sun.reflect.ReflectionFactory::newConstructorForSerialization` with method > handles. > > This API currently generates the bytecode which fails the verification > because `new C; invokespecial A()` where the given class `C` and invoke a > no-arg constructor of `C`'s first non-`Serializable` superclass `A` is not a > valid operation per the VM specification. VM special cases the classes > generated for reflection to skip verification for the constructors generated > for serialization and externalization. This change will allow such VM hack > to be removed. > > A `jdk.reflect.useOldSerializableConstructor` system property can be set to > use the old implementation in case if customers run into any compatibility > issue. I expect this change has very low compatibility risk. This system > property is undocumented and will be removed in a future release. Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: review feedback - Changes: - all: https://git.openjdk.org/jdk/pull/15600/files - new: https://git.openjdk.org/jdk/pull/15600/files/fb3bf590..c4e39d12 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15600&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15600&range=01-02 Stats: 10 lines in 2 files changed: 0 ins; 5 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/15600.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15600/head:pull/15600 PR: https://git.openjdk.org/jdk/pull/15600
Re: RFR: 8315810: Reimplement sun.reflect.ReflectionFactory::newConstructorForSerialization with method handles [v2]
On Tue, 12 Sep 2023 19:11:30 GMT, Roger Riggs wrote: >> Mandy Chung has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains three additional >> commits since the last revision: >> >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> reflect-serializable-ctor >> - minor cleanup >> - Reimplement ReflectionFactory::newConstructorForSerialization with method >> handle > > src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 3552: > >> 3550: } >> 3551: } >> 3552: return false; > > Does it break encapsulation too much to export and use the same method from > jdk.internal.reflect.MethodHandleAccessorFactory. > Maybe not worth it for a small utility method. It's a small utility and I think it's ok to leave it this way. - PR Review Comment: https://git.openjdk.org/jdk/pull/15600#discussion_r1323533889
Re: RFR: 8315810: Reimplement sun.reflect.ReflectionFactory::newConstructorForSerialization with method handles [v2]
On Tue, 12 Sep 2023 16:37:28 GMT, Mandy Chung wrote: >> This reimplements >> `sun.reflect.ReflectionFactory::newConstructorForSerialization` with method >> handles. >> >> This API currently generates the bytecode which fails the verification >> because `new C; invokespecial A()` where the given class `C` and invoke a >> no-arg constructor of `C`'s first non-`Serializable` superclass `A` is not a >> valid operation per the VM specification. VM special cases the classes >> generated for reflection to skip verification for the constructors generated >> for serialization and externalization. This change will allow such VM hack >> to be removed. >> >> A `jdk.reflect.useOldSerializableConstructor` system property can be set to >> use the old implementation in case if customers run into any compatibility >> issue. I expect this change has very low compatibility risk. This system >> property is undocumented and will be removed in a future release. > > Mandy Chung has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Merge branch 'master' of https://github.com/openjdk/jdk into > reflect-serializable-ctor > - minor cleanup > - Reimplement ReflectionFactory::newConstructorForSerialization with method > handle src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 3552: > 3550: } > 3551: } > 3552: return false; Does it break encapsulation too much to export and use the same method from jdk.internal.reflect.MethodHandleAccessorFactory. Maybe not worth it for a small utility method. src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line 384: > 382: > getConstructorAnnotations(constructorToCall), > 383: langReflectAccess. > 384: > getConstructorParameterAnnotations(constructorToCall)); This would be easier to read if either the line length or the indentation was modified. src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line 400: > 398: } > 399: > 400: Extra blank line. - PR Review Comment: https://git.openjdk.org/jdk/pull/15600#discussion_r1323453214 PR Review Comment: https://git.openjdk.org/jdk/pull/15600#discussion_r1318801680 PR Review Comment: https://git.openjdk.org/jdk/pull/15600#discussion_r1318803452
Re: RFR: 8315810: Reimplement sun.reflect.ReflectionFactory::newConstructorForSerialization with method handles [v2]
> This reimplements > `sun.reflect.ReflectionFactory::newConstructorForSerialization` with method > handles. > > This API currently generates the bytecode which fails the verification > because `new C; invokespecial A()` where the given class `C` and invoke a > no-arg constructor of `C`'s first non-`Serializable` superclass `A` is not a > valid operation per the VM specification. VM special cases the classes > generated for reflection to skip verification for the constructors generated > for serialization and externalization. This change will allow such VM hack > to be removed. > > A `jdk.reflect.useOldSerializableConstructor` system property can be set to > use the old implementation in case if customers run into any compatibility > issue. I expect this change has very low compatibility risk. This system > property is undocumented and will be removed in a future release. Mandy Chung has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jdk into reflect-serializable-ctor - minor cleanup - Reimplement ReflectionFactory::newConstructorForSerialization with method handle - Changes: - all: https://git.openjdk.org/jdk/pull/15600/files - new: https://git.openjdk.org/jdk/pull/15600/files/e98b5528..fb3bf590 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15600&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15600&range=00-01 Stats: 33895 lines in 1007 files changed: 19846 ins; 9174 del; 4875 mod Patch: https://git.openjdk.org/jdk/pull/15600.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15600/head:pull/15600 PR: https://git.openjdk.org/jdk/pull/15600
Re: RFR: 8315810: Reimplement sun.reflect.ReflectionFactory::newConstructorForSerialization with method handles
On Wed, 6 Sep 2023 18:34:29 GMT, Mandy Chung wrote:
> This reimplements
> `sun.reflect.ReflectionFactory::newConstructorForSerialization` with method
> handles.
>
> This API currently generates the bytecode which fails the verification
> because `new C; invokespecial A()` where the given class `C` and invoke a
> no-arg constructor of `C`'s first non-`Serializable` superclass `A` is not a
> valid operation per the VM specification. VM special cases the classes
> generated for reflection to skip verification for the constructors generated
> for serialization and externalization. This change will allow such VM hack
> to be removed.
>
> A `jdk.reflect.useOldSerializableConstructor` system property can be set to
> use the old implementation in case if customers run into any compatibility
> issue. I expect this change has very low compatibility risk. This system
> property is undocumented and will be removed in a future release.
src/java.base/share/classes/jdk/internal/reflect/DirectMethodHandleAccessor.java
line 198:
> 196: if (!declaringClass.isAssignableFrom(o.getClass())) {
> 197: throw new IllegalArgumentException("object of type " +
> o.getClass().getName()
> 198: + " is not an instance of " +
> declaringClass.getName());
Suggestion:
+ " is not an instance of " + declaringClass.getName());
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15600#discussion_r1318295076
Re: RFR: 8315810: Reimplement sun.reflect.ReflectionFactory::newConstructorForSerialization with method handles
On Wed, 6 Sep 2023 22:05:47 GMT, Chen Liang wrote:
>> This reimplements
>> `sun.reflect.ReflectionFactory::newConstructorForSerialization` with method
>> handles.
>>
>> This API currently generates the bytecode which fails the verification
>> because `new C; invokespecial A()` where the given class `C` and invoke a
>> no-arg constructor of `C`'s first non-`Serializable` superclass `A` is not a
>> valid operation per the VM specification. VM special cases the classes
>> generated for reflection to skip verification for the constructors generated
>> for serialization and externalization. This change will allow such VM hack
>> to be removed.
>>
>> A `jdk.reflect.useOldSerializableConstructor` system property can be set to
>> use the old implementation in case if customers run into any compatibility
>> issue. I expect this change has very low compatibility risk. This system
>> property is undocumented and will be removed in a future release.
>
> src/java.base/share/classes/jdk/internal/reflect/MethodHandleAccessorFactory.java
> line 127:
>
>> 125: static ConstructorAccessorImpl
>> newSerializableConstructorAccessor(Class decl, Constructor ctor) {
>> 126: if (!constructorInSuperclass(decl, ctor)) {
>> 127: throw new UnsupportedOperationException(ctor + " not a
>> superclass of " + decl.getName());
>
> Notice in JShell 20, you could do this:
>
> jshell> import sun.reflect.ReflectionFactory;
> jshell> var rf = ReflectionFactory.getReflectionFactory()
> jshell> var c = rf.newConstructorForSerialization(String.class,
> Boolean.class.getConstructor(boolean.class))
> jshell> String d = (String) c.newInstance(false)
> jshell> d.length()
>
> which ends up in an NPE for `d.value` is null. This UOE is technically a new
> behavior that should be mentioned in the CSR.
good catch. will update the CSR.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15600#discussion_r1317900597
Re: RFR: 8315810: Reimplement sun.reflect.ReflectionFactory::newConstructorForSerialization with method handles
On Wed, 6 Sep 2023 18:34:29 GMT, Mandy Chung wrote:
> This reimplements
> `sun.reflect.ReflectionFactory::newConstructorForSerialization` with method
> handles.
>
> This API currently generates the bytecode which fails the verification
> because `new C; invokespecial A()` where the given class `C` and invoke a
> no-arg constructor of `C`'s first non-`Serializable` superclass `A` is not a
> valid operation per the VM specification. VM special cases the classes
> generated for reflection to skip verification for the constructors generated
> for serialization and externalization. This change will allow such VM hack
> to be removed.
>
> A `jdk.reflect.useOldSerializableConstructor` system property can be set to
> use the old implementation in case if customers run into any compatibility
> issue. I expect this change has very low compatibility risk. This system
> property is undocumented and will be removed in a future release.
src/java.base/share/classes/jdk/internal/reflect/MethodHandleAccessorFactory.java
line 127:
> 125: static ConstructorAccessorImpl
> newSerializableConstructorAccessor(Class decl, Constructor ctor) {
> 126: if (!constructorInSuperclass(decl, ctor)) {
> 127: throw new UnsupportedOperationException(ctor + " not a
> superclass of " + decl.getName());
Notice in JShell 20, you could do this:
jshell> import sun.reflect.ReflectionFactory;
jshell> var rf = ReflectionFactory.getReflectionFactory()
jshell> var c = rf.newConstructorForSerialization(String.class,
Boolean.class.getConstructor(boolean.class))
jshell> String d = (String) c.newInstance(false)
jshell> d.length()
which ends up in an NPE for `d.value` is null. This UOE is technically a new
behavior that should be mentioned in the CSR.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15600#discussion_r1317878647
Re: RFR: 8315810: Reimplement sun.reflect.ReflectionFactory::newConstructorForSerialization with method handles
On Wed, 6 Sep 2023 19:51:07 GMT, ExE Boss wrote: >> What are you referring to "to implement serialization"? It's only used by >> ReflectionFactory. > > It’s called from the implementation of > `JavaLangInvokeAccess::serializableConstructor`, which is called by > `jdk.internal.reflect.ReflectionFactory::newConstructorForSerialization`, > which is called by `java.io.ObjectStreamClass::getSerializableConstructor`: > https://github.com/openjdk/jdk/blob/86a18f5e2e0825dddb77656b2f43f64684f1464c/src/java.base/share/classes/java/io/ObjectStreamClass.java#L1444-L1446 I see the confusion. `newConstructionForSerialization` is used by serialization, exposed via `sun.reflect.ReflectionFactory`. I may clarify it. - PR Review Comment: https://git.openjdk.org/jdk/pull/15600#discussion_r1317767729
Re: RFR: 8315810: Reimplement sun.reflect.ReflectionFactory::newConstructorForSerialization with method handles
On Wed, 6 Sep 2023 19:47:19 GMT, Mandy Chung wrote: >> src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 3534: >> >>> 3532: * This method is only used to implement the unsupported >>> 3533: * >>> sun.reflect.ReflectionFactory::newConstructorForSerialization API. >>> 3534: */ >> >> The following is probably more correct: >> Suggestion: >> >> /** >> * Produces a method handle that is capable of creating instances of >> the given class >> * and instantiated by the given constructor. No security manager >> check. >> * >> * This method is used to implement serialization and the unsupported >> * sun.reflect.ReflectionFactory::newConstructorForSerialization API. >> */ > > What are you referring to "to implement serialization"? It's only used by > ReflectionFactory. It’s called from the implementation of `JavaLangInvokeAccess::serializableConstructor`, which is called by `jdk.internal.reflect.ReflectionFactory::newConstructorForSerialization`, which is called by `java.io.ObjectStreamClass::getSerializableConstructor`: https://github.com/openjdk/jdk/blob/86a18f5e2e0825dddb77656b2f43f64684f1464c/src/java.base/share/classes/java/io/ObjectStreamClass.java#L1444-L1446 - PR Review Comment: https://git.openjdk.org/jdk/pull/15600#discussion_r1317753943
Re: RFR: 8315810: Reimplement sun.reflect.ReflectionFactory::newConstructorForSerialization with method handles
On Wed, 6 Sep 2023 19:31:22 GMT, ExE Boss wrote: >> This reimplements >> `sun.reflect.ReflectionFactory::newConstructorForSerialization` with method >> handles. >> >> This API currently generates the bytecode which fails the verification >> because `new C; invokespecial A()` where the given class `C` and invoke a >> no-arg constructor of `C`'s first non-`Serializable` superclass `A` is not a >> valid operation per the VM specification. VM special cases the classes >> generated for reflection to skip verification for the constructors generated >> for serialization and externalization. This change will allow such VM hack >> to be removed. >> >> A `jdk.reflect.useOldSerializableConstructor` system property can be set to >> use the old implementation in case if customers run into any compatibility >> issue. I expect this change has very low compatibility risk. This system >> property is undocumented and will be removed in a future release. > > src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 3534: > >> 3532: * This method is only used to implement the unsupported >> 3533: * >> sun.reflect.ReflectionFactory::newConstructorForSerialization API. >> 3534: */ > > The following is probably more correct: > Suggestion: > > /** > * Produces a method handle that is capable of creating instances of > the given class > * and instantiated by the given constructor. No security manager > check. > * > * This method is used to implement serialization and the unsupported > * sun.reflect.ReflectionFactory::newConstructorForSerialization API. > */ What are you referring to "to implement serialization"? It's only used by ReflectionFactory. - PR Review Comment: https://git.openjdk.org/jdk/pull/15600#discussion_r1317750757
Re: RFR: 8315810: Reimplement sun.reflect.ReflectionFactory::newConstructorForSerialization with method handles
On Wed, 6 Sep 2023 18:34:29 GMT, Mandy Chung wrote: > This reimplements > `sun.reflect.ReflectionFactory::newConstructorForSerialization` with method > handles. > > This API currently generates the bytecode which fails the verification > because `new C; invokespecial A()` where the given class `C` and invoke a > no-arg constructor of `C`'s first non-`Serializable` superclass `A` is not a > valid operation per the VM specification. VM special cases the classes > generated for reflection to skip verification for the constructors generated > for serialization and externalization. This change will allow such VM hack > to be removed. > > A `jdk.reflect.useOldSerializableConstructor` system property can be set to > use the old implementation in case if customers run into any compatibility > issue. I expect this change has very low compatibility risk. This system > property is undocumented and will be removed in a future release. src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 3534: > 3532: * This method is only used to implement the unsupported > 3533: * > sun.reflect.ReflectionFactory::newConstructorForSerialization API. > 3534: */ The following is probably more correct: Suggestion: /** * Produces a method handle that is capable of creating instances of the given class * and instantiated by the given constructor. No security manager check. * * This method is used to implement serialization and the unsupported * sun.reflect.ReflectionFactory::newConstructorForSerialization API. */ - PR Review Comment: https://git.openjdk.org/jdk/pull/15600#discussion_r1317735916
Re: RFR: 8315810: Reimplement sun.reflect.ReflectionFactory::newConstructorForSerialization with method handles
On Wed, 6 Sep 2023 18:34:29 GMT, Mandy Chung wrote: > This reimplements > `sun.reflect.ReflectionFactory::newConstructorForSerialization` with method > handles. > > This API currently generates the bytecode which fails the verification > because `new C; invokespecial A()` where the given class `C` and invoke a > no-arg constructor of `C`'s first non-`Serializable` superclass `A` is not a > valid operation per the VM specification. VM special cases the classes > generated for reflection to skip verification for the constructors generated > for serialization and externalization. This change will allow such VM hack > to be removed. > > A `jdk.reflect.useOldSerializableConstructor` system property can be set to > use the old implementation in case if customers run into any compatibility > issue. I expect this change has very low compatibility risk. This system > property is undocumented and will be removed in a future release. See also: [JDK‑8307575] ([GH‑13853]) [JDK‑8307575]: https://bugs.openjdk.org/browse/JDK-8307575 "[JDK‑8307575] Migrate the serialization constructor accessors to Method Handles" [GH‑13853]: https://github.com/openjdk/jdk/pull/13853 - PR Comment: https://git.openjdk.org/jdk/pull/15600#issuecomment-1708962019
