Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v5]
On Thu, 26 May 2022 22:33:29 GMT, Mandy Chung wrote: >> The original check and `Modules.addOpen` calls were added in >> [8159476](https://bugs.openjdk.java.net/browse/JDK-8159746), when the >> `invokeDefault` support was added. >> >> See: >> https://github.com/openjdk/jdk/commit/56b15fbbcc7c04252f2712d859ff7b820b7c79ad#diff-c15cc774a95bac204c294b9ca8e20332c81904e506e16bb7d5a82d1c30cced17R667, >> or #313 > > 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/8281
Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v5]
On Thu, 26 May 2022 22:01:56 GMT, liach wrote:
>> src/java.base/share/classes/java/lang/reflect/Proxy.java line 513:
>>
>>> 511:
>>> 512: if (!module.isOpen(pkg, Proxy.class.getModule())) {
>>> 513: // Required for default method invocation
>>
>> 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.
>
> The original check and `Modules.addOpen` calls were added in
> [8159476](https://bugs.openjdk.java.net/browse/JDK-8159746), when the
> `invokeDefault` support was added.
>
> See:
> https://github.com/openjdk/jdk/commit/56b15fbbcc7c04252f2712d859ff7b820b7c79ad#diff-c15cc774a95bac204c294b9ca8e20332c81904e506e16bb7d5a82d1c30cced17R667,
> or #313
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.
-
PR: https://git.openjdk.java.net/jdk/pull/8281
Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v5]
On Thu, 26 May 2022 21:32:53 GMT, Mandy Chung wrote:
>> liach 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 seven additional commits
>> since the last revision:
>>
>> - Group proxy location validation all into the class context constructor
>> - Merge branch 'master' into fix/proxy-single-pass
>> - Update
>> - Updates suggested by mandy
>> - Merge branch 'master' into fix/proxy-single-pass
>> - Don't need to complexify module cache
>> - 8284942: Proxy building can just iterate superinterfaces once
>
> src/java.base/share/classes/java/lang/reflect/Proxy.java line 513:
>
>> 511:
>> 512: if (!module.isOpen(pkg, Proxy.class.getModule())) {
>> 513: // Required for default method invocation
>
> 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.
The original check and `Modules.addOpen` calls were added in
[8159476](https://bugs.openjdk.java.net/browse/JDK-8159746), when the
`invokeDefault` support was added.
See:
https://github.com/openjdk/jdk/commit/56b15fbbcc7c04252f2712d859ff7b820b7c79ad#diff-c15cc774a95bac204c294b9ca8e20332c81904e506e16bb7d5a82d1c30cced17R667,
or #313
-
PR: https://git.openjdk.java.net/jdk/pull/8281
Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v5]
On Thu, 26 May 2022 20:53:29 GMT, liach wrote:
>> 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.
>
> liach 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 seven additional commits since
> the last revision:
>
> - Group proxy location validation all into the class context constructor
> - Merge branch 'master' into fix/proxy-single-pass
> - Update
> - Updates suggested by mandy
> - Merge branch 'master' into fix/proxy-single-pass
> - Don't need to complexify module cache
> - 8284942: Proxy building can just iterate superinterfaces once
src/java.base/share/classes/java/lang/reflect/Proxy.java line 498:
> 496: new ClassLoaderValue<>();
> 497:
> 498: private record ProxyClassContext(Module module, String pkg, int
> accessFlags) {
Suggestion:
private record ProxyClassContext(Module module, String packageName, int
accessFlags) {
src/java.base/share/classes/java/lang/reflect/Proxy.java line 499:
> 497:
> 498: private record ProxyClassContext(Module module, String pkg, int
> accessFlags) {
> 499: private ProxyClassContext {
We should validate the `accessFlags` value as it must be 0 or `PUBLIC`.
src/java.base/share/classes/java/lang/reflect/Proxy.java line 513:
> 511:
> 512: if (!module.isOpen(pkg, Proxy.class.getModule())) {
> 513: // Required for default method invocation
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.
-
PR: https://git.openjdk.java.net/jdk/pull/8281
Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v5]
> 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. liach 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 seven additional commits since the last revision: - Group proxy location validation all into the class context constructor - Merge branch 'master' into fix/proxy-single-pass - Update - Updates suggested by mandy - Merge branch 'master' into fix/proxy-single-pass - Don't need to complexify module cache - 8284942: Proxy building can just iterate superinterfaces once - Changes: - all: https://git.openjdk.java.net/jdk/pull/8281/files - new: https://git.openjdk.java.net/jdk/pull/8281/files/26bde80b..20edc525 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8281=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8281=03-04 Stats: 37340 lines in 627 files changed: 8639 ins; 27112 del; 1589 mod Patch: https://git.openjdk.java.net/jdk/pull/8281.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8281/head:pull/8281 PR: https://git.openjdk.java.net/jdk/pull/8281
