Re: RFR: 8323058: Revisit j.l.classfile.CodeBuilder API surface [v2]
On Mon, 5 Feb 2024 17:23:54 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> removed CodeBuilder::newObject methods
>
> src/java.base/share/classes/java/lang/classfile/CodeBuilder.java line 507:
>
>> 505: * @return this builder
>> 506: */
>> 507: default CodeBuilder newObject(ClassEntry type) {
>
> The two `newObject` methods seem to fit in the pattern of methods that are
> being removed, since they don't differentiate sufficiently with the `new_`
> methods that defer to them.
Right, we can remove also the `newObject` and keep `new_` methods, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17282#discussion_r1478638191
Re: RFR: 8323058: Revisit j.l.classfile.CodeBuilder API surface [v2]
> `java.lang.classfile.CodeBuilder` contains more than 230 API methods. > Existing ClassFile API use cases proved the concept one big CodeBuilder is > comfortable. However there are some redundancies, glitches in the naming > conventions, some frequently used methods are hard to find and some methods > have low practical use. > > This patch revisits the `CodeBuilder` API methods and introduces some changes. > > For more details, please, visit the [CSR > ](https://bugs.openjdk.org/browse/JDK-8323067) > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: removed CodeBuilder::newObject methods - Changes: - all: https://git.openjdk.org/jdk/pull/17282/files - new: https://git.openjdk.org/jdk/pull/17282/files/d5491d36..150393c7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17282=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17282=00-01 Stats: 24 lines in 4 files changed: 0 ins; 19 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/17282.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17282/head:pull/17282 PR: https://git.openjdk.org/jdk/pull/17282
RFR: 8323058: Revisit j.l.classfile.CodeBuilder API surface
`java.lang.classfile.CodeBuilder` contains more than 230 API methods. Existing ClassFile API use cases proved the concept one big CodeBuilder is comfortable. However there are some redundancies, glitches in the naming conventions, some frequently used methods are hard to find and some methods have low practical use. This patch revisits the `CodeBuilder` API methods and introduces some changes. For more details, please, visit the [CSR ](https://bugs.openjdk.org/browse/JDK-8323067) Please review. Thank you, Adam - Commit messages: - Merge branch 'master' into JDK-8323058-CodeBuilder - extended CodeBuilder::conversion functionality - removed CodeBuilder::newPrimitiveArray, newReferenceArray, newMultidimensionalArray and operator methods - fixed tests - fixed jdk/classfile tests - fixed benchmarks - fixed jdk.jfr and jdk.jlink - fixed java.base - 8323058: Revisit j.l.classfile.CodeBuilder API surface Changes: https://git.openjdk.org/jdk/pull/17282/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17282=00 Issue: https://bugs.openjdk.org/browse/JDK-8323058 Stats: 876 lines in 43 files changed: 41 ins; 188 del; 647 mod Patch: https://git.openjdk.org/jdk/pull/17282.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17282/head:pull/17282 PR: https://git.openjdk.org/jdk/pull/17282
Re: RFR: 8294977: Convert test/jdk/java tests from ASM library to Classfile API [v11]
On Sun, 17 Dec 2023 23:11:10 GMT, Chen Liang wrote: >> Summaries: >> 1. A few recommendations about updating the constant API is made at >> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000233.html >> and I may update this patch shall the API changes be integrated before >> 2. One ASM library-specific test, `LambdaAsm` is removed. Others have their >> code generation infrastructure upgraded from ASM to Classfile API. >> 3. Most tests are included in tier1, but some are not: >> In `:jdk_io`: (tier2, part 2) >> >> test/jdk/java/io/Serializable/records/SerialPersistentFieldsTest.java >> test/jdk/java/io/Serializable/records/ProhibitedMethods.java >> test/jdk/java/io/Serializable/records/BadCanonicalCtrTest.java >> >> In `:jdk_instrument`: (tier 3) >> >> test/jdk/java/lang/instrument/RetransformAgent.java >> test/jdk/java/lang/instrument/NativeMethodPrefixAgent.java >> test/jdk/java/lang/instrument/asmlib/Instrumentor.java >> >> >> @asotona Would you mind reviewing? > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Fix the 2 failing tests and add notes Yes, CodeBuilder method renames will require some refactoring. This is good. - Marked as reviewed by asotona (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13009#pullrequestreview-1826743185
Re: RFR: 8323183: ClassFile API performance improvements [v7]
> ClassFile API performance related improvements have been separated from > #17121 into this PR. > > These improvements are important to minimize performance regression of > 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the > Classfile API to generate proxy classes #17121 > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/jdk/internal/classfile/impl/StackMapDecoder.java Co-authored-by: Andrey Turbanov - Changes: - all: https://git.openjdk.org/jdk/pull/17306/files - new: https://git.openjdk.org/jdk/pull/17306/files/5f67f8c1..5a04ec59 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17306=06 - incr: https://webrevs.openjdk.org/?repo=jdk=17306=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17306.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17306/head:pull/17306 PR: https://git.openjdk.org/jdk/pull/17306
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v11]
> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes. > > This patch converts it to use Classfile API. > > It is continuation of https://github.com/openjdk/jdk/pull/10991 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java Co-authored-by: Mandy Chung - Changes: - all: https://git.openjdk.org/jdk/pull/17121/files - new: https://git.openjdk.org/jdk/pull/17121/files/e9bf7d57..48c2e6bc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17121=10 - incr: https://webrevs.openjdk.org/?repo=jdk=17121=09-10 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/17121.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17121/head:pull/17121 PR: https://git.openjdk.org/jdk/pull/17121
Re: RFR: 8323183: ClassFile API performance improvements [v6]
On Tue, 9 Jan 2024 13:30:58 GMT, Adam Sotona wrote: >> ClassFile API performance related improvements have been separated from >> #17121 into this PR. >> >> These improvements are important to minimize performance regression of >> 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the >> Classfile API to generate proxy classes #17121 >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with three additional > commits since the last revision: > > - updated copyright year > - reverted custom method slots counting in StackCounter > - improved and extended GenerateStackMaps benchmarks and renamed to > CodeAttributeTools StackMapGenerator benchmark is heavily benefiting from symbols caching for repeated execution. I've created new CodeAttributeTools::benchmarkStackMapsGenerator and CodeAttributeTools::benchmarkStackCounter with focus on single use of each parsed model to eliminate background caching effect. Then I've compared version with custom method parameter slots counting and version with symbols cached in NaT entries. There is insignificant difference between these two approaches. Explanation may by that repeated uses of the same NaT entry (of the called method) compensate the initial parsing cost. Based on these results I've decided to revert custom parameter slots counting method and apply the previously proposed solution. - PR Comment: https://git.openjdk.org/jdk/pull/17306#issuecomment-1883065817
Re: RFR: 8323183: ClassFile API performance improvements [v6]
> ClassFile API performance related improvements have been separated from > #17121 into this PR. > > These improvements are important to minimize performance regression of > 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the > Classfile API to generate proxy classes #17121 > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with three additional commits since the last revision: - updated copyright year - reverted custom method slots counting in StackCounter - improved and extended GenerateStackMaps benchmarks and renamed to CodeAttributeTools - Changes: - all: https://git.openjdk.org/jdk/pull/17306/files - new: https://git.openjdk.org/jdk/pull/17306/files/79394170..5f67f8c1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17306=05 - incr: https://webrevs.openjdk.org/?repo=jdk=17306=04-05 Stats: 313 lines in 3 files changed: 132 ins; 171 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/17306.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17306/head:pull/17306 PR: https://git.openjdk.org/jdk/pull/17306
Re: RFR: 8323183: ClassFile API performance improvements [v5]
> ClassFile API performance related improvements have been separated from > #17121 into this PR. > > These improvements are important to minimize performance regression of > 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the > Classfile API to generate proxy classes #17121 > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java Co-authored-by: liach <7806504+li...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/17306/files - new: https://git.openjdk.org/jdk/pull/17306/files/bde0..79394170 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17306=04 - incr: https://webrevs.openjdk.org/?repo=jdk=17306=03-04 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17306.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17306/head:pull/17306 PR: https://git.openjdk.org/jdk/pull/17306
Re: RFR: 8323183: ClassFile API performance improvements
On Mon, 8 Jan 2024 14:43:58 GMT, Chen Liang wrote: > You need to update the slot counting from `DirectCodeBuilder` and > `StackMapDecoder` to fully avoid creating any MethodTypeDesc. It would be good to avoid all bottlenecks, however not all of them have equal effect. This patch avoids MTD construction for every invocation instruction. We may cache the symbol in the relevant Utf8Entry, however it still means to create each MTD at least once. "Hairy" methods doing a lot of various invocations are the most affected. - PR Comment: https://git.openjdk.org/jdk/pull/17306#issuecomment-1881429589
Re: RFR: 8323183: ClassFile API performance improvements [v4]
On Mon, 8 Jan 2024 14:19:12 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> applied suggested changes
>
> src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java
> line 338:
>
>> 336: }
>> 337:
>> 338: private static int countMethodStack(Utf8Entry descriptor, boolean
>> subReturn) {
>
> Since we use this for both locals and stack, perhaps "countMethodSlots" would
> be better? And maybe the "subReturn" could be a flag to switch between
> locals/stack
I've changes the name to the suggested "countMethodSlots" and "subReturn" to
"asStackDelta" , thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17306#discussion_r1444934827
Re: RFR: 8323183: ClassFile API performance improvements [v4]
> ClassFile API performance related improvements have been separated from > #17121 into this PR. > > These improvements are important to minimize performance regression of > 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the > Classfile API to generate proxy classes #17121 > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: applied suggested changes - Changes: - all: https://git.openjdk.org/jdk/pull/17306/files - new: https://git.openjdk.org/jdk/pull/17306/files/718af508..bde0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17306=03 - incr: https://webrevs.openjdk.org/?repo=jdk=17306=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17306.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17306/head:pull/17306 PR: https://git.openjdk.org/jdk/pull/17306
Re: RFR: 8323183: ClassFile API performance improvements [v2]
> ClassFile API performance related improvements have been separated from > #17121 into this PR. > > These improvements are important to minimize performance regression of > 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the > Classfile API to generate proxy classes #17121 > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: improved StackMapDescoder::initFrameLocals performance - Changes: - all: https://git.openjdk.org/jdk/pull/17306/files - new: https://git.openjdk.org/jdk/pull/17306/files/bb2cf21c..c523c7be Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17306=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17306=00-01 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17306.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17306/head:pull/17306 PR: https://git.openjdk.org/jdk/pull/17306
Re: RFR: 8323183: ClassFile API performance improvements [v3]
On Mon, 8 Jan 2024 14:20:17 GMT, Maurizio Cimadamore wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> applied suggested changes > > src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java > line 417: > >> 415: bcs.bci, >> 416: methodName, >> 417: >> MethodTypeDesc.ofDescriptor(methodDesc.stringValue()).displayDescriptor())); > > This seems unrelated? methodDesc is newly an Utf8Entry, so it needs to be converted to throw with the same message - PR Review Comment: https://git.openjdk.org/jdk/pull/17306#discussion_r1444917187
Re: RFR: 8323183: ClassFile API performance improvements [v3]
> ClassFile API performance related improvements have been separated from > #17121 into this PR. > > These improvements are important to minimize performance regression of > 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the > Classfile API to generate proxy classes #17121 > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: applied suggested changes - Changes: - all: https://git.openjdk.org/jdk/pull/17306/files - new: https://git.openjdk.org/jdk/pull/17306/files/c523c7be..718af508 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17306=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17306=01-02 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/17306.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17306/head:pull/17306 PR: https://git.openjdk.org/jdk/pull/17306
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v7]
On Sun, 7 Jan 2024 18:24:02 GMT, Chen Liang wrote: > Also `StackMapDecoder::initFrameFromLocals(ClassEntry, ...)` can benefit from > 2 changes: > > 1. Iterate MTD by index instead of creating copies I'll add this fix to the performance improvements PR, thanks! > 2. Pass ClassEntry from reader/writer constant pool to > ObjectVerificationTypeInfoImpl instead of using the temporary pool Unfortunately initi frame is implicit and the entries may not exist in the constant pool at all. - PR Comment: https://git.openjdk.org/jdk/pull/17121#issuecomment-1881289533
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v3]
On Fri, 5 Jan 2024 23:55:53 GMT, Mandy Chung wrote: > > Profiling of the benchmarks revealed several slowdowns: > > ``` > > * many expensive conversions from `Class` to `ClassDesc` to > > `ClassEntry`, or even more expensive `MethodTypeDesc` > > > > * building proxy class from scratch from symbols also involves a lot of > > `String` concatenations, hashing, encoding and comparisons > > > > * computation of stack maps is also still expensive > > > > * `SplitConstantPool` was ineffective in some specific cases > > ``` > > Do you think these also cause the performance overhead in converting > java.lang.invoke to use ClassFile API (#17108)? Yes, I think it affects all performance critical applications. - PR Comment: https://git.openjdk.org/jdk/pull/17121#issuecomment-1881235421
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v7]
On Fri, 5 Jan 2024 21:03:28 GMT, Mandy Chung wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> StackCounter fix
>
> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 148:
>
>> 146: generateLookupAccessor(clb);
>> 147: var cp = clb.constantPool();
>> 148: q.entries = new PoolEntry[] {
>
> line 174-197 must match the order of CP entries being added.
>
> Is there other way to avoid `q.next()` coupling with the order of CP entries
> in `q.entries`? Maybe define constants for the indices to `q.entries` such
> that writing and reading the CP entries from the array using the constant
> index is easier to read and less error-prone?
I've rewritten the initialization to use indexes inside an array.
Thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1444825858
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v10]
> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes. > > This patch converts it to use Classfile API. > > It is continuation of https://github.com/openjdk/jdk/pull/10991 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: initialization of template entries by index - Changes: - all: https://git.openjdk.org/jdk/pull/17121/files - new: https://git.openjdk.org/jdk/pull/17121/files/80340914..e9bf7d57 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17121=09 - incr: https://webrevs.openjdk.org/?repo=jdk=17121=08-09 Stats: 100 lines in 1 file changed: 39 ins; 30 del; 31 mod Patch: https://git.openjdk.org/jdk/pull/17121.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17121/head:pull/17121 PR: https://git.openjdk.org/jdk/pull/17121
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v9]
> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes. > > This patch converts it to use Classfile API. > > It is continuation of https://github.com/openjdk/jdk/pull/10991 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with two additional commits since the last revision: - Apply suggestions from code review Co-authored-by: Mandy Chung - applied suggested changes - Changes: - all: https://git.openjdk.org/jdk/pull/17121/files - new: https://git.openjdk.org/jdk/pull/17121/files/2e50f842..80340914 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17121=08 - incr: https://webrevs.openjdk.org/?repo=jdk=17121=07-08 Stats: 41 lines in 1 file changed: 17 ins; 14 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/17121.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17121/head:pull/17121 PR: https://git.openjdk.org/jdk/pull/17121
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v7]
On Fri, 5 Jan 2024 21:23:02 GMT, Mandy Chung wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> StackCounter fix
>
> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 739:
>
>> 737: var desc = new StringJoiner("", "(", ")" +
>> returnType.descriptorString());
>> 738: for (var pt : parameterTypes) {
>> 739: desc.add(pt.descriptorString());
>
> Maybe worth refactor these as `ProxyMethod::toMethodTypeDescriptorString`
> with the comment to explain why `MethodType::descriptorString` is not used.
I've decided to revert this custom optimization.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1444732383
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v7]
On Fri, 5 Jan 2024 20:50:16 GMT, Mandy Chung wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> StackCounter fix
>
> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 135:
>
>> 133: var cc = ClassFile.of();
>> 134: var entries = new ArrayList(20);
>> 135: var q = new Object() {
>
> It'd be helpful to add a comment to explain what this optimization is doing.
Yes, I'll add an explanation comment, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1444717838
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v7]
On Fri, 5 Jan 2024 20:45:22 GMT, Mandy Chung wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> StackCounter fix > > src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 112: > >> 110: private static final ClassModel TEMPLATE; >> 111: >> 112: private static final Utf8Entry UE_Method; > > Nit: good to group this list of static variables by type for netter > readability. yes, will group it, thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1444697655
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v5]
On Sun, 7 Jan 2024 18:16:05 GMT, Chen Liang wrote: >> Original code has been significant in profiler. >> >> >> MethodTypeDesc desc = MethodTypeDesc.of(toClassDesc(returnType), >> >> Arrays.stream(parameterTypes).map(ProxyGenerator::toClassDesc).toArray(ClassDesc[]::new)); >> >> >> 1. each `toClassDesc` builds `descriptorString` and parses/validates it >> while constructing `ClassDesc` >> 2. `Arrays.stream(...).map(...).toArray(...)` allocates an array >> 3. `MethodTypeDesc.of(...)` clones the array and iterates params to check >> for void >> 4. `desc.descriptorString()` then finally use the `StringJoiner` >> >> Optimized code only joins `descriptorString`, no validations, no streaming, >> no arrays, no cloning. >> >> >> I suggest this patch as this code is considered as performance critical. >> However we can go through `ClassDesc` and `MethodTypeDesc` if not >> performance critical or if the conversions would be optimized. >> >> For example better (trusted) paths from `MethodType` to >> `MethodTypeDescriptor` and from `Class` to `ClassDesc`, avoiding at least >> validations. > > Turns out your approach to avoid MTD here is apparently useless; > `MethodTypeDesc` is still created for initializing the local tracker > `topLocal` in `DirectCodeBuilder`. In addition, `StackMapDecoder` also uses > `methodTypeSymbol` to compute the initial frame. > > IMO we should just stay with MTD; the descriptor breakdown happens too often > and, from previous benchmarks, descriptor breakdown is actually slow (which > gives CF API a small edge over ASM here). But we can still replace the > `parameterList()` iteration with index-based iteration to avoid array copies. Right, there are still so many conversions. I'll revert the custom code to simplify further optimizations. - PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1444690465
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v7]
On Fri, 5 Jan 2024 20:43:00 GMT, Mandy Chung wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> StackCounter fix > > src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 28: > >> 26: package java.lang.reflect; >> 27: >> 28: import java.lang.classfile.*; > > Nit: sort the imports in alphabetical orders. Will fix, thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1444693996
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v8]
> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes. > > This patch converts it to use Classfile API. > > It is continuation of https://github.com/openjdk/jdk/pull/10991 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with six additional commits since the last revision: - Revert "StackCounter performance boost" This reverts commit 0dc63d4edf40fd9458fbfa0c7661d57ed0022981. - Revert "SplitConstantPool performance fix" This reverts commit b7a60ae944983224e3b4c097576c496351394fe0. - Revert "applied the recommended changes" This reverts commit 7d0da2c0190c27f8e2cf89557e31f5d16ab4950e. - Revert "minor StackCounter fix" This reverts commit 41e879348c8f2ea70b25119e65527b81281c33ac. - Revert "Update src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java" This reverts commit c8f1d304358e19872450cd29449d82675f9bbe3e. - Revert "StackCounter fix" This reverts commit c6b761a157e66ccba30df68efa2849a92371acf2. - Changes: - all: https://git.openjdk.org/jdk/pull/17121/files - new: https://git.openjdk.org/jdk/pull/17121/files/c6b761a1..2e50f842 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17121=07 - incr: https://webrevs.openjdk.org/?repo=jdk=17121=06-07 Stats: 92 lines in 2 files changed: 14 ins; 57 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/17121.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17121/head:pull/17121 PR: https://git.openjdk.org/jdk/pull/17121
RFR: 8323183: ClassFile API performance improvements
ClassFile API performance related improvements have been separated from #17121 into this PR. These improvements are important to minimize performance regression of 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes #17121 Please review. Thanks, Adam - Commit messages: - Revert "8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes" - Revert "performance improvements" - Revert "performance improvements" - Revert "performance improvements" - StackCounter fix - Update src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java - minor StackCounter fix - applied the recommended changes - performance improvements - SplitConstantPool performance fix - ... and 4 more: https://git.openjdk.org/jdk/compare/2611a49e...bb2cf21c Changes: https://git.openjdk.org/jdk/pull/17306/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17306=00 Issue: https://bugs.openjdk.org/browse/JDK-8323183 Stats: 92 lines in 2 files changed: 57 ins; 14 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/17306.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17306/head:pull/17306 PR: https://git.openjdk.org/jdk/pull/17306
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v7]
On Wed, 3 Jan 2024 12:36:26 GMT, Adam Sotona wrote: >> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy >> classes. >> >> This patch converts it to use Classfile API. >> >> It is continuation of https://github.com/openjdk/jdk/pull/10991 >> >> Any comments and suggestions are welcome. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > StackCounter fix I've separated ClassFile API performance related improvements into #17306 - PR Comment: https://git.openjdk.org/jdk/pull/17121#issuecomment-1881026433
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v7]
On Sun, 7 Jan 2024 18:18:32 GMT, Chen Liang wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> StackCounter fix > > src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java > line 306: > >> 304: var cpe = cp.entryByIndex(bcs.getIndexU2()); >> 305: var nameAndType = opcode == INVOKEDYNAMIC ? >> ((DynamicConstantPoolEntry)cpe).nameAndType() : >> ((MemberRefEntry)cpe).nameAndType(); >> 306: >> addStackSlot(-countMethodStack(nameAndType.type(), true)); > > Can use something like: > > var mtd = Util.methodTypeSymbol(nameAndType); > addStackSlot(Util.slotSize(mtd.returnType()) - Util.parameterSlots(mtd)); > > if you stick with MTD. Calculation of MTD for each method invocation is extremely ineffective way to just count the size. - PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1444624257
Re: RFR: 8319463: ClassSignature should have superclass and superinterfaces as ClassTypeSig [v4]
On Fri, 5 Jan 2024 23:06:30 GMT, Chen Liang wrote: >> Discovered while writing a test for #16513 that >> `ClassSignature.superclassSignature()` does not return a `ClassTypeSig`, yet >> [JVM >> Spec](https://docs.oracle.com/javase/specs/jvms/se21/html/jvms-4.html#jvms-4.7.9.1-4100) >> requires it to be one. This patch adds such a requirement to the accessors, >> factories, and the parsing logic. > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains one commit: > > 8319463: ClassSignature should have superclass and superinterfaces as > ClassTypeSig The patch looks good to me. - Marked as reviewed by asotona (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16514#pullrequestreview-1808894111
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v7]
> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes. > > This patch converts it to use Classfile API. > > It is continuation of https://github.com/openjdk/jdk/pull/10991 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: StackCounter fix - Changes: - all: https://git.openjdk.org/jdk/pull/17121/files - new: https://git.openjdk.org/jdk/pull/17121/files/c8f1d304..c6b761a1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17121=06 - incr: https://webrevs.openjdk.org/?repo=jdk=17121=05-06 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17121.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17121/head:pull/17121 PR: https://git.openjdk.org/jdk/pull/17121
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v5]
On Sun, 24 Dec 2023 03:14:44 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> minor StackCounter fix
>
> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 737:
>
>> 735: private void generateMethod(ClassBuilder clb, ClassEntry
>> className) {
>> 736: var cp = clb.constantPool();
>> 737: var desc = new StringJoiner("", "(", ")" +
>> returnType.descriptorString());
>
> We should just use an MTD here; the MTD will be passed to StackCounter so we
> don't have to recompute a MTD.
>
> The MT to MTD conversion shouldn't be too costly; the overhead probably comes
> from Optional, which we have to wait until Valhalla, as proxy generation is
> unlikely to be hot and compiled by C2.
Original code has been significant in profiler.
MethodTypeDesc desc = MethodTypeDesc.of(toClassDesc(returnType),
Arrays.stream(parameterTypes).map(ProxyGenerator::toClassDesc).toArray(ClassDesc[]::new));
1. each `toClassDesc` builds `descriptorString` and parses/validates it while
constructing `ClassDesc`
2. `Arrays.stream(...).map(...).toArray(...)` allocates an array
3. `MethodTypeDesc.of(...)` clones the array and iterates params to check for
void
4. `desc.descriptorString()` then finally use the `StringJoiner`
Optimized code only joins `descriptorString`, no validations, no streaming, no
arrays, no cloning.
I suggest this patch as this code is considered as performance critical.
However we can go through `ClassDesc` and `MethodTypeDesc` if not performance
critical or if the conversions would be optimized.
For example better (trusted) paths from `MethodType` to `MethodTypeDescriptor`
and from `Class` to `ClassDesc`, avoiding at least validations.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1440395021
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v5]
On Sun, 24 Dec 2023 04:07:07 GMT, Chen Liang wrote: >> This code is part of the **ClassFile API**’s internals, and so it doesn’t >> have access to `ProxyGenerator`’s cached `MethodTypeDesc`s, only the >> underlying `Utf8Entry`, so it’d need to be parsed. > > I see that now Class return type and Class array parameter types are directly > converted to Strings instead of MTDs. I think that we should really run > ProxyGenerator with JFR to profile and see the results (for instance, old ASM > has a stackmap generation penalty due to parsing method descriptors on the > fly compared to CF API, despite CF's slower overall performance due to > allocations like Optional) These changes are based on profiling of the benchmarks. StackCounter was not used in any performance critical applications and profiling revealed some obvious bottlenecks. - PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1440364580
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v6]
> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes. > > This patch converts it to use Classfile API. > > It is continuation of https://github.com/openjdk/jdk/pull/10991 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/17121/files - new: https://git.openjdk.org/jdk/pull/17121/files/41e87934..c8f1d304 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17121=05 - incr: https://webrevs.openjdk.org/?repo=jdk=17121=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17121.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17121/head:pull/17121 PR: https://git.openjdk.org/jdk/pull/17121
Re: RFR: 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown [v2]
On Thu, 7 Dec 2023 07:53:58 GMT, Adam Sotona wrote:
>> ClassFile API throws `IndexOutOfBoundsException` when classfile structure is
>> corrupted so the parser attempts to read beyond the classfile bounds.
>> General contract is that only `IllegalArgumentException` or its subclasses
>> is expected when parser fails.
>> This patch wraps `IndexOutOfBoundsExceptions` thrown from all
>> `ClassReaderImpl.buffer` manipulations into an
>> `IllegalArgumentException("Reading beyond classfile bounds", iOOBECause)`.
>> Relevant tests are added.
>>
>> Please review.
>>
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains three commits:
>
> - Merge branch 'master' into JDK-8320360-bounds
>
># Conflicts:
># test/jdk/jdk/classfile/LimitsTest.java
> - added bug # to the test
> - 8320360: ClassFile.parse: Some defect class files cause unexpected
> exceptions to be thrown
Thanks for the review!
-
PR Comment: https://git.openjdk.org/jdk/pull/16762#issuecomment-1873994195
Integrated: 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown
On Tue, 21 Nov 2023 13:59:23 GMT, Adam Sotona wrote:
> ClassFile API throws `IndexOutOfBoundsException` when classfile structure is
> corrupted so the parser attempts to read beyond the classfile bounds.
> General contract is that only `IllegalArgumentException` or its subclasses is
> expected when parser fails.
> This patch wraps `IndexOutOfBoundsExceptions` thrown from all
> `ClassReaderImpl.buffer` manipulations into an
> `IllegalArgumentException("Reading beyond classfile bounds", iOOBECause)`.
> Relevant tests are added.
>
> Please review.
>
> Thanks,
> Adam
This pull request has now been integrated.
Changeset: a5cf4210
Author:Adam Sotona
URL:
https://git.openjdk.org/jdk/commit/a5cf4210cd9c293a9e9bce60dc6d0f08fd838c77
Stats: 71 lines in 2 files changed: 47 ins; 0 del; 24 mod
8320360: ClassFile.parse: Some defect class files cause unexpected exceptions
to be thrown
Reviewed-by: jpai
-
PR: https://git.openjdk.org/jdk/pull/16762
Re: RFR: 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown [v2]
On Tue, 2 Jan 2024 07:03:55 GMT, Jaikiran Pai wrote:
>> Adam Sotona has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains three commits:
>>
>> - Merge branch 'master' into JDK-8320360-bounds
>>
>># Conflicts:
>># test/jdk/jdk/classfile/LimitsTest.java
>> - added bug # to the test
>> - 8320360: ClassFile.parse: Some defect class files cause unexpected
>> exceptions to be thrown
>
> src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java
> line 200:
>
>> 198: try {
>> 199: return buffer[p] & 0xFF;
>> 200: } catch (IndexOutOfBoundsException e) {
>
> This and all other catch blocks introduced in this change can be changed to
> the specific exception type `ArrayIndexOutOfBoundsException`, because all
> these operations are dealing with only array access. If you prefer catching
> this more generic `IndexOutOfBoundsException`, that's fine with me.
Yes, I prefer the generic `IndexOutOfBoundsException`, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16762#discussion_r1439426562
Re: RFR: 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown [v3]
> ClassFile API throws `IndexOutOfBoundsException` when classfile structure is
> corrupted so the parser attempts to read beyond the classfile bounds.
> General contract is that only `IllegalArgumentException` or its subclasses is
> expected when parser fails.
> This patch wraps `IndexOutOfBoundsExceptions` thrown from all
> `ClassReaderImpl.buffer` manipulations into an
> `IllegalArgumentException("Reading beyond classfile bounds", iOOBECause)`.
> Relevant tests are added.
>
> Please review.
>
> Thanks,
> Adam
Adam Sotona has updated the pull request incrementally with one additional
commit since the last revision:
updated copyright years
-
Changes:
- all: https://git.openjdk.org/jdk/pull/16762/files
- new: https://git.openjdk.org/jdk/pull/16762/files/08bcc548..a16bf6da
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=16762=02
- incr: https://webrevs.openjdk.org/?repo=jdk=16762=01-02
Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/16762.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/16762/head:pull/16762
PR: https://git.openjdk.org/jdk/pull/16762
Re: RFR: 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown [v2]
On Tue, 2 Jan 2024 12:41:08 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java
>> line 283:
>>
>>> 281: public void copyBytesTo(BufWriter buf, int p, int len) {
>>> 282: try {
>>> 283: buf.writeBytes(buffer, p, len);
>>
>> `java.lang.classfile.BufWriter` doesn't specify any `@throws` for its
>> `writeXXX` methods. Should it be specified (of course in a separate PR)?
>
> That is a good point, thanks!
I've created [JDK-8322847](https://bugs.openjdk.org/browse/JDK-8322847)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16762#discussion_r1439418563
Re: RFR: 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown [v2]
On Tue, 2 Jan 2024 07:05:22 GMT, Jaikiran Pai wrote:
>> Adam Sotona has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains three commits:
>>
>> - Merge branch 'master' into JDK-8320360-bounds
>>
>># Conflicts:
>># test/jdk/jdk/classfile/LimitsTest.java
>> - added bug # to the test
>> - 8320360: ClassFile.parse: Some defect class files cause unexpected
>> exceptions to be thrown
>
> src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java
> line 283:
>
>> 281: public void copyBytesTo(BufWriter buf, int p, int len) {
>> 282: try {
>> 283: buf.writeBytes(buffer, p, len);
>
> `java.lang.classfile.BufWriter` doesn't specify any `@throws` for its
> `writeXXX` methods. Should it be specified (of course in a separate PR)?
That is a good point, thanks!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16762#discussion_r1439414618
Integrated: 8321540: ClassSignature.parseFrom() throws StringIndexOutOfBoundsException for invalid signatures
On Mon, 11 Dec 2023 15:17:49 GMT, Adam Sotona wrote: > ClassFile API models and parses signatures, however parsing of the signatures > mostly throws IIOBE when it fails. > This patch improves SignaturesImpl parsing methods implementation and errors > handling and adds relevant negative tests. > The parser is not an ultimate signatures validator yet, however this is a > step forward to it. > > Please review. > > Thanks, > Adam This pull request has now been integrated. Changeset: f9aec02f Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/f9aec02f3caabb6bc06672c214127f8912449615 Stats: 136 lines in 2 files changed: 103 ins; 5 del; 28 mod 8321540: ClassSignature.parseFrom() throws StringIndexOutOfBoundsException for invalid signatures Reviewed-by: jpai - PR: https://git.openjdk.org/jdk/pull/17058
Re: RFR: 8321540: ClassSignature.parseFrom() throws StringIndexOutOfBoundsException for invalid signatures [v2]
On Tue, 2 Jan 2024 07:17:46 GMT, Jaikiran Pai wrote: > I'm guessing you have intentionally not included the original > `IndexOutOfBoundsException` as a cause in the `IllegalArgumentException` that > gets thrown. Yes, original exception does not contain any meaningful information. > Also, it appears that the `SignaturesImpl` is more like an utility class and > isn't expected to be used in multi-thread access and thus the reliance on the > (non final) instance field `sig` appears OK. Right, `SignaturesImpl` instance is never exposed for multi-threaded access. Thanks for the review! - PR Comment: https://git.openjdk.org/jdk/pull/17058#issuecomment-1873974280
Re: RFR: 8321540: ClassSignature.parseFrom() throws StringIndexOutOfBoundsException for invalid signatures [v2]
> ClassFile API models and parses signatures, however parsing of the signatures > mostly throws IIOBE when it fails. > This patch improves SignaturesImpl parsing methods implementation and errors > handling and adds relevant negative tests. > The parser is not an ultimate signatures validator yet, however this is a > step forward to it. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: updated copyright year - Changes: - all: https://git.openjdk.org/jdk/pull/17058/files - new: https://git.openjdk.org/jdk/pull/17058/files/da060bbb..e856dffe Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17058=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17058=00-01 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17058.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17058/head:pull/17058 PR: https://git.openjdk.org/jdk/pull/17058
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v5]
> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes. > > This patch converts it to use Classfile API. > > It is continuation of https://github.com/openjdk/jdk/pull/10991 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: minor StackCounter fix - Changes: - all: https://git.openjdk.org/jdk/pull/17121/files - new: https://git.openjdk.org/jdk/pull/17121/files/7d0da2c0..41e87934 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17121=04 - incr: https://webrevs.openjdk.org/?repo=jdk=17121=03-04 Stats: 4 lines in 1 file changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17121.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17121/head:pull/17121 PR: https://git.openjdk.org/jdk/pull/17121
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v4]
> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes. > > This patch converts it to use Classfile API. > > It is continuation of https://github.com/openjdk/jdk/pull/10991 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: applied the recommended changes - Changes: - all: https://git.openjdk.org/jdk/pull/17121/files - new: https://git.openjdk.org/jdk/pull/17121/files/75e31e3a..7d0da2c0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17121=03 - incr: https://webrevs.openjdk.org/?repo=jdk=17121=02-03 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/17121.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17121/head:pull/17121 PR: https://git.openjdk.org/jdk/pull/17121
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v3]
On Thu, 21 Dec 2023 12:01:25 GMT, Sergey Tsypanov wrote: >> Adam Sotona has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - performance improvements >> - SplitConstantPool performance fix > > src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java > line 62: > >> 60: private final Utf8Entry methodDesc; >> 61: private final SplitConstantPool cp; >> 62: private final LinkedList targets; > > Wouldn't it be better to use `ArrayDeque` instead of `LinkedList`? Good point, I'll change it, thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1434015586
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v3]
On Thu, 21 Dec 2023 01:12:07 GMT, Adam Sotona wrote: >> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy >> classes. >> >> This patch converts it to use Classfile API. >> >> It is continuation of https://github.com/openjdk/jdk/pull/10991 >> >> Any comments and suggestions are welcome. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with two additional > commits since the last revision: > > - performance improvements > - SplitConstantPool performance fix Here are actual numbers (_22 is before this patch): Benchmark Mode Cnt Score Error Units ProxyPerf.genIntf_1avgt5 18354.110 ? 696.706 ns/op ProxyPerf.genIntf_1_22 avgt5 15744.305 ? 1032.092 ns/op ProxyPerf.genStringsIntf_3 avgt5 26388.493 ? 2614.877 ns/op ProxyPerf.genStringsIntf_3_22 avgt5 21431.752 ? 398.627 ns/op ProxyPerf.genZeroParamsavgt5 17627.978 ? 456.046 ns/op ProxyPerf.genZeroParams_22 avgt5 15411.519 ? 6426.321 ns/op ProxyPerf.getPrimsIntf_2 avgt5 23862.592 ? 2274.386 ns/op ProxyPerf.getPrimsIntf_2_22avgt5 19148.768 ? 648.304 ns/op Finished running test 'micro:.+ProxyPerf.+' - PR Comment: https://git.openjdk.org/jdk/pull/17121#issuecomment-1865370450
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v4]
On Wed, 20 Dec 2023 20:39:55 GMT, Mandy Chung wrote: > Can you include the performance comparison before and after this change and > what performance benchmarks you run? I've run two batches on Aurora with mixed results (linked to [JDK-8294960](https://bugs.openjdk.org/browse/JDK-8294960)): `org.openjdk.bech.java.lang.invoke.+` and startup benchmarks I can attach some of them here if needed. - PR Comment: https://git.openjdk.org/jdk/pull/17108#issuecomment-1865359565
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v3]
On Thu, 21 Dec 2023 01:12:07 GMT, Adam Sotona wrote: >> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy >> classes. >> >> This patch converts it to use Classfile API. >> >> It is continuation of https://github.com/openjdk/jdk/pull/10991 >> >> Any comments and suggestions are welcome. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with two additional > commits since the last revision: > > - performance improvements > - SplitConstantPool performance fix Profiling of the benchmarks revealed several slowdowns: - many expensive conversions from `Class` to `ClassDesc` to `ClassEntry`, or even more expensive `MethodTypeDesc` - building proxy class from scratch from symbols also involves a lot of `String` concatenations, hashing, encoding and comparisons - computation of stack maps is also still expensive - `SplitConstantPool` was ineffective in some specific cases Proposed performance improvements use pre-generated template class and each proxy is then transformed with share constant pool. Frequently used constant pool entries are materialized in the template class constant pool to avoid expensive conversions, comparisons and hashing. Stack maps are generated manually. Performance fixes related to ClassFile API `SplitConstantPool` and `StackCounter` may be separated into another PR. For now I keep them together to measure synergic effect. Full benchmark numbers will be ready soon. - PR Comment: https://git.openjdk.org/jdk/pull/17121#issuecomment-1865353203
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v3]
> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes. > > This patch converts it to use Classfile API. > > It is continuation of https://github.com/openjdk/jdk/pull/10991 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with two additional commits since the last revision: - performance improvements - SplitConstantPool performance fix - Changes: - all: https://git.openjdk.org/jdk/pull/17121/files - new: https://git.openjdk.org/jdk/pull/17121/files/a21fb264..75e31e3a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17121=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17121=01-02 Stats: 76 lines in 2 files changed: 21 ins; 10 del; 45 mod Patch: https://git.openjdk.org/jdk/pull/17121.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17121/head:pull/17121 PR: https://git.openjdk.org/jdk/pull/17121
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes
On Tue, 19 Dec 2023 20:45:58 GMT, Mandy Chung wrote: > Can you include a summary of the performance comparison before and after the > patch per the > [comment](https://github.com/openjdk/jdk/pull/10991#pullrequestreview-1333426016) > in #10991. I've found significant performance regressions when running these recommended benchmark. Performance improvements are now in progress. Thanks for the reminder. - PR Comment: https://git.openjdk.org/jdk/pull/17121#issuecomment-1865062212
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v2]
> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes. > > This patch converts it to use Classfile API. > > It is continuation of https://github.com/openjdk/jdk/pull/10991 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with three additional commits since the last revision: - performance improvements - StackCounter performance boost - performance improvements - Changes: - all: https://git.openjdk.org/jdk/pull/17121/files - new: https://git.openjdk.org/jdk/pull/17121/files/0267d657..a21fb264 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17121=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17121=00-01 Stats: 338 lines in 2 files changed: 166 ins; 33 del; 139 mod Patch: https://git.openjdk.org/jdk/pull/17121.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17121/head:pull/17121 PR: https://git.openjdk.org/jdk/pull/17121
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v2]
On Sat, 16 Dec 2023 15:54:57 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added missing comment
>
> src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
> line 548:
>
>> 546: static ClassDesc classDesc(Class cls) {
>> 547: return cls.isHidden() ?
>> ClassDesc.ofInternalName(cls.getName().replace('.', '/'))
>> 548: :
>> ClassDesc.ofDescriptor(cls.descriptorString());
>
> That said, all the usages already anticipate a valid `ClassDesc` that can be
> encoded in bytecode except `implClass`. You should make 2 versions of
> `classDesc()`, one that eagerly throws for all other usages, and another
> special version that handles hidden class conversion (with the same code as
> existing method) for
> https://github.com/openjdk/jdk/pull/17108/files#diff-76236a0dd20022f749d95ad5890e2bece0c4ac212d1b2ffab90ca86875f14282R173
Split, thank you.
> src/java.base/share/classes/java/lang/invoke/TypeConvertingMethodAdapter.java
> line 162:
>
>> 160: } else {
>> 161: Class src;
>> 162: if (arg == functional || functional.isPrimitive()) {
>
> `arg == functional` avoids a cast, though it's not in parity with the older
> code. Should probably restore the old cast that takes a source argument to
> avoid adding `==` checks to avoid casts.
Original cast is avoided when class names (derived from descriptors, derived
from classes) are equal.
Correct me, please, but I think that this simple `arg == functional` check does
the same job, without unnecessary String conversions and comparisons.
Or is there a case, where the cast is missing or added incorrectly?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1429931780
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1429919564
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v4]
> java.base java.lang.invoke package heavily uses ASM to generate lambdas and > method handles. > > This patch converts ASM calls to Classfile API. > > This PR is continuation of https://github.com/openjdk/jdk/pull/12945 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Apply suggestions from code review Co-authored-by: liach <7806504+li...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/17108/files - new: https://git.openjdk.org/jdk/pull/17108/files/8fee4564..098df109 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17108=03 - incr: https://webrevs.openjdk.org/?repo=jdk=17108=02-03 Stats: 145 lines in 7 files changed: 51 ins; 55 del; 39 mod Patch: https://git.openjdk.org/jdk/pull/17108.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17108/head:pull/17108 PR: https://git.openjdk.org/jdk/pull/17108
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v2]
On Sat, 16 Dec 2023 16:53:26 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added missing comment
>
> src/java.base/share/classes/java/lang/invoke/MethodType.java line 1295:
>
>> 1293: public Optional describeConstable() {
>> 1294: try {
>> 1295: var params = new ClassDesc[parameterCount()];
>
> We can probably handle like this:
>
> var retDesc = returnType().describeConstable();
> if (retDesc.isEmpty())
> return Optional.empty();
>
> if (parameterCount() == 0)
> return Optional.of(MethodTypeDesc.of(retDesc.get()));
>
> var params = new ClassDesc[parameterCount()];
> for (int i = 0; i < params.length; i++) {
> var paramDesc = parameterType(i).describeConstable();
> if (paramDesc.isEmpty())
> return Optional.empty();
> params[i] = paramDesc.get();
> }
> return Optional.of(MethodTypeDesc.of(returnType().get(), params));
>
>
> To avoid creating exceptions and allocating array when the params array is
> empty.
yes, it looks better, thanks.
> src/java.base/share/classes/jdk/internal/classfile/impl/SplitConstantPool.java
> line 379:
>
>> 377: @Override
>> 378: public ClassEntry classEntry(ClassDesc classDesc) {
>> 379: if (classDesc == ConstantDescs.CD_Object) {
>
> What causes the slow lookup of Object CE? If it's because that hashing needs
> to compute the substring first, I recommend changing the hashing algorithm if
> that's the case.
Right, I'll take this specific shortcut back and will think about more generic
performance improvement.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1429863879
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1429866344
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v2]
On Sat, 16 Dec 2023 16:49:26 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added missing comment
>
> src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 2290:
>
>> 2288: int accessFlags;
>> 2289: try {
>> 2290: ClassModel cm =
>> java.lang.classfile.ClassFile.of().parse(bytes);
>
> No need for fully-qualified name.
Unfortunately there is conflict with MethodHandles.Lookup.ClassFile
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1429855148
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v2]
On Sat, 16 Dec 2023 16:42:09 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added missing comment
>
> src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
> line 1519:
>
>> 1517: // double - d2i,i2b d2i,i2s d2i,i2cd2i
>> d2l d2f <->
>> 1518: if (from != to && from != TypeKind.BooleanType && to !=
>> TypeKind.BooleanType) try {
>> 1519: cob.convertInstruction(from, to);
>
> Need an intermediate int if you are converting from long/float/double to
> byte/short/char.
Will fix, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1429794214
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v3]
On Sat, 16 Dec 2023 16:37:16 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Apply suggestions from code review
>>
>> Co-authored-by: liach <7806504+li...@users.noreply.github.com>
>
> src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
> line 468:
>
>> 466: }
>> 467:
>> 468: private void emitStoreInsn(BasicType type, int index) {
>
> If we are going to use `localsMap` in every removed `emitStoreInsn`, perhaps
> we should restore it instead? Will also make it easier to move away from
> localsMap to CodeBuilder's tracking system.
Right, I'll restore emitLoadInsn and emitStoreInsn
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1429791664
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v2]
On Sat, 16 Dec 2023 16:32:23 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added missing comment
>
> src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
> line 1145:
>
>> 1143: private static Opcode popInsnOpcode(BasicType type) {
>> 1144: switch (type) {
>> 1145: case I_TYPE:
>
> Should restore the enhanced switch syntax
Will fix, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1429753608
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v2]
On Sat, 16 Dec 2023 16:12:05 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added missing comment
>
> src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
> line 1171:
>
>> 1169:
>> cob.getfield(ClassDesc.ofInternalName("java/lang/invoke/MethodHandleImpl$CasesHolder"),
>> "cases",
>> 1170: CD_MethodHandle.arrayType());
>> 1171: int casesLocal = extendLocalsMap(new Class[] {
>> MethodHandle[].class });
>
> We should look into replacing the local map with `CodeBuilder.allocateLocal`
> etc. in the future.
Yes, however actual API does not match directly.
It would require an extension of the API or more significant refactoring of
InvokerBytecodeGenerator.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1429746763
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v3]
> java.base java.lang.invoke package heavily uses ASM to generate lambdas and > method handles. > > This patch converts ASM calls to Classfile API. > > This PR is continuation of https://github.com/openjdk/jdk/pull/12945 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Apply suggestions from code review Co-authored-by: liach <7806504+li...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/17108/files - new: https://git.openjdk.org/jdk/pull/17108/files/0f356eb3..8fee4564 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17108=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17108=01-02 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/17108.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17108/head:pull/17108 PR: https://git.openjdk.org/jdk/pull/17108
RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes
java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes. This patch converts it to use Classfile API. It is continuation of https://github.com/openjdk/jdk/pull/10991 Any comments and suggestions are welcome. Please review. Thank you, Adam - Commit messages: - 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes Changes: https://git.openjdk.org/jdk/pull/17121/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17121=00 Issue: https://bugs.openjdk.org/browse/JDK-8294961 Stats: 444 lines in 2 files changed: 19 ins; 191 del; 234 mod Patch: https://git.openjdk.org/jdk/pull/17121.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17121/head:pull/17121 PR: https://git.openjdk.org/jdk/pull/17121
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v2]
On Fri, 15 Dec 2023 12:27:16 GMT, ExE Boss wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added missing comment
>
> src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
> line 548:
>
>> 546: static ClassDesc classDesc(Class cls) {
>> 547: return cls.isHidden() ?
>> ClassDesc.ofInternalName(cls.getName().replace('.', '/'))
>> 548: :
>> ClassDesc.ofDescriptor(cls.descriptorString());
>
> This still isn’t correct, as [`Class::getName()`] includes the trailing
> `/` for a hidden class.
> Suggestion:
>
> if (cls.isHidden()) {
> var name = cls.getName();
> return ClassDesc.of(name.substring(0, name.indexOf('/'));
> }
> return ClassDesc.ofDescriptor(cls.descriptorString());
>
>
> [`Class::getName()`]:
> https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/Class.html#getName()
This is fix of
`runtime/cds/appcds/dynamicArchive/LambdaProxyCallerIsHidden.java` test.
It is based on the original code way of getting the class name.
Cutting of the class name behind the first slash will remove important suffix.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1427953577
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v2]
> java.base java.lang.invoke package heavily uses ASM to generate lambdas and > method handles. > > This patch converts ASM calls to Classfile API. > > This PR is continuation of https://github.com/openjdk/jdk/pull/12945 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: added missing comment - Changes: - all: https://git.openjdk.org/jdk/pull/17108/files - new: https://git.openjdk.org/jdk/pull/17108/files/dc53d750..0f356eb3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17108=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17108=00-01 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17108.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17108/head:pull/17108 PR: https://git.openjdk.org/jdk/pull/17108
RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles
java.base java.lang.invoke package heavily uses ASM to generate lambdas and method handles. This patch converts ASM calls to Classfile API. This PR is continuation of https://github.com/openjdk/jdk/pull/12945 Any comments and suggestions are welcome. Please review. Thank you, Adam - Commit messages: - fixed InnerClassLambdaMetafactory for hidden caller classes - performance improvements - consolidation of descriptors handling - InvokerBytecodeGenerator::emit... improvements - 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handlers Changes: https://git.openjdk.org/jdk/pull/17108/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17108=00 Issue: https://bugs.openjdk.org/browse/JDK-8294960 Stats: 2142 lines in 13 files changed: 443 ins; 859 del; 840 mod Patch: https://git.openjdk.org/jdk/pull/17108.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17108/head:pull/17108 PR: https://git.openjdk.org/jdk/pull/17108
Re: RFR: 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown [v2]
On Thu, 7 Dec 2023 07:53:58 GMT, Adam Sotona wrote:
>> ClassFile API throws `IndexOutOfBoundsException` when classfile structure is
>> corrupted so the parser attempts to read beyond the classfile bounds.
>> General contract is that only `IllegalArgumentException` or its subclasses
>> is expected when parser fails.
>> This patch wraps `IndexOutOfBoundsExceptions` thrown from all
>> `ClassReaderImpl.buffer` manipulations into an
>> `IllegalArgumentException("Reading beyond classfile bounds", iOOBECause)`.
>> Relevant tests are added.
>>
>> Please review.
>>
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains three commits:
>
> - Merge branch 'master' into JDK-8320360-bounds
>
># Conflicts:
># test/jdk/jdk/classfile/LimitsTest.java
> - added bug # to the test
> - 8320360: ClassFile.parse: Some defect class files cause unexpected
> exceptions to be thrown
May I ask for review of this fix?
Thank you.
-
PR Comment: https://git.openjdk.org/jdk/pull/16762#issuecomment-1853511673
[jdk22] Integrated: 8321641: ClassFile ModuleAttribute.ModuleAttributeBuilder::moduleVersion spec contains a copy-paste error
On Mon, 11 Dec 2023 10:19:52 GMT, Adam Sotona wrote: > 8321641: ClassFile ModuleAttribute.ModuleAttributeBuilder::moduleVersion spec > contains a copy-paste error This pull request has now been integrated. Changeset: a55e18ba Author: Adam Sotona URL: https://git.openjdk.org/jdk22/commit/a55e18baf093aa53aae954e3c9650770d703266e Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8321641: ClassFile ModuleAttribute.ModuleAttributeBuilder::moduleVersion spec contains a copy-paste error Reviewed-by: alanb Backport-of: 3c6459e1de9e75898a1b32a95acf684050fbe1af - PR: https://git.openjdk.org/jdk22/pull/5
RFR: 8321540: ClassSignature.parseFrom() throws StringIndexOutOfBoundsException for invalid signatures
ClassFile API models and parses signatures, however parsing of the signatures mostly throws IIOBE when it fails. This patch improves SignaturesImpl parsing methods implementation and errors handling and adds relevant negative tests. The parser is not an ultimate signatures validator yet, however this is a step forward to it. Please review. Thanks, Adam - Commit messages: - 8321540: ClassSignature.parseFrom() throws StringIndexOutOfBoundsException for invalid signatures Changes: https://git.openjdk.org/jdk/pull/17058/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17058=00 Issue: https://bugs.openjdk.org/browse/JDK-8321540 Stats: 134 lines in 2 files changed: 103 ins; 5 del; 26 mod Patch: https://git.openjdk.org/jdk/pull/17058.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17058/head:pull/17058 PR: https://git.openjdk.org/jdk/pull/17058
[jdk22] RFR: 8321641: ClassFile ModuleAttribute.ModuleAttributeBuilder::moduleVersion spec contains a copy-paste error
8321641: ClassFile ModuleAttribute.ModuleAttributeBuilder::moduleVersion spec contains a copy-paste error - Commit messages: - Backport 3c6459e1de9e75898a1b32a95acf684050fbe1af Changes: https://git.openjdk.org/jdk22/pull/5/files Webrev: https://webrevs.openjdk.org/?repo=jdk22=5=00 Issue: https://bugs.openjdk.org/browse/JDK-8321641 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk22/pull/5.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/5/head:pull/5 PR: https://git.openjdk.org/jdk22/pull/5
Integrated: 8321641: ClassFile ModuleAttribute.ModuleAttributeBuilder::moduleVersion spec contains a copy-paste error
On Mon, 11 Dec 2023 10:00:45 GMT, Adam Sotona wrote: > This is a fix of ClassFile > ModuleAttribute.ModuleAttributeBuilder::moduleVersion javadoc copy-paste > error. > > Please review. > > Thanks, > Adam This pull request has now been integrated. Changeset: 3c6459e1 Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/3c6459e1de9e75898a1b32a95acf684050fbe1af Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8321641: ClassFile ModuleAttribute.ModuleAttributeBuilder::moduleVersion spec contains a copy-paste error Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/17051
RFR: 8321641: ClassFile ModuleAttribute.ModuleAttributeBuilder::moduleVersion spec contains a copy-paste error
This is a fix of ClassFile ModuleAttribute.ModuleAttributeBuilder::moduleVersion javadoc copy-paste error. Please review. Thanks, Adam - Commit messages: - 8321641: ClassFile ModuleAttribute.ModuleAttributeBuilder::moduleVersion spec contains a copy-paste error Changes: https://git.openjdk.org/jdk/pull/17051/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17051=00 Issue: https://bugs.openjdk.org/browse/JDK-8321641 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17051.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17051/head:pull/17051 PR: https://git.openjdk.org/jdk/pull/17051
Re: RFR: 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown [v2]
> ClassFile API throws `IndexOutOfBoundsException` when classfile structure is
> corrupted so the parser attempts to read beyond the classfile bounds.
> General contract is that only `IllegalArgumentException` or its subclasses is
> expected when parser fails.
> This patch wraps `IndexOutOfBoundsExceptions` thrown from all
> `ClassReaderImpl.buffer` manipulations into an
> `IllegalArgumentException("Reading beyond classfile bounds", iOOBECause)`.
> Relevant tests are added.
>
> Please review.
>
> Thanks,
> Adam
Adam Sotona has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains three commits:
- Merge branch 'master' into JDK-8320360-bounds
# Conflicts:
#test/jdk/jdk/classfile/LimitsTest.java
- added bug # to the test
- 8320360: ClassFile.parse: Some defect class files cause unexpected
exceptions to be thrown
-
Changes: https://git.openjdk.org/jdk/pull/16762/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=16762=01
Stats: 69 lines in 2 files changed: 47 ins; 0 del; 22 mod
Patch: https://git.openjdk.org/jdk/pull/16762.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/16762/head:pull/16762
PR: https://git.openjdk.org/jdk/pull/16762
Re: RFR: 8321248: ClassFile API ClassModel::verify is inconsistent with the rest of the API [v2]
On Wed, 6 Dec 2023 10:40:51 GMT, Adam Sotona wrote: >> ClassFile API `ClassModel::verify` accepts `ClassHierarchyResolver` as an >> optional argument and does not respect >> `ClassFile.ClassHierarchyResolverOption` of the actual context. >> Parsing, building and transforming take options from the actual `ClassFile` >> context and verification should follow the same API pattern. >> >> This patch removes `ClassModel::verify` methods and introduces new top level >> methods: >> >> List ClassFile::verify(ClassModel model); >> List ClassFile::verify(byte[] bytes); >> List ClassFile::verify(Path path); >> >> >> Impact of the API change is minimal as the API has not been released yet. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > reverted modified test Thank you for the reviews. Yes, next step is to improve verifier range and also cover it with negative tests (see https://github.com/openjdk/jdk/pull/16809/files#diff-a23859572e86e946a9ce66361e64e4b7e4473f134bb49a248fccd70d2ac96ea2) - PR Comment: https://git.openjdk.org/jdk/pull/16947#issuecomment-1843120837
Integrated: 8321248: ClassFile API ClassModel::verify is inconsistent with the rest of the API
On Mon, 4 Dec 2023 11:12:37 GMT, Adam Sotona wrote: > ClassFile API `ClassModel::verify` accepts `ClassHierarchyResolver` as an > optional argument and does not respect > `ClassFile.ClassHierarchyResolverOption` of the actual context. > Parsing, building and transforming take options from the actual `ClassFile` > context and verification should follow the same API pattern. > > This patch removes `ClassModel::verify` methods and introduces new top level > methods: > > List ClassFile::verify(ClassModel model); > List ClassFile::verify(byte[] bytes); > List ClassFile::verify(Path path); > > > Impact of the API change is minimal as the API has not been released yet. > > Please review. > > Thanks, > Adam This pull request has now been integrated. Changeset: 0217b5ac Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/0217b5ac8b25db96fce026ac027b18024e25a329 Stats: 94 lines in 11 files changed: 44 ins; 32 del; 18 mod 8321248: ClassFile API ClassModel::verify is inconsistent with the rest of the API Reviewed-by: jlahoda, mcimadamore - PR: https://git.openjdk.org/jdk/pull/16947
Re: RFR: 8321248: ClassFile API ClassModel::verify is inconsistent with the rest of the API [v2]
> ClassFile API `ClassModel::verify` accepts `ClassHierarchyResolver` as an > optional argument and does not respect > `ClassFile.ClassHierarchyResolverOption` of the actual context. > Parsing, building and transforming take options from the actual `ClassFile` > context and verification should follow the same API pattern. > > This patch removes `ClassModel::verify` methods and introduces new top level > methods: > > List ClassFile::verify(ClassModel model); > List ClassFile::verify(byte[] bytes); > List ClassFile::verify(Path path); > > > Impact of the API change is minimal as the API has not been released yet. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: reverted modified test - Changes: - all: https://git.openjdk.org/jdk/pull/16947/files - new: https://git.openjdk.org/jdk/pull/16947/files/4485a369..bacdf481 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16947=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16947=00-01 Stats: 30 lines in 1 file changed: 30 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16947.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16947/head:pull/16947 PR: https://git.openjdk.org/jdk/pull/16947
Re: RFR: 8321248: ClassFile API ClassModel::verify is inconsistent with the rest of the API
On Wed, 6 Dec 2023 09:42:49 GMT, Adam Sotona wrote:
>> test/jdk/jdk/classfile/VerifierSelfTest.java line 62:
>>
>>> 60:
>>> 61: @Test
>>> 62: void testFailedDump() throws IOException {
>>
>> Why is this removed?
>
> Dump (print) of the classfile to an optional log (ConsumerString
> argument) has been removed from the API.
> It was a relic from early phase of the ClassFile API development and it has
> no use except for this test.
> This functionality can be replaced by explicit use of ClassPrinter.
I'll rename the test to "testFailed" and keep it without the "dump" part.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16947#discussion_r1417013768
Re: RFR: 8321248: ClassFile API ClassModel::verify is inconsistent with the rest of the API
On Wed, 6 Dec 2023 09:26:29 GMT, Maurizio Cimadamore
wrote:
>> ClassFile API `ClassModel::verify` accepts `ClassHierarchyResolver` as an
>> optional argument and does not respect
>> `ClassFile.ClassHierarchyResolverOption` of the actual context.
>> Parsing, building and transforming take options from the actual `ClassFile`
>> context and verification should follow the same API pattern.
>>
>> This patch removes `ClassModel::verify` methods and introduces new top level
>> methods:
>>
>> List ClassFile::verify(ClassModel model);
>> List ClassFile::verify(byte[] bytes);
>> List ClassFile::verify(Path path);
>>
>>
>> Impact of the API change is minimal as the API has not been released yet.
>>
>> Please review.
>>
>> Thanks,
>> Adam
>
> test/jdk/jdk/classfile/VerifierSelfTest.java line 62:
>
>> 60:
>> 61: @Test
>> 62: void testFailedDump() throws IOException {
>
> Why is this removed?
Dump (print) of the classfile to an optional log (ConsumerString
argument) has been removed from the API.
It was a relic from early phase of the ClassFile API development and it has no
use except for this test.
This functionality can be replaced by explicit use of ClassPrinter.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16947#discussion_r1417008753
RFR: 8321248: ClassFile API ClassModel::verify is inconsistent with the rest of the API
ClassFile API `ClassModel::verify` accepts `ClassHierarchyResolver` as an optional argument and does not respect `ClassFile.ClassHierarchyResolverOption` of the actual context. Parsing, building and transforming take options from the actual `ClassFile` context and verification should follow the same API pattern. This patch removes `ClassModel::verify` methods and introduces new top level methods: List ClassFile::verify(ClassModel model); List ClassFile::verify(byte[] bytes); List ClassFile::verify(Path path); ClassFile API has been recently opened as a preview feature, so this bug technically requires a CSR (temporary unable to create). Impact of the API change is none as the API has not been released yet. Please review. Thanks, Adam - Commit messages: - tests fix - dependent code fix - implementation - 8321248: ClassFile API ClassModel::verify is inconsistent with the rest of the API Changes: https://git.openjdk.org/jdk/pull/16947/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16947=00 Issue: https://bugs.openjdk.org/browse/JDK-8321248 Stats: 122 lines in 11 files changed: 44 ins; 62 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/16947.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16947/head:pull/16947 PR: https://git.openjdk.org/jdk/pull/16947
Integrated: 8308753: Class-File API transition to Preview
On Wed, 13 Sep 2023 09:48:00 GMT, Adam Sotona wrote: > Classfile API is an internal library under package `jdk.internal.classfile` > in JDK 21. > This pull request turns the Classfile API into a preview feature and moves it > into `java.lang.classfile`. > It repackages all uses across JDK and tests and adds lots of missing Javadoc. > > This PR goes in sync with > [JDK-8308754](https://bugs.openjdk.org/browse/JDK-8308754): Class-File API > (Preview) (CSR) > and [JDK-8280389](https://bugs.openjdk.org/browse/JDK-8280389): Class-File > API (Preview) (JEP). > > Online javadoc is available at: > https://cr.openjdk.org/~asotona/JDK-8308753-preview/api/java.base/java/lang/classfile/package-summary.html > > In addition to the primary transition to preview, this pull request also > includes: > - All Classfile* classes ranamed to ClassFile* (based on JEP discussion). > - A new preview feature, `CLASSFILE_API`, has been added. > - Buildsystem tool required a little patch to support annotated modules. > - All JDK modules using the Classfile API are newly participating in the > preview. > - All tests that use the Classfile API now have preview enabled. > - A few Javac tests not allowing preview have been partially reverted; their > conversion can be re-applied when the Classfile API leaves preview mode. > > Despite the number of affected files, this pull request is relatively > straight-forward. The preview version of the Classfile API is based on the > internal version of the library from the JDK master branch, and there are no > API features added. > > Please review this pull request to help the Classfile API turn into a preview. > > Any comments are welcome. > > Thanks, > Adam This pull request has now been integrated. Changeset: 2b00ac0d Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/2b00ac0d02a110326846c75ea7ea535dccbb1924 Stats: 32362 lines in 787 files changed: 14660 ins; 14113 del; 3589 mod 8308753: Class-File API transition to Preview Reviewed-by: ihse, mchung, vromero - PR: https://git.openjdk.org/jdk/pull/15706
Re: RFR: 8308753: Class-File API transition to Preview [v34]
> Classfile API is an internal library under package `jdk.internal.classfile` > in JDK 21. > This pull request turns the Classfile API into a preview feature and moves it > into `java.lang.classfile`. > It repackages all uses across JDK and tests and adds lots of missing Javadoc. > > This PR goes in sync with > [JDK-8308754](https://bugs.openjdk.org/browse/JDK-8308754): Class-File API > (Preview) (CSR) > and [JDK-8280389](https://bugs.openjdk.org/browse/JDK-8280389): Class-File > API (Preview) (JEP). > > Online javadoc is available at: > https://cr.openjdk.org/~asotona/JDK-8308753-preview/api/java.base/java/lang/classfile/package-summary.html > > In addition to the primary transition to preview, this pull request also > includes: > - All Classfile* classes ranamed to ClassFile* (based on JEP discussion). > - A new preview feature, `CLASSFILE_API`, has been added. > - Buildsystem tool required a little patch to support annotated modules. > - All JDK modules using the Classfile API are newly participating in the > preview. > - All tests that use the Classfile API now have preview enabled. > - A few Javac tests not allowing preview have been partially reverted; their > conversion can be re-applied when the Classfile API leaves preview mode. > > Despite the number of affected files, this pull request is relatively > straight-forward. The preview version of the Classfile API is based on the > internal version of the library from the JDK master branch, and there are no > API features added. > > Please review this pull request to help the Classfile API turn into a preview. > > Any comments are welcome. > > Thanks, > Adam Adam Sotona has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 372 commits: - Merge branch 'master' into JDK-8308753-preview - Merge branch 'master' into JDK-8308753-preview # Conflicts: #src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java - fixed SwitchBootstrapTest - fixed SwitchBootstrap and StackMapsTest - fixed SwtichBootstrapTest and ModuleVersionTest - Merge branch 'master' into JDK-8308753-preview - Merge branch 'master' into JDK-8308753-preview # Conflicts: #test/jdk/jdk/classfile/StackMapsTest.java - Merge branch 'master' into JDK-8308753-preview # Conflicts: #test/langtools/tools/javac/7199823/InnerClassCannotBeVerified.java - added new package-info snippets and fixed ClassFile::parse throws javadoc description - fixed lambda tests - ... and 362 more: https://git.openjdk.org/jdk/compare/b9df827a...a908e805 - Changes: https://git.openjdk.org/jdk/pull/15706/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15706=33 Stats: 32362 lines in 787 files changed: 14660 ins; 14113 del; 3589 mod Patch: https://git.openjdk.org/jdk/pull/15706.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15706/head:pull/15706 PR: https://git.openjdk.org/jdk/pull/15706
Re: RFR: 8308753: Class-File API transition to Preview [v33]
> Classfile API is an internal library under package `jdk.internal.classfile` > in JDK 21. > This pull request turns the Classfile API into a preview feature and moves it > into `java.lang.classfile`. > It repackages all uses across JDK and tests and adds lots of missing Javadoc. > > This PR goes in sync with > [JDK-8308754](https://bugs.openjdk.org/browse/JDK-8308754): Class-File API > (Preview) (CSR) > and [JDK-8280389](https://bugs.openjdk.org/browse/JDK-8280389): Class-File > API (Preview) (JEP). > > Online javadoc is available at: > https://cr.openjdk.org/~asotona/JDK-8308753-preview/api/java.base/java/lang/classfile/package-summary.html > > In addition to the primary transition to preview, this pull request also > includes: > - All Classfile* classes ranamed to ClassFile* (based on JEP discussion). > - A new preview feature, `CLASSFILE_API`, has been added. > - Buildsystem tool required a little patch to support annotated modules. > - All JDK modules using the Classfile API are newly participating in the > preview. > - All tests that use the Classfile API now have preview enabled. > - A few Javac tests not allowing preview have been partially reverted; their > conversion can be re-applied when the Classfile API leaves preview mode. > > Despite the number of affected files, this pull request is relatively > straight-forward. The preview version of the Classfile API is based on the > internal version of the library from the JDK master branch, and there are no > API features added. > > Please review this pull request to help the Classfile API turn into a preview. > > Any comments are welcome. > > Thanks, > Adam Adam Sotona has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 371 commits: - Merge branch 'master' into JDK-8308753-preview # Conflicts: #src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java - fixed SwitchBootstrapTest - fixed SwitchBootstrap and StackMapsTest - fixed SwtichBootstrapTest and ModuleVersionTest - Merge branch 'master' into JDK-8308753-preview - Merge branch 'master' into JDK-8308753-preview # Conflicts: #test/jdk/jdk/classfile/StackMapsTest.java - Merge branch 'master' into JDK-8308753-preview # Conflicts: #test/langtools/tools/javac/7199823/InnerClassCannotBeVerified.java - added new package-info snippets and fixed ClassFile::parse throws javadoc description - fixed lambda tests - Merge branch 'master' into JDK-8308753-preview - ... and 361 more: https://git.openjdk.org/jdk/compare/3b30095a...b7b2e4e4 - Changes: https://git.openjdk.org/jdk/pull/15706/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15706=32 Stats: 32362 lines in 787 files changed: 14660 ins; 14113 del; 3589 mod Patch: https://git.openjdk.org/jdk/pull/15706.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15706/head:pull/15706 PR: https://git.openjdk.org/jdk/pull/15706
Re: RFR: 8308753: Class-File API transition to Preview [v32]
On Wed, 29 Nov 2023 08:46:46 GMT, Per Minborg wrote: >> This is explicit list of supported class file versions, so I don't see any >> other way. > > Would it not be possible to expose an immutable `Map` that > maps from Java version to major class version? This is actually out of scope of this PR. Feel free to request a new ClassFile API feature. - PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1412090733
Re: RFR: 8308753: Class-File API transition to Preview [v32]
On Tue, 28 Nov 2023 10:21:25 GMT, Per Minborg wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fixed SwitchBootstrapTest > > src/java.base/share/classes/java/lang/classfile/ClassFile.java line 1404: > >> (failed to retrieve contents of file, check the PR for context) > Is there a more scalable way to express the major class version? The current > approach means we have to add two fields per year. Maybe a lookup function > would be better? This is explicit list of supported class file versions, so I don't see any other way. - PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1407726710
Re: RFR: 8308753: Class-File API transition to Preview [v32]
> Classfile API is an internal library under package `jdk.internal.classfile` > in JDK 21. > This pull request turns the Classfile API into a preview feature and moves it > into `java.lang.classfile`. > It repackages all uses across JDK and tests and adds lots of missing Javadoc. > > This PR goes in sync with > [JDK-8308754](https://bugs.openjdk.org/browse/JDK-8308754): Class-File API > (Preview) (CSR) > and [JDK-8280389](https://bugs.openjdk.org/browse/JDK-8280389): Class-File > API (Preview) (JEP). > > Online javadoc is available at: > https://cr.openjdk.org/~asotona/JDK-8308753-preview/api/java.base/java/lang/classfile/package-summary.html > > In addition to the primary transition to preview, this pull request also > includes: > - All Classfile* classes ranamed to ClassFile* (based on JEP discussion). > - A new preview feature, `CLASSFILE_API`, has been added. > - Buildsystem tool required a little patch to support annotated modules. > - All JDK modules using the Classfile API are newly participating in the > preview. > - All tests that use the Classfile API now have preview enabled. > - A few Javac tests not allowing preview have been partially reverted; their > conversion can be re-applied when the Classfile API leaves preview mode. > > Despite the number of affected files, this pull request is relatively > straight-forward. The preview version of the Classfile API is based on the > internal version of the library from the JDK master branch, and there are no > API features added. > > Please review this pull request to help the Classfile API turn into a preview. > > Any comments are welcome. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: fixed SwitchBootstrapTest - Changes: - all: https://git.openjdk.org/jdk/pull/15706/files - new: https://git.openjdk.org/jdk/pull/15706/files/deb064fc..dd9a4aa7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15706=31 - incr: https://webrevs.openjdk.org/?repo=jdk=15706=30-31 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15706.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15706/head:pull/15706 PR: https://git.openjdk.org/jdk/pull/15706
Re: RFR: 8308753: Class-File API transition to Preview [v31]
> Classfile API is an internal library under package `jdk.internal.classfile` > in JDK 21. > This pull request turns the Classfile API into a preview feature and moves it > into `java.lang.classfile`. > It repackages all uses across JDK and tests and adds lots of missing Javadoc. > > This PR goes in sync with > [JDK-8308754](https://bugs.openjdk.org/browse/JDK-8308754): Class-File API > (Preview) (CSR) > and [JDK-8280389](https://bugs.openjdk.org/browse/JDK-8280389): Class-File > API (Preview) (JEP). > > Online javadoc is available at: > https://cr.openjdk.org/~asotona/JDK-8308753-preview/api/java.base/java/lang/classfile/package-summary.html > > In addition to the primary transition to preview, this pull request also > includes: > - All Classfile* classes ranamed to ClassFile* (based on JEP discussion). > - A new preview feature, `CLASSFILE_API`, has been added. > - Buildsystem tool required a little patch to support annotated modules. > - All JDK modules using the Classfile API are newly participating in the > preview. > - All tests that use the Classfile API now have preview enabled. > - A few Javac tests not allowing preview have been partially reverted; their > conversion can be re-applied when the Classfile API leaves preview mode. > > Despite the number of affected files, this pull request is relatively > straight-forward. The preview version of the Classfile API is based on the > internal version of the library from the JDK master branch, and there are no > API features added. > > Please review this pull request to help the Classfile API turn into a preview. > > Any comments are welcome. > > Thanks, > Adam Adam Sotona has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 369 commits: - fixed SwitchBootstrap and StackMapsTest - fixed SwtichBootstrapTest and ModuleVersionTest - Merge branch 'master' into JDK-8308753-preview - Merge branch 'master' into JDK-8308753-preview # Conflicts: #test/jdk/jdk/classfile/StackMapsTest.java - Merge branch 'master' into JDK-8308753-preview # Conflicts: #test/langtools/tools/javac/7199823/InnerClassCannotBeVerified.java - added new package-info snippets and fixed ClassFile::parse throws javadoc description - fixed lambda tests - Merge branch 'master' into JDK-8308753-preview - fixed SignaturesTest - fixed condy tests - ... and 359 more: https://git.openjdk.org/jdk/compare/28d3762b...deb064fc - Changes: https://git.openjdk.org/jdk/pull/15706/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15706=30 Stats: 32361 lines in 787 files changed: 14660 ins; 14113 del; 3588 mod Patch: https://git.openjdk.org/jdk/pull/15706.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15706/head:pull/15706 PR: https://git.openjdk.org/jdk/pull/15706
Integrated: 8320618: NPE: Cannot invoke "java.lang.constant.ClassDesc.isArray()" because "this.sym" is null
On Thu, 23 Nov 2023 11:52:05 GMT, Adam Sotona wrote: > ClassFile API StackMapGenerator caused a NPE for a test case with invalid > sequence of instructions. > This fix simply avoids the NPE and adds StackMapsTest::testInvalidAALOADStack > > Please review. > > Thanks, > Adam This pull request has now been integrated. Changeset: 28d3762b Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/28d3762bd30a31623f2ed97a1870313d3a2b9acb Stats: 15 lines in 2 files changed: 12 ins; 1 del; 2 mod 8320618: NPE: Cannot invoke "java.lang.constant.ClassDesc.isArray()" because "this.sym" is null Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/16793
RFR: 8320618: NPE: Cannot invoke "java.lang.constant.ClassDesc.isArray()" because "this.sym" is null
ClassFile API StackMapGenerator caused a NPE for a test case with invalid sequence of instructions. This fix simply avoids the NPE and adds StackMapsTest::testInvalidAALOADStack Please review. Thanks, Adam - Commit messages: - 8320618: NPE: Cannot invoke "java.lang.constant.ClassDesc.isArray()" because "this.sym" is null Changes: https://git.openjdk.org/jdk/pull/16793/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16793=00 Issue: https://bugs.openjdk.org/browse/JDK-8320618 Stats: 15 lines in 2 files changed: 12 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16793.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16793/head:pull/16793 PR: https://git.openjdk.org/jdk/pull/16793
RFR: 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown
ClassFile API throws `IndexOutOfBoundsException` when classfile structure is
corrupted so the parser attempts to read beyond the classfile bounds.
General contract is that only `IllegalArgumentException` or its subclasses is
expected when parser fails.
This patch wraps `IndexOutOfBoundsExceptions` thrown from all
`ClassReaderImpl.buffer` manipulations into an
`IllegalArgumentException("Reading beyond classfile bounds", iOOBECause)`.
Relevant tests are added.
Please review.
Thanks,
Adam
-
Commit messages:
- added bug # to the test
- 8320360: ClassFile.parse: Some defect class files cause unexpected
exceptions to be thrown
Changes: https://git.openjdk.org/jdk/pull/16762/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=16762=00
Issue: https://bugs.openjdk.org/browse/JDK-8320360
Stats: 69 lines in 2 files changed: 47 ins; 0 del; 22 mod
Patch: https://git.openjdk.org/jdk/pull/16762.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/16762/head:pull/16762
PR: https://git.openjdk.org/jdk/pull/16762
Re: RFR: 8308753: Class-File API transition to Preview [v30]
> Classfile API is an internal library under package `jdk.internal.classfile` > in JDK 21. > This pull request turns the Classfile API into a preview feature and moves it > into `java.lang.classfile`. > It repackages all uses across JDK and tests and adds lots of missing Javadoc. > > This PR goes in sync with > [JDK-8308754](https://bugs.openjdk.org/browse/JDK-8308754): Class-File API > (Preview) (CSR) > and [JDK-8280389](https://bugs.openjdk.org/browse/JDK-8280389): Class-File > API (Preview) (JEP). > > Online javadoc is available at: > https://cr.openjdk.org/~asotona/JDK-8308753-preview/api/java.base/java/lang/classfile/package-summary.html > > In addition to the primary transition to preview, this pull request also > includes: > - All Classfile* classes ranamed to ClassFile* (based on JEP discussion). > - A new preview feature, `CLASSFILE_API`, has been added. > - Buildsystem tool required a little patch to support annotated modules. > - All JDK modules using the Classfile API are newly participating in the > preview. > - All tests that use the Classfile API now have preview enabled. > - A few Javac tests not allowing preview have been partially reverted; their > conversion can be re-applied when the Classfile API leaves preview mode. > > Despite the number of affected files, this pull request is relatively > straight-forward. The preview version of the Classfile API is based on the > internal version of the library from the JDK master branch, and there are no > API features added. > > Please review this pull request to help the Classfile API turn into a preview. > > Any comments are welcome. > > Thanks, > Adam Adam Sotona has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 366 commits: - Merge branch 'master' into JDK-8308753-preview # Conflicts: #test/jdk/jdk/classfile/StackMapsTest.java - Merge branch 'master' into JDK-8308753-preview # Conflicts: #test/langtools/tools/javac/7199823/InnerClassCannotBeVerified.java - added new package-info snippets and fixed ClassFile::parse throws javadoc description - fixed lambda tests - Merge branch 'master' into JDK-8308753-preview - fixed SignaturesTest - fixed condy tests - Merge branch 'master' into JDK-8308753-preview - Merge branch 'master' into JDK-8308753-preview # Conflicts: #test/jdk/tools/lib/tests/JImageValidator.java - fixed jdk.jfr - ... and 356 more: https://git.openjdk.org/jdk/compare/e055fae1...48aa8ba8 - Changes: https://git.openjdk.org/jdk/pull/15706/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15706=29 Stats: 32342 lines in 784 files changed: 14659 ins; 14109 del; 3574 mod Patch: https://git.openjdk.org/jdk/pull/15706.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15706/head:pull/15706 PR: https://git.openjdk.org/jdk/pull/15706
Integrated: 8320222: Wrong bytecode accepted, and StackMap table generated
On Thu, 16 Nov 2023 10:00:44 GMT, Adam Sotona wrote: > Stack map generator in ClassFile API performs only minimal checks in favour > of performance. > However it led to situations where it generates invalid stack maps for > corrupted code. > This patch adds basic checks of stack when two frames are merged and throws > an exception in case of stack size or content mismatch. Generated or > transformed code with inconsistent stack will fail on stack maps generation. > Relevant tests are added. > > Please review. > > Thanks, > Adam This pull request has now been integrated. Changeset: c4aee66d Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/c4aee66d742008848e5b5bc8ce3b2e3032a39bc3 Stats: 74 lines in 2 files changed: 46 ins; 0 del; 28 mod 8320222: Wrong bytecode accepted, and StackMap table generated Reviewed-by: jlahoda - PR: https://git.openjdk.org/jdk/pull/16685
Integrated: 8304446: javap --system flag doesn't override system APIs
On Thu, 2 Nov 2023 13:49:17 GMT, Adam Sotona wrote: > Javap ignores --system option when searching for JDK classes. > This patch prepends search over JDK system modules. > > Please review. > > Thanks, > Adam This pull request has now been integrated. Changeset: 604d29a8 Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/604d29a8c911c1064ba0fab17f9192bb4e640709 Stats: 13 lines in 1 file changed: 13 ins; 0 del; 0 mod 8304446: javap --system flag doesn't override system APIs Reviewed-by: jlahoda - PR: https://git.openjdk.org/jdk/pull/16476
Re: RFR: 8308753: Class-File API transition to Preview [v29]
> Classfile API is an internal library under package `jdk.internal.classfile` > in JDK 21. > This pull request turns the Classfile API into a preview feature and moves it > into `java.lang.classfile`. > It repackages all uses across JDK and tests and adds lots of missing Javadoc. > > This PR goes in sync with > [JDK-8308754](https://bugs.openjdk.org/browse/JDK-8308754): Class-File API > (Preview) (CSR) > and [JDK-8280389](https://bugs.openjdk.org/browse/JDK-8280389): Class-File > API (Preview) (JEP). > > Online javadoc is available at: > https://cr.openjdk.org/~asotona/JDK-8308753-preview/api/java.base/java/lang/classfile/package-summary.html > > In addition to the primary transition to preview, this pull request also > includes: > - All Classfile* classes ranamed to ClassFile* (based on JEP discussion). > - A new preview feature, `CLASSFILE_API`, has been added. > - Buildsystem tool required a little patch to support annotated modules. > - All JDK modules using the Classfile API are newly participating in the > preview. > - All tests that use the Classfile API now have preview enabled. > - A few Javac tests not allowing preview have been partially reverted; their > conversion can be re-applied when the Classfile API leaves preview mode. > > Despite the number of affected files, this pull request is relatively > straight-forward. The preview version of the Classfile API is based on the > internal version of the library from the JDK master branch, and there are no > API features added. > > Please review this pull request to help the Classfile API turn into a preview. > > Any comments are welcome. > > Thanks, > Adam Adam Sotona has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 365 commits: - Merge branch 'master' into JDK-8308753-preview # Conflicts: #test/langtools/tools/javac/7199823/InnerClassCannotBeVerified.java - added new package-info snippets and fixed ClassFile::parse throws javadoc description - fixed lambda tests - Merge branch 'master' into JDK-8308753-preview - fixed SignaturesTest - fixed condy tests - Merge branch 'master' into JDK-8308753-preview - Merge branch 'master' into JDK-8308753-preview # Conflicts: #test/jdk/tools/lib/tests/JImageValidator.java - fixed jdk.jfr - Merge branch 'master' into JDK-8308753-preview # Conflicts: #src/java.base/share/classes/module-info.java - ... and 355 more: https://git.openjdk.org/jdk/compare/6b96bb64...46ad6906 - Changes: https://git.openjdk.org/jdk/pull/15706/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15706=28 Stats: 32339 lines in 784 files changed: 14659 ins; 14109 del; 3571 mod Patch: https://git.openjdk.org/jdk/pull/15706.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15706/head:pull/15706 PR: https://git.openjdk.org/jdk/pull/15706
Re: RFR: 8308753: Class-File API transition to Preview [v28]
> Classfile API is an internal library under package `jdk.internal.classfile` > in JDK 21. > This pull request turns the Classfile API into a preview feature and moves it > into `java.lang.classfile`. > It repackages all uses across JDK and tests and adds lots of missing Javadoc. > > This PR goes in sync with > [JDK-8308754](https://bugs.openjdk.org/browse/JDK-8308754): Class-File API > (Preview) (CSR) > and [JDK-8280389](https://bugs.openjdk.org/browse/JDK-8280389): Class-File > API (Preview) (JEP). > > Online javadoc is available at: > https://cr.openjdk.org/~asotona/JDK-8308753-preview/api/java.base/java/lang/classfile/package-summary.html > > In addition to the primary transition to preview, this pull request also > includes: > - All Classfile* classes ranamed to ClassFile* (based on JEP discussion). > - A new preview feature, `CLASSFILE_API`, has been added. > - Buildsystem tool required a little patch to support annotated modules. > - All JDK modules using the Classfile API are newly participating in the > preview. > - All tests that use the Classfile API now have preview enabled. > - A few Javac tests not allowing preview have been partially reverted; their > conversion can be re-applied when the Classfile API leaves preview mode. > > Despite the number of affected files, this pull request is relatively > straight-forward. The preview version of the Classfile API is based on the > internal version of the library from the JDK master branch, and there are no > API features added. > > Please review this pull request to help the Classfile API turn into a preview. > > Any comments are welcome. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: added new package-info snippets and fixed ClassFile::parse throws javadoc description - Changes: - all: https://git.openjdk.org/jdk/pull/15706/files - new: https://git.openjdk.org/jdk/pull/15706/files/8c80afff..3860e6c9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15706=27 - incr: https://webrevs.openjdk.org/?repo=jdk=15706=26-27 Stats: 43 lines in 3 files changed: 36 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/15706.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15706/head:pull/15706 PR: https://git.openjdk.org/jdk/pull/15706
Re: RFR: 8320222: Wrong bytecode accepted, and StackMap table generated [v2]
On Thu, 16 Nov 2023 11:25:07 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> changed StackMapGenerator::generatorError to return an exception instead
>> of directly throw
>
> src/java.base/share/classes/jdk/internal/classfile/impl/StackMapGenerator.java
> line 1138:
>
>> 1136: }
>> 1137: if (stackSize != target.stackSize) {
>> 1138: generatorError("Stack size mismatch");
>
> Just a side comment, `generatorError` should return a Throwable than throw
> directly, so when used as `throw generatorError()`, it is more clear that the
> code branch will terminate to javac.
Good point, changed, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16685#discussion_r1399513530
Re: RFR: 8320222: Wrong bytecode accepted, and StackMap table generated [v2]
> Stack map generator in ClassFile API performs only minimal checks in favour > of performance. > However it led to situations where it generates invalid stack maps for > corrupted code. > This patch adds basic checks of stack when two frames are merged and throws > an exception in case of stack size or content mismatch. Generated or > transformed code with inconsistent stack will fail on stack maps generation. > Relevant tests are added. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: changed StackMapGenerator::generatorError to return an exception instead of directly throw - Changes: - all: https://git.openjdk.org/jdk/pull/16685/files - new: https://git.openjdk.org/jdk/pull/16685/files/594e4797..3c3ac9be Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16685=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16685=00-01 Stats: 27 lines in 1 file changed: 0 ins; 0 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/16685.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16685/head:pull/16685 PR: https://git.openjdk.org/jdk/pull/16685
RFR: 8320222: Wrong bytecode accepted, and StackMap table generated
Stack map generator in ClassFile API performs only minimal checks in favour of performance. However it led to situations where it generates invalid stack maps for corrupted code. This patch adds basic checks of stack when two frames are merged and throws an exception in case of stack size or content mismatch. Generated or transformed code with inconsistent stack will fail on stack maps generation. Relevant tests are added. Please review. Thanks, Adam - Commit messages: - 8320222: Wrong bytecode accepted, and StackMap table generated Changes: https://git.openjdk.org/jdk/pull/16685/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16685=00 Issue: https://bugs.openjdk.org/browse/JDK-8320222 Stats: 49 lines in 2 files changed: 46 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/16685.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16685/head:pull/16685 PR: https://git.openjdk.org/jdk/pull/16685
Re: RFR: 8304446: javap --system flag doesn't override system APIs [v3]
> Javap ignores --system option when searching for JDK classes. > This patch prepends search over JDK system modules. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: applied suggested changes - Changes: - all: https://git.openjdk.org/jdk/pull/16476/files - new: https://git.openjdk.org/jdk/pull/16476/files/78dc9840..1141201d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16476=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16476=01-02 Stats: 12 lines in 1 file changed: 4 ins; 3 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/16476.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16476/head:pull/16476 PR: https://git.openjdk.org/jdk/pull/16476
Re: RFR: 8304446: javap --system flag doesn't override system APIs [v2]
On Fri, 10 Nov 2023 07:28:30 GMT, Jan Lahoda wrote: > Is there a chance for a jtreg test? Unfortunately I haven't found a simple way to include a different JDK as `--system` for the test. Any suggestions are welcome. - PR Comment: https://git.openjdk.org/jdk/pull/16476#issuecomment-1805312908
Re: RFR: 8304446: javap --system flag doesn't override system APIs [v2]
On Fri, 10 Nov 2023 07:25:45 GMT, Jan Lahoda wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Skip search over JDK modules for classes in unnamed package (mainly to
>> skip JDK module-infos)
>
> src/jdk.jdeps/share/classes/com/sun/tools/javap/JavapTask.java line 28:
>
>> 26: package com.sun.tools.javap;
>> 27:
>> 28: import com.sun.tools.javac.file.Locations;
>
> Nit: seems unused.
Removed, thanks.
> src/jdk.jdeps/share/classes/com/sun/tools/javap/JavapTask.java line 864:
>
>> 862: fo = fileManager.getJavaFileForInput(moduleLocation,
>> className, JavaFileObject.Kind.CLASS);
>> 863: } else {
>> 864: if (className.indexOf('.') > 0) try {
>
> I would put the try into a block - it is slightly longer, but much clear, I
> think.
Fixed, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16476#discussion_r1389076255
PR Review Comment: https://git.openjdk.org/jdk/pull/16476#discussion_r1389076418
Re: RFR: 8308753: Class-File API transition to Preview [v27]
> Classfile API is an internal library under package `jdk.internal.classfile` > in JDK 21. > This pull request turns the Classfile API into a preview feature and moves it > into `java.lang.classfile`. > It repackages all uses across JDK and tests and adds lots of missing Javadoc. > > This PR goes in sync with > [JDK-8308754](https://bugs.openjdk.org/browse/JDK-8308754): Class-File API > (Preview) (CSR) > and [JDK-8280389](https://bugs.openjdk.org/browse/JDK-8280389): Class-File > API (Preview) (JEP). > > Online javadoc is available at: > https://cr.openjdk.org/~asotona/JDK-8308753-preview/api/java.base/java/lang/classfile/package-summary.html > > In addition to the primary transition to preview, this pull request also > includes: > - All Classfile* classes ranamed to ClassFile* (based on JEP discussion). > - A new preview feature, `CLASSFILE_API`, has been added. > - Buildsystem tool required a little patch to support annotated modules. > - All JDK modules using the Classfile API are newly participating in the > preview. > - All tests that use the Classfile API now have preview enabled. > - A few Javac tests not allowing preview have been partially reverted; their > conversion can be re-applied when the Classfile API leaves preview mode. > > Despite the number of affected files, this pull request is relatively > straight-forward. The preview version of the Classfile API is based on the > internal version of the library from the JDK master branch, and there are no > API features added. > > Please review this pull request to help the Classfile API turn into a preview. > > Any comments are welcome. > > Thanks, > Adam Adam Sotona has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 363 commits: - fixed lambda tests - Merge branch 'master' into JDK-8308753-preview - fixed SignaturesTest - fixed condy tests - Merge branch 'master' into JDK-8308753-preview - Merge branch 'master' into JDK-8308753-preview # Conflicts: #test/jdk/tools/lib/tests/JImageValidator.java - fixed jdk.jfr - Merge branch 'master' into JDK-8308753-preview # Conflicts: #src/java.base/share/classes/module-info.java - applied javadoc fix suggestions - fixed new benchmark - ... and 353 more: https://git.openjdk.org/jdk/compare/b0fc8082...8c80afff - Changes: https://git.openjdk.org/jdk/pull/15706/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15706=26 Stats: 32311 lines in 785 files changed: 14623 ins; 14113 del; 3575 mod Patch: https://git.openjdk.org/jdk/pull/15706.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15706/head:pull/15706 PR: https://git.openjdk.org/jdk/pull/15706
