Re: RFR: 8304425: ClassHierarchyResolver from Reflection [v8]
On Fri, 2 Jun 2023 03:36:35 GMT, Chen Liang wrote: >> Add API to explore Class Hierarchy with a `ClassLoader` or a `Lookup` with >> proper privileges, with tests. >> >> This addition is useful in case classes at runtime are not loaded from the >> system class loader, such as Proxy. This is also useful to APIs that >> generate bytecode with a `Lookup` object, such as a custom >> single-abstract-method class implementations from a method handle. >> >> See >> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000249.html >> as well. >> >> Current questions, which I wish to discuss with @asotona: >> 1. Should the resolver fail fast on `IllegalAccessException` from the >> lookup? This usually indicates the hierarchy resolver is set up improperly, >> and proceeding may simply yield verification errors in class loading that >> are hard to track. For bytecode-generating APIs, throwing access errors for >> the Lookup eagerly is also more preferable than later silent generation >> failure. >> 2. Whether the default resolver should be reading from jrt alone, reflection >> alone, or jrt then reflection. I personally believe reflection alone is more >> reliable, for classes may redefined with instrumentation or jfr, which may >> not be reflected in the system resources. >> 3. In addition, I don't think chaining system class loader reflection after >> system resource retrieval is really meaningful: is there any case where >> reflection always works while the system resource retrieval always fails? > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 12 commits: > > - 1. Moved the default resolver to a static method, in anticipation of > future changes >2. Removed SecurityManager related content >3. Changed ClassHierarchyInfo into an interface >4. Moved caching method from static to instance method > - Merge branch 'master' into hierarchy-resolve > - rename to ofClassLoading/ofResourceParsing >convert the default class provider to bypass security manager restrictions > - Merge branch 'master' into hierarchy-resolve > - Merge branch 'master' into hierarchy-resolve > - Test both cached and uncached resolvers > - Update the class hierarchy resolver api as per mailing list last week > - Merge branch 'master' into hierarchy-resolve > - Update > src/java.base/share/classes/jdk/internal/classfile/impl/ClassHierarchyImpl.java > >Co-authored-by: Andrey Turbanov > - Make lookup based resolver throw on illegal access eagerly > - ... and 2 more: https://git.openjdk.org/jdk/compare/101bf229...9e9079fb Done. I've synchronized the global static cache for now, and thanks for the review! - PR Comment: https://git.openjdk.org/jdk/pull/13082#issuecomment-1580904371
Re: RFR: 8304425: ClassHierarchyResolver from Reflection [v8]
On Fri, 2 Jun 2023 03:36:35 GMT, Chen Liang wrote:
>> Add API to explore Class Hierarchy with a `ClassLoader` or a `Lookup` with
>> proper privileges, with tests.
>>
>> This addition is useful in case classes at runtime are not loaded from the
>> system class loader, such as Proxy. This is also useful to APIs that
>> generate bytecode with a `Lookup` object, such as a custom
>> single-abstract-method class implementations from a method handle.
>>
>> See
>> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000249.html
>> as well.
>>
>> Current questions, which I wish to discuss with @asotona:
>> 1. Should the resolver fail fast on `IllegalAccessException` from the
>> lookup? This usually indicates the hierarchy resolver is set up improperly,
>> and proceeding may simply yield verification errors in class loading that
>> are hard to track. For bytecode-generating APIs, throwing access errors for
>> the Lookup eagerly is also more preferable than later silent generation
>> failure.
>> 2. Whether the default resolver should be reading from jrt alone, reflection
>> alone, or jrt then reflection. I personally believe reflection alone is more
>> reliable, for classes may redefined with instrumentation or jfr, which may
>> not be reflected in the system resources.
>> 3. In addition, I don't think chaining system class loader reflection after
>> system resource retrieval is really meaningful: is there any case where
>> reflection always works while the system resource retrieval always fails?
>
> Chen Liang has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 12 commits:
>
> - 1. Moved the default resolver to a static method, in anticipation of
> future changes
>2. Removed SecurityManager related content
>3. Changed ClassHierarchyInfo into an interface
>4. Moved caching method from static to instance method
> - Merge branch 'master' into hierarchy-resolve
> - rename to ofClassLoading/ofResourceParsing
>convert the default class provider to bypass security manager restrictions
> - Merge branch 'master' into hierarchy-resolve
> - Merge branch 'master' into hierarchy-resolve
> - Test both cached and uncached resolvers
> - Update the class hierarchy resolver api as per mailing list last week
> - Merge branch 'master' into hierarchy-resolve
> - Update
> src/java.base/share/classes/jdk/internal/classfile/impl/ClassHierarchyImpl.java
>
>Co-authored-by: Andrey Turbanov
> - Make lookup based resolver throw on illegal access eagerly
> - ... and 2 more: https://git.openjdk.org/jdk/compare/101bf229...9e9079fb
Globas static cache needs to be synchronised If this PR goes in before #14180
Or a `defaultResolver()` should create a new cache instance (and document it is
thread-unsafe) if it goes after #14180
src/java.base/share/classes/jdk/internal/classfile/ClassHierarchyResolver.java
line 57:
> 55: */
> 56: static ClassHierarchyResolver defaultResolver() {
> 57: return ClassHierarchyImpl.DEFAULT_RESOLVER;
defaultResolver() delegates to a thread-unsafe global static instance of cached
resolver.
src/java.base/share/classes/jdk/internal/classfile/impl/ClassHierarchyImpl.java
line 130:
> 128: @Override
> 129: public ClassHierarchyInfo getClassInfo(ClassDesc classDesc) {
> 130: var ret = resolvedCache.computeIfAbsent(classDesc, new
> Function<>() {
couldn't this function be cached?
test/jdk/jdk/classfile/ClassHierarchyInfoTest.java line 34:
> 32: * java.base/jdk.internal.classfile.impl
> 33: * java.base/jdk.internal.classfile.impl.verifier
> 34: * java.base/jdk.internal.classfile.java.lang.constant
jdk.internal.classfile.java.lang.constant package has been removed
-
Changes requested by asotona (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13082#pullrequestreview-1466930855
PR Review Comment: https://git.openjdk.org/jdk/pull/13082#discussion_r1221124185
PR Review Comment: https://git.openjdk.org/jdk/pull/13082#discussion_r1221102574
PR Review Comment: https://git.openjdk.org/jdk/pull/13082#discussion_r1221108184
Re: RFR: 8304425: ClassHierarchyResolver from Reflection [v8]
On Fri, 2 Jun 2023 03:36:35 GMT, Chen Liang wrote: >> Add API to explore Class Hierarchy with a `ClassLoader` or a `Lookup` with >> proper privileges, with tests. >> >> This addition is useful in case classes at runtime are not loaded from the >> system class loader, such as Proxy. This is also useful to APIs that >> generate bytecode with a `Lookup` object, such as a custom >> single-abstract-method class implementations from a method handle. >> >> See >> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000249.html >> as well. >> >> Current questions, which I wish to discuss with @asotona: >> 1. Should the resolver fail fast on `IllegalAccessException` from the >> lookup? This usually indicates the hierarchy resolver is set up improperly, >> and proceeding may simply yield verification errors in class loading that >> are hard to track. For bytecode-generating APIs, throwing access errors for >> the Lookup eagerly is also more preferable than later silent generation >> failure. >> 2. Whether the default resolver should be reading from jrt alone, reflection >> alone, or jrt then reflection. I personally believe reflection alone is more >> reliable, for classes may redefined with instrumentation or jfr, which may >> not be reflected in the system resources. >> 3. In addition, I don't think chaining system class loader reflection after >> system resource retrieval is really meaningful: is there any case where >> reflection always works while the system resource retrieval always fails? > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 12 commits: > > - 1. Moved the default resolver to a static method, in anticipation of > future changes >2. Removed SecurityManager related content >3. Changed ClassHierarchyInfo into an interface >4. Moved caching method from static to instance method > - Merge branch 'master' into hierarchy-resolve > - rename to ofClassLoading/ofResourceParsing >convert the default class provider to bypass security manager restrictions > - Merge branch 'master' into hierarchy-resolve > - Merge branch 'master' into hierarchy-resolve > - Test both cached and uncached resolvers > - Update the class hierarchy resolver api as per mailing list last week > - Merge branch 'master' into hierarchy-resolve > - Update > src/java.base/share/classes/jdk/internal/classfile/impl/ClassHierarchyImpl.java > >Co-authored-by: Andrey Turbanov > - Make lookup based resolver throw on illegal access eagerly > - ... and 2 more: https://git.openjdk.org/jdk/compare/101bf229...9e9079fb A brief summary of the latest update: 1. I've moved to implement all 3 API changes to ClassHierarchyResolver discussed on classfile-api-dev 2. I've dropped SecurityManager content; if they are needed by another patch, those patches can add on their own. This will be in conflict with the Classfile object update; I can update accordingly if that one is integrated first. - PR Comment: https://git.openjdk.org/jdk/pull/13082#issuecomment-1573096991
Re: RFR: 8304425: ClassHierarchyResolver from Reflection [v8]
> Add API to explore Class Hierarchy with a `ClassLoader` or a `Lookup` with > proper privileges, with tests. > > This addition is useful in case classes at runtime are not loaded from the > system class loader, such as Proxy. This is also useful to APIs that generate > bytecode with a `Lookup` object, such as a custom single-abstract-method > class implementations from a method handle. > > See > https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000249.html > as well. > > Current questions, which I wish to discuss with @asotona: > 1. Should the resolver fail fast on `IllegalAccessException` from the lookup? > This usually indicates the hierarchy resolver is set up improperly, and > proceeding may simply yield verification errors in class loading that are > hard to track. For bytecode-generating APIs, throwing access errors for the > Lookup eagerly is also more preferable than later silent generation failure. > 2. Whether the default resolver should be reading from jrt alone, reflection > alone, or jrt then reflection. I personally believe reflection alone is more > reliable, for classes may redefined with instrumentation or jfr, which may > not be reflected in the system resources. > 3. In addition, I don't think chaining system class loader reflection after > system resource retrieval is really meaningful: is there any case where > reflection always works while the system resource retrieval always fails? Chen Liang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 12 commits: - 1. Moved the default resolver to a static method, in anticipation of future changes 2. Removed SecurityManager related content 3. Changed ClassHierarchyInfo into an interface 4. Moved caching method from static to instance method - Merge branch 'master' into hierarchy-resolve - rename to ofClassLoading/ofResourceParsing convert the default class provider to bypass security manager restrictions - Merge branch 'master' into hierarchy-resolve - Merge branch 'master' into hierarchy-resolve - Test both cached and uncached resolvers - Update the class hierarchy resolver api as per mailing list last week - Merge branch 'master' into hierarchy-resolve - Update src/java.base/share/classes/jdk/internal/classfile/impl/ClassHierarchyImpl.java Co-authored-by: Andrey Turbanov - Make lookup based resolver throw on illegal access eagerly - ... and 2 more: https://git.openjdk.org/jdk/compare/101bf229...9e9079fb - Changes: https://git.openjdk.org/jdk/pull/13082/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13082&range=07 Stats: 388 lines in 8 files changed: 294 ins; 29 del; 65 mod Patch: https://git.openjdk.org/jdk/pull/13082.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13082/head:pull/13082 PR: https://git.openjdk.org/jdk/pull/13082
