Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8242888: Convert dynamic proxy to hidden classes #19356

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

liach
Copy link
Member

@liach liach commented May 23, 2024

Please review this change that convert dynamic proxies implementations to hidden classes, intended to target JDK 24.

Summary:

  1. Adds new implementation while preserving the old implementation behind -Djdk.reflect.useLegacyProxyImpl=true in case there are compatibility issues.
  2. ClassLoader.defineClass0 takes a ClassLoader instance but discards it in native code; I updated native code to reuse that ClassLoader for Proxy support.
  3. ProxyGenerator changes mainly involve using Class data to pass Method list (accessed in a single condy) and removal of obsolete setup code generation.

Testing: tier1 and tier2 have no related failures.

Comment: Since #8278, Proxy has been converted to ClassFile API, and infrastructure has changed; now, the migration to hidden classes is much cleaner and has less impact, such as preserving ProtectionDomain and dynamic module without "anchor classes", and avoiding java.lang.invoke package.


Progress

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

Issues

  • JDK-8242888: Convert dynamic proxy to hidden classes (Enhancement - P4)
  • JDK-8332770: Convert dynamic proxy to hidden classes (CSR)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19356

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented May 23, 2024

👋 Welcome back liach! 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.

@openjdk
Copy link

openjdk bot commented May 23, 2024

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@openjdk openjdk bot added the rfr Pull request is ready for review label May 23, 2024
@openjdk
Copy link

openjdk bot commented May 23, 2024

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

  • core-libs
  • hotspot
  • kulla

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.

@openjdk openjdk bot added hotspot hotspot-dev@openjdk.org core-libs core-libs-dev@openjdk.org kulla kulla-dev@openjdk.org labels May 23, 2024
@mlbridge
Copy link

mlbridge bot commented May 23, 2024

Webrevs

@AlanBateman
Copy link
Contributor

/csr

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label May 23, 2024
@openjdk
Copy link

openjdk bot commented May 23, 2024

@AlanBateman has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@liach please create a CSR request for issue JDK-8242888 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

@AlanBateman
Copy link
Contributor

There are compatibility concerns and behavioural differences that will require significant effort to consider before doing this. This is the reason that this one has been kicked down the road several times.

@liach
Copy link
Member Author

liach commented May 23, 2024

A CSR targeting 24 describing the compatibility concerns and behavioral differences is here, somehow not linked by skara: https://bugs.openjdk.org/browse/JDK-8332770
The incompatibilities were much greater in the previous iterations of this issue, such as in dynamic modules, serialization, and in proxy class protection domain. Now these aspects are addressed by this patch, the only real one left is the change in stack trace. Feel free to raise other incompatibilities you have discovered.

@AlanBateman
Copy link
Contributor

AlanBateman commented May 23, 2024

A CSR targeting 24 describing the compatibility concerns and behavioral differences is here, somehow not linked by skara: https://bugs.openjdk.org/browse/JDK-8332770 The incompatibilities were much greater in the previous iterations of this issue, such as in dynamic modules, serialization, and in proxy class protection domain. Now these aspects are addressed by this patch, the only real one left is the change in stack trace. Feel free to raise other incompatibilities you have discovered.

Thanks for starting a CSR. The CSR can't be low risk, it's medium at least, maybe high. If we are doing this change then doing it early in a release and putting into effort to outreach to frameworks will be important.

@liach
Copy link
Member Author

liach commented May 23, 2024

I have updated the compatibility risk description of the CSR.

My CSR proposes to allow dynamic unloading of the proxy implementation classes, but currently it's not implemented as they are strongly referenced in the ClassLoaderValue caches. Should I implement dynamic unloading suggested in the CSR in this patch, or should I do it later?

@AlanBateman
Copy link
Contributor

I have updated the compatibility risk description of the CSR.

My CSR proposes to allow dynamic unloading of the proxy implementation classes, but currently it's not implemented as they are strongly referenced in the ClassLoaderValue caches. Should I implement dynamic unloading suggested in the CSR in this patch, or should I do it later?

I think the main compatibility concern is going to be that hidden classes don't have a binary name so we have to get a sense as to whether there are frameworks that do anything with the class name and Class.forName.

I suspect the work will also mean looking at cases where agents are somehow instrumenting proxy class (hidden classes are not modifiable). In the JDK 8 time frame we had to back out a change in this area due to one of the mocking tools filtering by class name and trying to redefine proxy classes.

@mlbridge
Copy link

mlbridge bot commented May 23, 2024

Mailing list message from Remi Forax on kulla-dev:

----- Original Message -----

From: "Chen Liang" <liach at openjdk.org>
To: "core-libs-dev" <core-libs-dev at openjdk.org>, "hotspot-dev" <hotspot-dev at openjdk.org>, kulla-dev at openjdk.org
Sent: Thursday, May 23, 2024 1:28:01 PM
Subject: Re: RFR: 8242888: Convert dynamic proxy to hidden classes

On Thu, 23 May 2024 03:28:30 GMT, Chen Liang <liach at openjdk.org> wrote:

Please review this change that convert dynamic proxies implementations to hidden
classes, intended to target JDK 24.

Summary:
1. Adds new implementation while preserving the old implementation behind
`-Djdk.reflect.useLegacyProxyImpl=true` in case there are compatibility issues.
2. ClassLoader.defineClass0 takes a ClassLoader instance but discards it in
native code; I updated native code to reuse that ClassLoader for Proxy support.
3. ProxyGenerator changes mainly involve using Class data to pass Method list
(accessed in a single condy) and removal of obsolete setup code generation.

Testing: tier1 and tier2 have no related failures.

Comment: Since #8278, Proxy has been converted to ClassFile API, and
infrastructure has changed; now, the migration to hidden classes is much
cleaner and has less impact, such as preserving ProtectionDomain and dynamic
module without "anchor classes", and avoiding java.lang.invoke package.

A CSR targeting 24 describing the compatibility concerns and behavioral
differences is here, somehow not linked by skara:
https://bugs.openjdk.org/browse/JDK-8332770
The incompatibilities were much greater in the previous iterations of this
issue, such as in dynamic modules, serialization, and in proxy class protection
domain. Now these aspects are addressed by this patch, the only real one left
is the change in stack trace. Feel free to raise other incompatibilities you
have discovered.

I wonder if instead of using hidden classes, we should not use usual named classes and add a new Lookup.defineClass() that takes a classData as parameter.
This will solve the both the problem of the stacktrace and the problem of the roundtrip proxyClass != Class.forName(proxyClass.getName()).

R?mi

@mlbridge
Copy link

mlbridge bot commented May 23, 2024

Mailing list message from Chen Liang on core-libs-dev:

Hmm, I think Proxy being hidden in stacktraces might be an advantage; the
same happens for lambdas.
The main advantage of hidden classes compared to an explicit class with
classData is that it supports flexible unloading, which might be useful for
Proxy.
I still believe the flexible unloading advantage justifies the migration to
hidden classes.

Chen

On Thu, May 23, 2024 at 6:43?AM Remi Forax <forax at univ-mlv.fr> wrote:

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20240523/4c32512f/attachment-0001.htm>

@mlbridge
Copy link

mlbridge bot commented May 23, 2024

Mailing list message from forax at univ-mlv.fr on core-libs-dev:

From: "-" <liangchenblue at gmail.com>
To: "Remi Forax" <forax at univ-mlv.fr>
Cc: "Chen Liang" <liach at openjdk.org>, "core-libs-dev"
<core-libs-dev at openjdk.org>, "hotspot-dev" <hotspot-dev at openjdk.org>,
"kulla-dev" <kulla-dev at openjdk.org>
Sent: Thursday, May 23, 2024 2:56:58 PM
Subject: Re: RFR: 8242888: Convert dynamic proxy to hidden classes

Hmm, I think Proxy being hidden in stacktraces might be an advantage; the same
happens for lambdas.
The main advantage of hidden classes compared to an explicit class with
classData is that it supports flexible unloading, which might be useful for
Proxy.

Flexible unloading has a high cost in term of memory, the class + methods, etc need their own metaspace.
While on paper it seems great, I've my doubt that it's a good idea to use that option for proxies given that the Proxy API allows an umbounded number of proxy classes.
That's why lambda proxies does not use the flexible unloading anymore.

I still believe the flexible unloading advantage justifies the migration to
hidden classes.

Chen

R?mi

On Thu, May 23, 2024 at 6:43 AM Remi Forax < [ mailto:forax at univ-mlv.fr |
forax at univ-mlv.fr ] > wrote:

----- Original Message -----

From: "Chen Liang" < [ mailto:liach at openjdk.org | liach at openjdk.org ] >
To: "core-libs-dev" < [ mailto:core-libs-dev at openjdk.org |
core-libs-dev at openjdk.org ] >, "hotspot-dev" < [ mailto:hotspot-dev at openjdk.org
| hotspot-dev at openjdk.org ] >, [ mailto:kulla-dev at openjdk.org |
kulla-dev at openjdk.org ]
Sent: Thursday, May 23, 2024 1:28:01 PM
Subject: Re: RFR: 8242888: Convert dynamic proxy to hidden classes

On Thu, 23 May 2024 03:28:30 GMT, Chen Liang < [ mailto:liach at openjdk.org |
liach at openjdk.org ] > wrote:

Please review this change that convert dynamic proxies implementations to hidden
classes, intended to target JDK 24.

Summary:
1. Adds new implementation while preserving the old implementation behind
`-Djdk.reflect.useLegacyProxyImpl=true` in case there are compatibility issues.
2. ClassLoader.defineClass0 takes a ClassLoader instance but discards it in
native code; I updated native code to reuse that ClassLoader for Proxy support.
3. ProxyGenerator changes mainly involve using Class data to pass Method list
(accessed in a single condy) and removal of obsolete setup code generation.

Testing: tier1 and tier2 have no related failures.

Comment: Since #8278, Proxy has been converted to ClassFile API, and
infrastructure has changed; now, the migration to hidden classes is much
cleaner and has less impact, such as preserving ProtectionDomain and dynamic
module without "anchor classes", and avoiding java.lang.invoke package.

A CSR targeting 24 describing the compatibility concerns and behavioral
differences is here, somehow not linked by skara:
[ https://bugs.openjdk.org/browse/JDK-8332770 |
https://bugs.openjdk.org/browse/JDK-8332770 ]
The incompatibilities were much greater in the previous iterations of this
issue, such as in dynamic modules, serialization, and in proxy class protection
domain. Now these aspects are addressed by this patch, the only real one left
is the change in stack trace. Feel free to raise other incompatibilities you
have discovered.

I wonder if instead of using hidden classes, we should not use usual named
classes and add a new Lookup.defineClass() that takes a classData as parameter.
This will solve the both the problem of the stacktrace and the problem of the
roundtrip proxyClass != Class.forName(proxyClass.getName()).

R?mi

-------------

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20240523/926a8572/attachment.htm>

@liach
Copy link
Member Author

liach commented May 23, 2024

Hmm, actually, looking at the specs of the method again, does it imply that Proxy classes are never unloaded once defined in a ClassLoader, as seen in Proxy::getProxyClass:

If a proxy class for the same permutation of interfaces has already been defined by the class loader, then the existing proxy class will be returned

If that's the case, Remi's suggestion on passing classdata to a non-hidden class might be better, and it seems to accomplish that in hotspot isn't too hard too.

@AlanBateman
Copy link
Contributor

Hmm, actually, looking at the specs of the method again, does it imply that Proxy classes are never unloaded once defined in a ClassLoader, as seen in Proxy::getProxyClass:

It's not specified, Proxy pre-dates hidden classes although its Proxy did require some changes to specify that it can't be a proxy to a hidden class. Given the getProxyClass is deprecated then it may be better to have it work the same way as it has always done.

If Proxy::newInstanceClass is changed to return an instance of a hidden class then spec changes are needed. Maybe too early to think about that now as there is a lot of analysis work required to do before going near code.

Copy link

@ExE-Boss ExE-Boss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useLegacyProxyImpl && !useOldSerializableConstructor would always be false when useOldSerializableConstructor is true, which is the opposite of what’s described in the CSR.

@liach liach marked this pull request as draft May 27, 2024 13:15
@liach
Copy link
Member Author

liach commented May 27, 2024

Will have to rework since this is in conflict with #19410. Converting into draft for now.

@openjdk openjdk bot removed the rfr Pull request is ready for review label May 27, 2024
@openjdk
Copy link

openjdk bot commented Jun 6, 2024

@liach this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout feature/hidden-proxy
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Jun 6, 2024
liach added 2 commits June 7, 2024 09:29
…hidden-proxy

# Conflicts:
#	src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java
@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Jun 7, 2024
@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org csr Pull request needs approved CSR before integration hotspot hotspot-dev@openjdk.org kulla kulla-dev@openjdk.org merge-conflict Pull request has merge conflict with target branch
3 participants