8315810: Reimplement sun.reflect.ReflectionFactory::newConstructorForSerialization with method handles by mlchung · Pull Request #15600 · openjdk/jdk

mlchung · 2023-09-06T18:34:29Z

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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Change requires CSR request JDK-8315811 to be approved
Commit message must refer to an issue

Issues

JDK-8315810: Reimplement sun.reflect.ReflectionFactory::newConstructorForSerialization with method handles (Enhancement - P3)
JDK-8315811: Reimplement sun.reflect.ReflectionFactory::newConstructorForSerialization with method handles (CSR)

Reviewers

Roger Riggs (@RogerRiggs - Reviewer)

Contributors

Chen Liang <liach@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15600/head:pull/15600
$ git checkout pull/15600

Update a local copy of the PR:
$ git checkout pull/15600
$ git pull https://git.openjdk.org/jdk.git pull/15600/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15600

View PR using the GUI difftool:
$ git pr show -t 15600

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15600.diff

Webrev

Link to Webrev Comment

…thod handle

bridgekeeper · 2023-09-06T18:35:30Z

👋 Welcome back mchung! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlchung · 2023-09-06T18:36:36Z

/contributor add liach

openjdk · 2023-09-06T18:37:22Z

@mlchung
Contributor Chen Liang <liach@openjdk.org> successfully added.

openjdk · 2023-09-06T18:38:14Z

@mlchung The following labels will be automatically applied to this pull request:

core-libs
security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlchung · 2023-09-06T18:40:36Z

/label remove security

openjdk · 2023-09-06T18:40:45Z

@mlchung
The security label was successfully removed.

mlbridge · 2023-09-06T18:41:53Z

Webrevs

ExE-Boss · 2023-09-06T19:24:54Z

See also: JDK‑8307575 (GH‑13853)

src/java.base/share/classes/java/lang/invoke/MethodHandles.java

src/java.base/share/classes/jdk/internal/reflect/MethodHandleAccessorFactory.java

turbanoff · 2023-09-07T08:50:07Z

src/java.base/share/classes/jdk/internal/reflect/DirectMethodHandleAccessor.java

        if (!declaringClass.isAssignableFrom(o.getClass())) {
-            throw new IllegalArgumentException("object is not an instance of declaring class");
+            throw new IllegalArgumentException("object of type " + o.getClass().getName()
+                    +  " is not an instance of " + declaringClass.getName());


Suggested change

+ " is not an instance of " + declaringClass.getName());

+ " is not an instance of " + declaringClass.getName());

…serializable-ctor

RogerRiggs · 2023-09-07T15:43:16Z

src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java

+                                             langReflectAccess.
+                                             getConstructorSlot(constructorToCall),
+                                             langReflectAccess.
+                                             getConstructorSignature(constructorToCall),
+                                             langReflectAccess.
+                                             getConstructorAnnotations(constructorToCall),
+                                             langReflectAccess.
+                                             getConstructorParameterAnnotations(constructorToCall));


This would be easier to read if either the line length or the indentation was modified.

RogerRiggs · 2023-09-07T15:44:45Z

src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java

+        return ctor;
    }

+


Extra blank line.

RogerRiggs · 2023-09-12T19:11:30Z

src/java.base/share/classes/java/lang/invoke/MethodHandles.java

+        private static boolean constructorInSuperclass(Class<?> decl, Constructor<?> ctor) {
+            if (decl == ctor.getDeclaringClass())
+                return true;
+
+            Class<?> cl = decl;
+            while ((cl = cl.getSuperclass()) != null) {
+                if (cl == ctor.getDeclaringClass()) {
+                    return true;
+                }
+            }
+            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.

RogerRiggs

LGTM

openjdk · 2023-09-13T19:56:21Z

@mlchung This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8315810: Reimplement sun.reflect.ReflectionFactory::newConstructorForSerialization with method handles

Co-authored-by: Chen Liang <liach@openjdk.org>
Reviewed-by: rriggs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 24 new commits pushed to the master branch:

3b0a6d2: 8314226: Series of colon-style fallthrough switch cases with guards compiled incorrectly
ff240a9: 8316087: Test SignedLoggerFinderTest.java is still failing
a731a24: 8315934: RISC-V: Disable conservative fences per vendor
b3dad24: 8316021: Serial: Remove unused Generation::post_compact
f9ab115: 8316050: Use hexadecimal encoding in MemorySegment::toString
f804f86: 8314612: TestUnorderedReduction.java fails with -XX:MaxVectorSize=32 and -XX:+AlignVector
f8df754: 8311207: Cleanup for Optimization for UUID.toString
fecd2fd: 8315898: Open source swing JMenu tests
bb6b3f2: 8315761: Open source few swing JList and JMenuBar tests
2d168c5: 8313202: MutexLocker should disallow null Mutexes
... and 14 more: https://git.openjdk.org/jdk/compare/d75d9774c806e4bf73caa69cd78c31a132e4c812...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

mlchung · 2023-09-14T16:09:59Z

/integrate

openjdk · 2023-09-14T16:10:57Z

Going to push as commit 5cea53d.
Since your change was applied there have been 43 commits pushed to the master branch:

eb1f67b: 8315937: Enable parallelism in vmTestbase/nsk/stress/numeric tests
a57b9da: 8316242: Opensource SwingGraphics manual test
3abe798: 8316115: Parallel: Fix -Wconversion warnings around NUMA node ID
83dca62: 8313638: Add test for dump of resolved references
cfa8901: 8315338: RISC-V: Change flags for stable extensions to non-experimental
edd454b: 8315766: Parallelize gc/stress/TestStressIHOPMultiThread.java test
de9b971: 8315933: Serial: Remove empty Generation::ensure_parsability
b48dbf6: 8316181: Move the fast locking implementation out of the .ad files
8f4dfc4: 8316164: Opensource JMenuBar manual test
33c62e4: 8316154: Opensource JTextArea manual tests
... and 33 more: https://git.openjdk.org/jdk/compare/d75d9774c806e4bf73caa69cd78c31a132e4c812...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-09-14T16:11:04Z

@mlchung Pushed as commit 5cea53d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Reimplement ReflectionFactory::newConstructorForSerialization with me…

e98b552

…thod handle

openjdk bot added the rfr Pull request is ready for review label Sep 6, 2023

openjdk bot added security security-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Sep 6, 2023

openjdk bot removed the security security-dev@openjdk.org label Sep 6, 2023

openjdk bot added the csr Pull request needs approved CSR before integration label Sep 6, 2023

mlchung changed the title ~~8315810: Reimplement ReflectionFactory::newConstructorForSerialization with method handles~~ 8315810: Reimplement sun.reflect.ReflectionFactory::newConstructorForSerialization with method handles Sep 6, 2023

ExE-Boss reviewed Sep 6, 2023

View reviewed changes

src/java.base/share/classes/java/lang/invoke/MethodHandles.java Show resolved Hide resolved

liach reviewed Sep 6, 2023

View reviewed changes

src/java.base/share/classes/jdk/internal/reflect/MethodHandleAccessorFactory.java Show resolved Hide resolved

turbanoff reviewed Sep 7, 2023

View reviewed changes

mlchung added 2 commits September 12, 2023 09:31

minor cleanup

2c731ee

Merge branch 'master' of https://github.com/openjdk/jdk into reflect-…

fb3bf59

…serializable-ctor

RogerRiggs reviewed Sep 12, 2023

View reviewed changes

review feedback

c4e39d1

RogerRiggs approved these changes Sep 13, 2023

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Sep 13, 2023

openjdk bot added the integrated Pull request has been integrated label Sep 14, 2023

openjdk bot closed this Sep 14, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 14, 2023

mlchung deleted the reflect-serializable-ctor branch October 4, 2023 21:08

	+ " is not an instance of " + declaringClass.getName());
	+ " is not an instance of " + declaringClass.getName());

Conversation

mlchung commented Sep 6, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Contributors

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Sep 6, 2023

Uh oh!

mlchung commented Sep 6, 2023

Uh oh!

openjdk bot commented Sep 6, 2023

Uh oh!

openjdk bot commented Sep 6, 2023

Uh oh!

mlchung commented Sep 6, 2023

Uh oh!

openjdk bot commented Sep 6, 2023

Uh oh!

mlbridge bot commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

ExE-Boss commented Sep 6, 2023

Uh oh!

Uh oh!

Uh oh!

turbanoff Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

RogerRiggs Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

RogerRiggs Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

RogerRiggs Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

mlchung Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

RogerRiggs left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Sep 13, 2023

Uh oh!

mlchung commented Sep 14, 2023

Uh oh!

openjdk bot commented Sep 14, 2023

Uh oh!

openjdk bot commented Sep 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

mlchung commented Sep 6, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 6, 2023 •

edited

Loading