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

8284942: Proxy building can just iterate superinterfaces once #8281

Closed
wants to merge 8 commits into from

Conversation

liach
Copy link
Contributor

@liach liach commented Apr 18, 2022

Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass, the interfaces are iterated twice. The two passes can be merged into one, yielding the whole proxy definition context (module, package, whether there's package-private interface) when determining the module.

Split from #8278. Helpful for moving proxies to hidden classes, but is a good cleanup on its own.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8284942: Proxy building can just iterate superinterfaces once

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8281/head:pull/8281
$ git checkout pull/8281

Update a local copy of the PR:
$ git checkout pull/8281
$ git pull https://git.openjdk.java.net/jdk pull/8281/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8281

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8281.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 18, 2022

👋 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 openjdk bot added the rfr label Apr 18, 2022
@openjdk
Copy link

openjdk bot commented Apr 18, 2022

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

  • core-libs

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

@openjdk openjdk bot added the core-libs label Apr 18, 2022
@mlbridge
Copy link

mlbridge bot commented Apr 18, 2022

Webrevs

@bridgekeeper
Copy link

bridgekeeper bot commented May 19, 2022

@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@liach
Copy link
Contributor Author

liach commented May 19, 2022

Can anyone, such as Mandy, review this cleanup?

String proxyPkg = null; // package to define proxy class in
int accessFlags = Modifier.PUBLIC | Modifier.FINAL;
boolean nonExported = false;
private record ProxyContext(Module module, String pkg, boolean packagePrivate) {}
Copy link
Member

@mlchung mlchung May 19, 2022

Choose a reason for hiding this comment

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

This represents the context for a proxy class. It seems ProxyClassContext would be clearer. I suggest the context to have the accessFlags for a proxy class rather than the boolean whether it's package private. The constructor should check that it must be 0 or Modifier.PUBLIC. FINAL will be added to the access flags when it generates the proxy class.

private static Module mapToModule(ClassLoader loader,
List<Class<?>> interfaces,
Set<Class<?>> refTypes) {
private static ProxyContext setupContext(ClassLoader loader,
Copy link
Member

@mlchung mlchung May 19, 2022

Choose a reason for hiding this comment

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

Perhaps name this method proxyClassContext that returns ProxyClassContext.

}

// All proxy interfaces are public. So maps to a dynamic proxy module
// and add reads edge and qualified exports, if necessary
Module targetModule = getDynamicModule(loader);
var context = getDynamicModuleContext(loader, nonExported);
Copy link
Member

@mlchung mlchung May 19, 2022

Choose a reason for hiding this comment

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

I suggest to keep the getDynamicModule method as creating a dynamic module of a given class loader is a clear function. The context can be created in this method body.

            var targetModule = getDynamicModule(loader);
            var pkgName = nonExported ? PROXY_PACKAGE_PREFIX + '.' + targetModule.getName()
                                      : targetModule.getName();
            var context = new ProxyClassContext(targetModule, pkgName, Modifier.PUBLIC);

Copy link
Member

@mlchung mlchung left a comment

Looks good with a couple comments.

@@ -555,6 +531,7 @@ private static Class<?> defineProxyClass(Module m, List<Class<?>> interfaces) {
/*
* Generate the specified proxy class.
*/
accessFlags |= Modifier.FINAL;
Copy link
Member

@mlchung mlchung May 20, 2022

Choose a reason for hiding this comment

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

It can be inlined in the call to generateProxyClass:

            byte[] proxyClassFile = ProxyGenerator.generateProxyClass(loader, proxyName, interfaces, accessFlags | Modifier.FINAL);

if (!m.isNamed())
throw new InternalError("unnamed module: " + m);
proxyPkg = nonExported ? PROXY_PACKAGE_PREFIX + "." + m.getName()
: m.getName();
} else if (proxyPkg.isEmpty() && m.isNamed()) {
Copy link
Member

@mlchung mlchung May 20, 2022

Choose a reason for hiding this comment

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

With the refactoring, it may be easier to understand to make this check for both public and non-public access (i.e. drop the else) for clarity - a named module can't have unnamed package.

Copy link
Contributor Author

@liach liach May 20, 2022

Choose a reason for hiding this comment

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

a named module can't have unnamed package

Since this is mandated by the Java Language Specification 7.4.2, I am tempted to change the thrown exception to InternalError, but I cannot find any restriction in the JVM Specification that prevents declaring a package-private superinterface in the unnamed package of a named module. If we can find a reference in the JVM Specification, I'm more than glad to refer to that in a comment and throw InternalError instead.

if (proxyPkg.isEmpty() && m.isNamed()) {
// Per JLS 7.4.2, unnamed package can only exist in unnamed modules.
// This means a package-private superinterface exist in the unnamed
// package of a named module.
Copy link
Member

@mlchung mlchung May 26, 2022

Choose a reason for hiding this comment

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

With this patch, I think line 505-517 can turn into assert or internal error as you suggested. Probably can be moved to the ProxyClassContext constructor too.

int accessFlags = Modifier.PUBLIC | Modifier.FINAL;
boolean nonExported = false;
private record ProxyClassContext(Module module, String pkg, int accessFlags) {
private ProxyClassContext {
Copy link
Member

@mlchung mlchung May 26, 2022

Choose a reason for hiding this comment

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

We should validate the accessFlags value as it must be 0 or PUBLIC.

}

if (!module.isOpen(pkg, Proxy.class.getModule())) {
// Required for default method invocation
Copy link
Member

@mlchung mlchung May 26, 2022

Choose a reason for hiding this comment

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

Is this comment true?

The module of the proxy class opens the package to java.base if the proxy interface is non-public in a named module or if all proxy interfaces are public but a proxy interface is not unconditionally exported.

Copy link
Contributor Author

@liach liach May 26, 2022

Choose a reason for hiding this comment

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

The original check and Modules.addOpen calls were added in 8159476, when the invokeDefault support was added.

See: 56b15fb#diff-c15cc774a95bac204c294b9ca8e20332c81904e506e16bb7d5a82d1c30cced17R667, or #313

Copy link
Member

@mlchung mlchung May 26, 2022

Choose a reason for hiding this comment

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

Ah, I see. That assert was added in PR #313 as a clarification to the current proxy implementation but not required by the InvocationHandle::invokeDefault. It could have been added before the default method support.

Copy link
Contributor Author

@liach liach May 26, 2022

Choose a reason for hiding this comment

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

It appears to me that it's required by proxyClassLookup to perform reflective access on the dynamic module or the module where the package-private interface is located.

String proxyPkg = null; // package to define proxy class in
int accessFlags = Modifier.PUBLIC | Modifier.FINAL;
boolean nonExported = false;
private record ProxyClassContext(Module module, String pkg, int accessFlags) {
Copy link
Member

@mlchung mlchung May 26, 2022

Choose a reason for hiding this comment

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

Suggested change
private record ProxyClassContext(Module module, String pkg, int accessFlags) {
private record ProxyClassContext(Module module, String packageName, int accessFlags) {

@liach
Copy link
Contributor Author

liach commented May 31, 2022

@mlchung Would you review again that I've updated per your suggestions? In addition, I've moved all proxy class context-related validation into the record compact constructor.

Copy link
Member

@mlchung mlchung left a comment

Looks good. Thanks.

@openjdk
Copy link

openjdk bot commented May 31, 2022

@liach 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:

8284942: Proxy building can just iterate superinterfaces once

Reviewed-by: mchung

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 49 new commits pushed to the master branch:

  • 8db5247: 8282771: Create test case for JDK-8262981
  • cfdbde1: 8282778: Create a regression test for JDK-4699544
  • 8df5f10: 8282857: Create a regression test for JDK-4702690
  • e0382c5: 8285401: Proxy class initializer should use 3-arg Class.forName to avoid unnecessary class initialization
  • 37a5130: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo
  • d5b6c7b: 8287384: Speed up jdk.test.lib.util.ForceGC
  • 6f6486e: 8284960: Integration of JEP 426: Vector API (Fourth Incubator)
  • 171a7cd: 8286895: InternalError: Exception during analyze
  • 1b44f6c: 8287526: java/nio/channels/FileChannel/LargeMapTest.java fails on 32-bit systems
  • 7ef6993: 8276836: Error in javac caused by switch expression without result expressions: Internal error: stack sim error
  • ... and 39 more: https://git.openjdk.java.net/jdk/compare/7cb368b34df97614f0d208b8b0b93aa714a49282...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mlchung) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label May 31, 2022
@liach
Copy link
Contributor Author

liach commented May 31, 2022

/integrate

@openjdk openjdk bot added the sponsor label May 31, 2022
@openjdk
Copy link

openjdk bot commented May 31, 2022

@liach
Your change (at version 9d7ebaa) is now ready to be sponsored by a Committer.

@liach
Copy link
Contributor Author

liach commented May 31, 2022

@mlchung Would you mind sponsoring this patch?

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 29, 2022

@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@liach
Copy link
Contributor Author

liach commented Jun 29, 2022

Would someone review or sponsor this mino improvement?

@jaikiran
Copy link
Member

jaikiran commented Jun 29, 2022

Would someone review or sponsor this mino improvement?

Hello @liach, I see that Mandy has already reviewed and approved this PR and there hasn't been any change in this PR after that (which is a good thing). I'll run some tests internally today and if that goes fine, then I'll go ahead and sponsor this.

@jaikiran
Copy link
Member

jaikiran commented Jun 29, 2022

Locally, I merged the latest master branch with this PR and then ran tier1, tier2 and tier3 tests. The tests passed without any related failures. I'll go ahead and sponsor this now.

@jaikiran
Copy link
Member

jaikiran commented Jun 29, 2022

/sponsor

@openjdk
Copy link

openjdk bot commented Jun 29, 2022

Going to push as commit 0709a6a.
Since your change was applied there have been 461 commits pushed to the master branch:

  • 2961b7e: 8285364: Remove REF_ enum for java.lang.ref.Reference
  • 167ce4d: 8289421: No-PCH build for Minimal VM was broken by JDK-8287001
  • 108cd69: 8283726: x86_64 intrinsics for compareUnsigned method in Integer and Long
  • b96ba19: 8289182: NMT: MemTracker::baseline should return void
  • 779b4e1: 8287001: Add warning message when fail to load hsdis libraries
  • 910053b: 8280235: Deprecated flag FlightRecorder missing from VMDeprecatedOptions test
  • 7b3bf97: 8289401: Add dump output to TestRawRSACipher.java
  • 86dc760: Merge
  • 1504804: 8289398: ProblemList jdk/jfr/api/consumer/recordingstream/TestOnEvent.java on linux-x64 again
  • 9b7805e: 8289069: Very slow C1 arraycopy jcstress tests after JDK-8279886
  • ... and 451 more: https://git.openjdk.org/jdk/compare/7cb368b34df97614f0d208b8b0b93aa714a49282...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label Jun 29, 2022
@openjdk openjdk bot closed this Jun 29, 2022
@openjdk openjdk bot removed ready rfr sponsor labels Jun 29, 2022
@openjdk
Copy link

openjdk bot commented Jun 29, 2022

@jaikiran @liach Pushed as commit 0709a6a.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs integrated
3 participants