Re: RFR: 8308842: Consolidate exceptions thrown from Class-File API [v2]
> Class-File API actually throws wide variety of exceptions based on the actual > situation. Complete error handling code must cover > `IndexOutOfBoundsException`, `IllegalStateException` and > `IllegalArgumentException`. > > Based on previous discussions we decided to consolidate on > `IllegalArgumentException` with possible sub-classes. > > It allows easy common error handling in majority of use case, however it > allows also to distinguish source of the error when needed (for example > `javap` needs to know if the exception comes from constant poll or not). > > Newly introduced `ConstantPoolException` extends `IllegalArgumentException` > to indicate the source of the problem is in constant pool. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: fixed javadoc - Changes: - all: https://git.openjdk.org/jdk/pull/14143/files - new: https://git.openjdk.org/jdk/pull/14143/files/d2ec1c99..8bc444df Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14143=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14143=00-01 Stats: 6 lines in 1 file changed: 1 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/14143.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14143/head:pull/14143 PR: https://git.openjdk.org/jdk/pull/14143
RFR: 8308856: jdk.internal.classfile.impl.EntryMap::nextPowerOfTwo math problem
Fix of jdk.internal.classfile.impl.EntryMap::nextPowerOfTwo returning correct zero power of two. Please review. Thanks, Adam - Commit messages: - 8308856: jdk.internal.classfile.impl.EntryMap::nextPowerOfTwo math problem Changes: https://git.openjdk.org/jdk/pull/14148/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14148=00 Issue: https://bugs.openjdk.org/browse/JDK-8308856 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14148.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14148/head:pull/14148 PR: https://git.openjdk.org/jdk/pull/14148
RFR: 8308842: Consolidate exceptions thrown from Class-File API
Class-File API actually throws wide variety of exceptions based on the actual situation. Complete error handling code must cover `IndexOutOfBoundsException`, `IllegalStateException` and `IllegalArgumentException`. Based on previous discussions we decided to consolidate on `IllegalArgumentException` with possible sub-classes. It allows easy common error handling in majority of use case, however it allows also to distinguish source of the error when needed (for example `javap` needs to know if the exception comes from constant poll or not). Newly introduced `ConstantPoolException` extends `IllegalArgumentException` to indicate the source of the problem is in constant pool. Please review. Thanks, Adam - Commit messages: - 8308842: Consolidate exceptions thrown from Class-File API Changes: https://git.openjdk.org/jdk/pull/14143/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14143=00 Issue: https://bugs.openjdk.org/browse/JDK-8308842 Stats: 120 lines in 15 files changed: 61 ins; 7 del; 52 mod Patch: https://git.openjdk.org/jdk/pull/14143.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14143/head:pull/14143 PR: https://git.openjdk.org/jdk/pull/14143
Re: RFR: 8294969: Convert jdk.jdeps javap to use the Classfile API [v3]
> javap uses proprietary com.sun.tools.classfile library to parse class files. > > This patch converts javap to use Classfile API. > > 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 217 commits: - fixed build - Merge branch 'master' into JDK-8294969-javap # Conflicts: #src/java.base/share/classes/module-info.java - more consolidation on IllegalArgumentException - Merge branch 'master' into JDK-8294969-javap - fixed TestClassNameWarning - Merge branch 'master' into JDK-8294969-javap # Conflicts: #src/java.base/share/classes/module-info.java - consolidated safeguarding of IAE in javap - consolidation of constant pool originating IAEs and IOOBEs into ConstantPoolException extending IAE - Safeguarding CP errors + test - Merge branch 'master' into JDK-8294969-javap - ... and 207 more: https://git.openjdk.org/jdk/compare/bfcae68e...f704791a - Changes: https://git.openjdk.org/jdk/pull/11411/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11411=02 Stats: 3758 lines in 37 files changed: 896 ins; 1723 del; 1139 mod Patch: https://git.openjdk.org/jdk/pull/11411.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/11411/head:pull/11411 PR: https://git.openjdk.org/jdk/pull/11411
Integrated: 8308549: Classfile API should fail to generate over-sized Code attribute
On Tue, 23 May 2023 12:54:20 GMT, Adam Sotona wrote: > Classfile API allowed to generate Code attribute exceeding the 65k limit. No > exception has been thrown during class generation and the class failed > verification later during class loading. > This patch adds Code size limit check throwing IllegalArgumentException. > The patch also adds similar check for constant pool size limit to avoid > generation class file with corrupted constant pool. > Two new tests are added to check response on oversized Code attribute and > constant pool. > `VerifierImpl` is extended to check Code attribute size as a part of class > verification process. > > Please review. > > Thanks, > Adam This pull request has now been integrated. Changeset: bfcae68e Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/bfcae68ed10e4330c35f5de0bdb2d31e44e2872e Stats: 50 lines in 6 files changed: 45 ins; 0 del; 5 mod 8308549: Classfile API should fail to generate over-sized Code attribute Reviewed-by: mchung - PR: https://git.openjdk.org/jdk/pull/14100
Re: RFR: 8308549: Classfile API should fail to generate over-sized Code attribute [v2]
> Classfile API allowed to generate Code attribute exceeding the 65k limit. No > exception has been thrown during class generation and the class failed > verification later during class loading. > This patch adds Code size limit check throwing IllegalArgumentException. > The patch also adds similar check for constant pool size limit to avoid > generation class file with corrupted constant pool. > Two new tests are added to check response on oversized Code attribute and > constant pool. > `VerifierImpl` is extended to check Code attribute size as a part of class > verification process. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: added check for empty Code + test - Changes: - all: https://git.openjdk.org/jdk/pull/14100/files - new: https://git.openjdk.org/jdk/pull/14100/files/32c00386..d4b21a22 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14100=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14100=00-01 Stats: 12 lines in 4 files changed: 10 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14100.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14100/head:pull/14100 PR: https://git.openjdk.org/jdk/pull/14100
Re: RFR: 8308549: Classfile API should fail to generate over-sized Code attribute
On Wed, 24 May 2023 12:04:05 GMT, Jaikiran Pai wrote:
>> Classfile API allowed to generate Code attribute exceeding the 65k limit. No
>> exception has been thrown during class generation and the class failed
>> verification later during class loading.
>> This patch adds Code size limit check throwing IllegalArgumentException.
>> The patch also adds similar check for constant pool size limit to avoid
>> generation class file with corrupted constant pool.
>> Two new tests are added to check response on oversized Code attribute and
>> constant pool.
>> `VerifierImpl` is extended to check Code attribute size as a part of class
>> verification process.
>>
>> Please review.
>>
>> Thanks,
>> Adam
>
> src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java
> line 314:
>
>> 312:
>> 313: int codeLength = curPc();
>> 314: if (codeLength >= 65536) {
>
> Hello Adam, looking at the JVM spec, section 4.7.3
> https://docs.oracle.com/javase/specs/jvms/se17/html/jvms-4.html#jvms-4.7.3,
> it states:
>
>> The value of code_length must be greater than zero (as the code array must
>> not be empty) and less than 65536.
>
> Do you think this check then should also verify (and throw) if the codeLength
> <= 0?
Right, DirectCodeBuilder can be triggered empty, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14100#discussion_r1204085835
Re: RFR: 8308549: Classfile API should fail to generate over-sized Code attribute
On Tue, 23 May 2023 15:13:04 GMT, Chen Liang wrote: > On a side note, does Classfile API reject methods with too many slots > (locals) (MethodTypeDesc can represent parameter lists with over 255 slots) > or stack (operand)? Classfile API does not perform any extra verifications of MethodTypeDesc and 255 slots is limit for method parameters. Code attribute maxLocals and maxStack have limit of 65535, however it is not checked yet. - PR Comment: https://git.openjdk.org/jdk/pull/14100#issuecomment-1560933070
RFR: 8308549: Classfile API should fail to generate over-sized Code attribute
Classfile API allowed to generate Code attribute exceeding the 65k limit. No exception has been thrown during class generation and the class failed verification later during class loading. This patch adds Code size limit check throwing IllegalArgumentException. The patch also adds similar check for constant pool size limit to avoid generation class file with corrupted constant pool. Two new tests are added to check response on oversized Code attribute and constant pool. `VerifierImpl` is extended to check Code attribute size as a part of class verification process. Please review. Thanks, Adam - Commit messages: - 8308549: Classfile API should fail to generate over-sized Code attribute Changes: https://git.openjdk.org/jdk/pull/14100/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14100=00 Issue: https://bugs.openjdk.org/browse/JDK-8308549 Stats: 40 lines in 4 files changed: 35 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/14100.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14100/head:pull/14100 PR: https://git.openjdk.org/jdk/pull/14100
Integrated: 8308093: Disable language preview features use in JDK
On Mon, 22 May 2023 10:01:55 GMT, Adam Sotona wrote: > This patch disables temporary use of language preview features in JDK. > Temporary enabled language preview features (to allow Pattern Matching for > switch use in the Classfile API library) are no more necessary. > All redundant use of --enable-preview in the Classfile API tests are also > removed. > > Please review. > > Thanks, > Adam This pull request has now been integrated. Changeset: c4408278 Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/c4408278d1012746c91ba4c31068538850c68d30 Stats: 24 lines in 12 files changed: 0 ins; 22 del; 2 mod 8308093: Disable language preview features use in JDK Reviewed-by: liach, erikj, alanb, darcy - PR: https://git.openjdk.org/jdk/pull/14076
RFR: 8308093: Disable language preview features use in JDK
This patch disables temporary use of language preview features in JDK. Temporary enabled language preview features (to allow Pattern Matching for switch use in the Classfile API library) are no more necessary. All redundant use of --enable-preview in the Classfile API tests are also removed. Please review. Thanks, Adam - Commit messages: - 8308093: Disable language preview features use in JDK Changes: https://git.openjdk.org/jdk/pull/14076/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14076=00 Issue: https://bugs.openjdk.org/browse/JDK-8308093 Stats: 24 lines in 12 files changed: 0 ins; 22 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14076.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14076/head:pull/14076 PR: https://git.openjdk.org/jdk/pull/14076
Integrated: 8308410: broken compilation of test\jdk\tools\launcher\exeJliLaunchTest.c
On Fri, 19 May 2023 12:08:55 GMT, Adam Sotona wrote: > JDK-8303669 patch to test\jdk\tools\launcher\exeJliLaunchTest.c broke > compilation on windows. > Unfortunately MSVC does not support variable length arrays. > This patch fixes test\jdk\tools\launcher\exeJliLaunchTest.c to use dynamic > array allocation. > > Please review. > > Thanks, > Adam This pull request has now been integrated. Changeset: 80ef5c22 Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/80ef5c228b0f8a7a881a333c418a5d3068fe5a6e Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod 8308410: broken compilation of test\jdk\tools\launcher\exeJliLaunchTest.c Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/14060
Re: RFR: 8308410: broken compilation of test\jdk\tools\launcher\exeJliLaunchTest.c
On Fri, 19 May 2023 12:08:55 GMT, Adam Sotona wrote: > JDK-8303669 patch to test\jdk\tools\launcher\exeJliLaunchTest.c broke > compilation on windows. > Unfortunately MSVC does not support variable length arrays. > This patch fixes test\jdk\tools\launcher\exeJliLaunchTest.c to use dynamic > array allocation. > > Please review. > > Thanks, > Adam Thank you for quick review. - PR Comment: https://git.openjdk.org/jdk/pull/14060#issuecomment-1554551187
Re: RFR: 8308410: broken compilation of test\jdk\tools\launcher\exeJliLaunchTest.c
On Fri, 19 May 2023 12:35:47 GMT, Alan Bateman wrote:
>> JDK-8303669 patch to test\jdk\tools\launcher\exeJliLaunchTest.c broke
>> compilation on windows.
>> Unfortunately MSVC does not support variable length arrays.
>> This patch fixes test\jdk\tools\launcher\exeJliLaunchTest.c to use dynamic
>> array allocation.
>>
>> Please review.
>>
>> Thanks,
>> Adam
>
> test/jdk/tools/launcher/exeJliLaunchTest.c line 39:
>
>> 37: {
>> 38: //avoid null-terminated array of arguments to test JDK-8303669
>> 39: char **argv = malloc(sizeof(char *) * argc);
>
> If this is building on all platforms then okay but I would have expected
> you'd need a cast here.
I've learned that there are completely opposite rules for C and C++.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14060#discussion_r1198933225
Re: RFR: 8308410: broken compilation of test\jdk\tools\launcher\exeJliLaunchTest.c
On Fri, 19 May 2023 13:01:37 GMT, Adam Sotona wrote:
>> test/jdk/tools/launcher/exeJliLaunchTest.c line 39:
>>
>>> 37: {
>>> 38: //avoid null-terminated array of arguments to test JDK-8303669
>>> 39: char **argv = malloc(sizeof(char *) * argc);
>>
>> If this is building on all platforms then okay but I would have expected
>> you'd need a cast here.
>
> I've learned that there are completely opposite rules for C and C++.
Tier1 tests just passed on all platforms.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14060#discussion_r1198935073
RFR: 8308410: broken compilation of test\jdk\tools\launcher\exeJliLaunchTest.c
JDK-8303669 patch to test\jdk\tools\launcher\exeJliLaunchTest.c broke compilation on windows. Unfortunately MSVC does not support variable length arrays. This patch fixes test\jdk\tools\launcher\exeJliLaunchTest.c to use dynamic array allocation. Please review. Thanks, Adam - Commit messages: - 8308410: broken compilation of test\jdk\tools\launcher\exeJliLaunchTest.c Changes: https://git.openjdk.org/jdk/pull/14060/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14060=00 Issue: https://bugs.openjdk.org/browse/JDK-8308410 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14060.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14060/head:pull/14060 PR: https://git.openjdk.org/jdk/pull/14060
Integrated: 8303669: SelectVersion indexes past the end of the argv array
On Wed, 3 May 2023 12:03:34 GMT, Adam Sotona wrote: > libjli/java.c's SelectVersion method receives argc and argv but ignores argc > in some circumstances an instead checks if *argv == 0 in its while loop, > which results in a segmentation fault if the provided array is not NULL > terminated. > > This patch counts down argc in the while loops instead of looking for zero > termination. > > Please review. > > Thank you, > Adam This pull request has now been integrated. Changeset: fa143148 Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/fa14314853e161c6ca5561be3e1e280691d8fe99 Stats: 10 lines in 3 files changed: 6 ins; 0 del; 4 mod 8303669: SelectVersion indexes past the end of the argv array Reviewed-by: vromero - PR: https://git.openjdk.org/jdk/pull/13775
Re: RFR: 8303669: SelectVersion indexes past the end of the argv array [v2]
On Thu, 4 May 2023 09:17:05 GMT, Adam Sotona wrote: >> libjli/java.c's SelectVersion method receives argc and argv but ignores argc >> in some circumstances an instead checks if *argv == 0 in its while loop, >> which results in a segmentation fault if the provided array is not NULL >> terminated. >> >> This patch counts down argc in the while loops instead of looking for zero >> termination. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > added test Thank you. - PR Comment: https://git.openjdk.org/jdk/pull/13775#issuecomment-1554205049
Re: RFR: 8306842: Classfile API performance improvements [v9]
> Following improvements implemented: > - Switch over `String` replaced with switch single char > - Binary search for frames in `StackMapGenerator` > - `StackMapGenerator.rawHandlers` with pre-calculated offsets > - `ClassEntry` is caching `ClassDesc` symbol > - Caching of type symbols in `NameAndTypeEntry` and `MethodTypeEntry` > - Caching `MethodTypeDesc` in `MethodInfo` implementations > - `StackMapGenerator` and `Utils` delegating to cached `MethodTypeDesc` > instead of custom parsing > > No API change. > > Benchmarks show stack map generation improved by 21% and code generation from > symbols improved by 30%. > > > Benchmark Mode Cnt Score Error Units > GenerateStackMaps.benchmark thrpt 10 407931.202 ± 13101.023 ops/s > RebuildMethodBodies.shared thrpt4 10258.597 ± 383.699 ops/s > RebuildMethodBodies.unshared thrpt47224.543 ± 256.800 ops/s > > > > Benchmark Mode Cnt Score Error Units > GenerateStackMaps.benchmark thrpt 10 495101.110 ± 2389.628 ops/s > RebuildMethodBodies.sharedthrpt 4 13380.272 ± 810.113 ops/s > RebuildMethodBodies.unshared thrpt 49399.863 ± 557.060 ops/s Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: removed obsolete exports - Changes: - all: https://git.openjdk.org/jdk/pull/13671/files - new: https://git.openjdk.org/jdk/pull/13671/files/d99e7ad0..07079387 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13671=08 - incr: https://webrevs.openjdk.org/?repo=jdk=13671=07-08 Stats: 3 lines in 3 files changed: 0 ins; 3 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13671.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13671/head:pull/13671 PR: https://git.openjdk.org/jdk/pull/13671
Integrated: 8306842: Classfile API performance improvements
On Wed, 26 Apr 2023 15:04:50 GMT, Adam Sotona wrote: > Following improvements implemented: > - Switch over `String` replaced with switch single char > - Binary search for frames in `StackMapGenerator` > - `StackMapGenerator.rawHandlers` with pre-calculated offsets > - `ClassEntry` is caching `ClassDesc` symbol > - Caching of type symbols in `NameAndTypeEntry` and `MethodTypeEntry` > - Caching `MethodTypeDesc` in `MethodInfo` implementations > - `StackMapGenerator` and `Utils` delegating to cached `MethodTypeDesc` > instead of custom parsing > > No API change. > > Benchmarks show stack map generation improved by 21% and code generation from > symbols improved by 30%. > > > Benchmark Mode Cnt Score Error Units > GenerateStackMaps.benchmark thrpt 10 407931.202 ± 13101.023 ops/s > RebuildMethodBodies.shared thrpt4 10258.597 ± 383.699 ops/s > RebuildMethodBodies.unshared thrpt47224.543 ± 256.800 ops/s > > > > Benchmark Mode Cnt Score Error Units > GenerateStackMaps.benchmark thrpt 10 495101.110 ± 2389.628 ops/s > RebuildMethodBodies.sharedthrpt 4 13380.272 ± 810.113 ops/s > RebuildMethodBodies.unshared thrpt 49399.863 ± 557.060 ops/s This pull request has now been integrated. Changeset: f4f5542f Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/f4f5542f8d49dbb756f52a281b745c3c2bbc9829 Stats: 589 lines in 28 files changed: 383 ins; 90 del; 116 mod 8306842: Classfile API performance improvements Reviewed-by: redestad - PR: https://git.openjdk.org/jdk/pull/13671
Re: RFR: 8306842: Classfile API performance improvements [v8]
On Wed, 17 May 2023 13:42:32 GMT, Chen Liang 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 34 commits: >> >> - Merge branch 'master' into JDK-8306842-perf-improvements >> >># Conflicts: >># make/RunTests.gmk >># src/java.base/share/classes/jdk/internal/classfile/impl/Util.java >> - LinkedList replaced with ArrayList in benchmarks >> - Apply suggestions from code review >> >>Co-authored-by: Andrey Turbanov >> - changed LinkedList to ArrayList in RebuildMethodBodies benchmark >> - added RepeatedModelTraversal JMH benchmark >> - fixed jmh benchmark parameters >> - fixed StackMapGenerator >> - Apply suggestions from code review >> >>Co-authored-by: liach <7806504+li...@users.noreply.github.com> >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> JDK-8306842-perf-improvements >> >># Conflicts: >># >> src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java >># >> src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java >># >> src/java.base/share/classes/jdk/internal/classfile/impl/StackMapGenerator.java >># test/jdk/jdk/classfile/DiscontinuedInstructionsTest.java >># test/jdk/jdk/classfile/StackMapsTest.java >> - more use of MethodInfo::methodTypeSymbol and faster ClassDesc slot size >> calculation >> - ... and 24 more: https://git.openjdk.org/jdk/compare/5763be72...d99e7ad0 > > test/micro/org/openjdk/bench/jdk/classfile/Write.java line 76: > >> 74: "--add-exports", >> "java.base/jdk.internal.classfile.constantpool=ALL-UNNAMED", >> 75: "--add-exports", >> "java.base/jdk.internal.classfile.instruction=ALL-UNNAMED", >> 76: "--add-exports", >> "java.base/jdk.internal.classfile.java.lang.constant=ALL-UNNAMED", > > Since this package is gone, this export arg can be removed. There are a total > of 3 occurrences I think. Good catch, thanks! - PR Review Comment: https://git.openjdk.org/jdk/pull/13671#discussion_r1197409528
Re: RFR: 8306842: Classfile API performance improvements [v8]
> Following improvements implemented: > - Switch over `String` replaced with switch single char > - Binary search for frames in `StackMapGenerator` > - `StackMapGenerator.rawHandlers` with pre-calculated offsets > - `ClassEntry` is caching `ClassDesc` symbol > - Caching of type symbols in `NameAndTypeEntry` and `MethodTypeEntry` > - Caching `MethodTypeDesc` in `MethodInfo` implementations > - `StackMapGenerator` and `Utils` delegating to cached `MethodTypeDesc` > instead of custom parsing > > No API change. > > Benchmarks show stack map generation improved by 21% and code generation from > symbols improved by 30%. > > > Benchmark Mode Cnt Score Error Units > GenerateStackMaps.benchmark thrpt 10 407931.202 ± 13101.023 ops/s > RebuildMethodBodies.shared thrpt4 10258.597 ± 383.699 ops/s > RebuildMethodBodies.unshared thrpt47224.543 ± 256.800 ops/s > > > > Benchmark Mode Cnt Score Error Units > GenerateStackMaps.benchmark thrpt 10 495101.110 ± 2389.628 ops/s > RebuildMethodBodies.sharedthrpt 4 13380.272 ± 810.113 ops/s > RebuildMethodBodies.unshared thrpt 49399.863 ± 557.060 ops/s Adam Sotona has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 34 commits: - Merge branch 'master' into JDK-8306842-perf-improvements # Conflicts: #make/RunTests.gmk #src/java.base/share/classes/jdk/internal/classfile/impl/Util.java - LinkedList replaced with ArrayList in benchmarks - Apply suggestions from code review Co-authored-by: Andrey Turbanov - changed LinkedList to ArrayList in RebuildMethodBodies benchmark - added RepeatedModelTraversal JMH benchmark - fixed jmh benchmark parameters - fixed StackMapGenerator - Apply suggestions from code review Co-authored-by: liach <7806504+li...@users.noreply.github.com> - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8306842-perf-improvements # Conflicts: # src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java # src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java # src/java.base/share/classes/jdk/internal/classfile/impl/StackMapGenerator.java #test/jdk/jdk/classfile/DiscontinuedInstructionsTest.java #test/jdk/jdk/classfile/StackMapsTest.java - more use of MethodInfo::methodTypeSymbol and faster ClassDesc slot size calculation - ... and 24 more: https://git.openjdk.org/jdk/compare/5763be72...d99e7ad0 - Changes: https://git.openjdk.org/jdk/pull/13671/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13671=07 Stats: 592 lines in 28 files changed: 386 ins; 90 del; 116 mod Patch: https://git.openjdk.org/jdk/pull/13671.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13671/head:pull/13671 PR: https://git.openjdk.org/jdk/pull/13671
Re: RFR: 8306842: Classfile API performance improvements [v7]
On Tue, 16 May 2023 22:00:18 GMT, Claes Redestad wrote: >> I plan to share a backing list in #13186, with API additions so users can >> avoid copying their input immutable parameter list as well. I see no reason >> to use a direct array, for MTD is not passed to and used by the VM, unlike >> MethodType. > > Good, I think it makes perfect sense for the MTD parameter list to be > immutable and shareable without copying. Thank you for the review. Yes, I'm looking forward to see it together with Constants API improvements in one benchamark ;) - PR Review Comment: https://git.openjdk.org/jdk/pull/13671#discussion_r1196525870
Re: RFR: 8306457: Classfile API components implementations should not be exposed [v3]
On Tue, 16 May 2023 18:10:08 GMT, Chen Liang wrote: >> The jdk.internal.classfile.components package's interfaces have >> implementations in their nested classes, which are implicitly public and >> exported with the package. They are now moved to the impl package to avoid >> unwanted exposures. Fixed a few minor javadoc issues in the interfaces as >> well. >> >> In conflict with #13021; one needs updating if the other is merged. > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains five commits: > > - Don't change formatting in the migration > >The changes are done by copying old implementation and removing one level > (4 spaces) of indent. >The constructors of CodeLocalsShifterImpl and CodeStackTrackerImpl are > promoted to public > - Merge branch 'master' into fix/hide-component-impl > - merge indy remap fix > - Merge branch 'master' into fix/hide-component-impl > - 8306457: Classfile API components implementations should not be exposed Looks good, thank you. - Marked as reviewed by asotona (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13541#pullrequestreview-1430647466
Integrated: 8307326: Package jdk.internal.classfile.java.lang.constant become obsolete
On Mon, 15 May 2023 08:38:54 GMT, Adam Sotona wrote: > Package `jdk.internal.classfile.java.lang.constant` containing `ModuleDesc` > and `PackageDesc` become obsolete after > [JDK-8306729](https://bugs.openjdk.org/browse/JDK-8306729). > All references to `jdk.internal.classfile.java.lang.constant.ModuleDesc` and > `jdk.internal.classfile.java.lang.constant.PackageDesc` across all JDK > sources, tests and JMH benchmarks are replaced with > `java.lang.constant.ModuleDesc` and `java.lang.constant.PackageDesc`. > `jdk.internal.classfile.java.lang.constant` package export hooks are removed > from java.base module-info, make files and test headers. > Content of `jdk.internal.classfile.java.lang.constant` package and related > tests under `jdk.classfile` are deleted. > Method references renamed in > [JDK-8306729](https://bugs.openjdk.org/browse/JDK-8306729) are fixed: > - `PackageDesc::packageName` to `PackageDesc::name` > - `PackageDesc::packageInternalName` to `PackageDesc::internalName` > - `ModuleDesc::moduleName` to `ModuleDesc::name`. > > Please review this pull request. > > Thanks, > Adam This pull request has now been integrated. Changeset: 5763be72 Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/5763be726700be322de3bbaf345d80e11936b472 Stats: 503 lines in 46 files changed: 0 ins; 446 del; 57 mod 8307326: Package jdk.internal.classfile.java.lang.constant become obsolete Reviewed-by: erikj, liach - PR: https://git.openjdk.org/jdk/pull/13979
Re: RFR: 8306842: Classfile API performance improvements [v5]
On Mon, 15 May 2023 19:06:16 GMT, Glavo wrote:
>> Adam Sotona has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - changed LinkedList to ArrayList in RebuildMethodBodies benchmark
>> - added RepeatedModelTraversal JMH benchmark
>
> test/micro/org/openjdk/bench/jdk/classfile/RepeatedModelTraversal.java line
> 54:
>
>> 52: @Setup(Level.Trial)
>> 53: public void setup() throws IOException {
>> 54: models = new LinkedList<>();
>
> I think `ArrayList` can be used instead of `LinkedList` here too.
>
> While this may not be an actual improvement, I don't think `LinkedList`
> should be used unless necessary.
I've replaced all `LinkedList` with `ArrayList` in the benchmarks, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13671#discussion_r1194784609
Re: RFR: 8306842: Classfile API performance improvements [v7]
> Following improvements implemented: > - Switch over `String` replaced with switch single char > - Binary search for frames in `StackMapGenerator` > - `StackMapGenerator.rawHandlers` with pre-calculated offsets > - `ClassEntry` is caching `ClassDesc` symbol > - Caching of type symbols in `NameAndTypeEntry` and `MethodTypeEntry` > - Caching `MethodTypeDesc` in `MethodInfo` implementations > - `StackMapGenerator` and `Utils` delegating to cached `MethodTypeDesc` > instead of custom parsing > > No API change. > > Benchmarks show stack map generation improved by 21% and code generation from > symbols improved by 30%. > > > Benchmark Mode Cnt Score Error Units > GenerateStackMaps.benchmark thrpt 10 407931.202 ± 13101.023 ops/s > RebuildMethodBodies.shared thrpt4 10258.597 ± 383.699 ops/s > RebuildMethodBodies.unshared thrpt47224.543 ± 256.800 ops/s > > > > Benchmark Mode Cnt Score Error Units > GenerateStackMaps.benchmark thrpt 10 495101.110 ± 2389.628 ops/s > RebuildMethodBodies.sharedthrpt 4 13380.272 ± 810.113 ops/s > RebuildMethodBodies.unshared thrpt 49399.863 ± 557.060 ops/s Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: LinkedList replaced with ArrayList in benchmarks - Changes: - all: https://git.openjdk.org/jdk/pull/13671/files - new: https://git.openjdk.org/jdk/pull/13671/files/71e7fa98..d353bb33 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13671=06 - incr: https://webrevs.openjdk.org/?repo=jdk=13671=05-06 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/13671.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13671/head:pull/13671 PR: https://git.openjdk.org/jdk/pull/13671
Re: RFR: 8306842: Classfile API performance improvements [v4]
On Mon, 15 May 2023 17:04:54 GMT, Claes Redestad wrote: >> I'm not questioning performance differences between list implementations. >> The implementation of top level list for this benchmark is irrelevant >> because ~10ns difference cannot affect ~100µs benchmark run. > > Fair point. The only counter-point I'd like to make is that these things tend > to percolate and get copied around over to other places where it _could_ > matter, so if it's no big deal I'd be slightly happier if we could avoid ever > using `LinkedList`. Right, fixed, thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/13671#discussion_r1194724816
Re: RFR: 8306842: Classfile API performance improvements [v6]
> Following improvements implemented: > - Switch over `String` replaced with switch single char > - Binary search for frames in `StackMapGenerator` > - `StackMapGenerator.rawHandlers` with pre-calculated offsets > - `ClassEntry` is caching `ClassDesc` symbol > - Caching of type symbols in `NameAndTypeEntry` and `MethodTypeEntry` > - Caching `MethodTypeDesc` in `MethodInfo` implementations > - `StackMapGenerator` and `Utils` delegating to cached `MethodTypeDesc` > instead of custom parsing > > No API change. > > Benchmarks show stack map generation improved by 21% and code generation from > symbols improved by 30%. > > > Benchmark Mode Cnt Score Error Units > GenerateStackMaps.benchmark thrpt 10 407931.202 ± 13101.023 ops/s > RebuildMethodBodies.shared thrpt4 10258.597 ± 383.699 ops/s > RebuildMethodBodies.unshared thrpt47224.543 ± 256.800 ops/s > > > > Benchmark Mode Cnt Score Error Units > GenerateStackMaps.benchmark thrpt 10 495101.110 ± 2389.628 ops/s > RebuildMethodBodies.sharedthrpt 4 13380.272 ± 810.113 ops/s > RebuildMethodBodies.unshared thrpt 49399.863 ± 557.060 ops/s Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Apply suggestions from code review Co-authored-by: Andrey Turbanov - Changes: - all: https://git.openjdk.org/jdk/pull/13671/files - new: https://git.openjdk.org/jdk/pull/13671/files/3b299727..71e7fa98 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13671=05 - incr: https://webrevs.openjdk.org/?repo=jdk=13671=04-05 Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/13671.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13671/head:pull/13671 PR: https://git.openjdk.org/jdk/pull/13671
Re: RFR: 8306842: Classfile API performance improvements [v5]
> Following improvements implemented: > - Switch over `String` replaced with switch single char > - Binary search for frames in `StackMapGenerator` > - `StackMapGenerator.rawHandlers` with pre-calculated offsets > - `ClassEntry` is caching `ClassDesc` symbol > - Caching of type symbols in `NameAndTypeEntry` and `MethodTypeEntry` > - Caching `MethodTypeDesc` in `MethodInfo` implementations > - `StackMapGenerator` and `Utils` delegating to cached `MethodTypeDesc` > instead of custom parsing > > No API change. > > Benchmarks show stack map generation improved by 21% and code generation from > symbols improved by 30%. > > > Benchmark Mode Cnt Score Error Units > GenerateStackMaps.benchmark thrpt 10 407931.202 ± 13101.023 ops/s > RebuildMethodBodies.shared thrpt4 10258.597 ± 383.699 ops/s > RebuildMethodBodies.unshared thrpt47224.543 ± 256.800 ops/s > > > > Benchmark Mode Cnt Score Error Units > GenerateStackMaps.benchmark thrpt 10 495101.110 ± 2389.628 ops/s > RebuildMethodBodies.sharedthrpt 4 13380.272 ± 810.113 ops/s > RebuildMethodBodies.unshared thrpt 49399.863 ± 557.060 ops/s Adam Sotona has updated the pull request incrementally with two additional commits since the last revision: - changed LinkedList to ArrayList in RebuildMethodBodies benchmark - added RepeatedModelTraversal JMH benchmark - Changes: - all: https://git.openjdk.org/jdk/pull/13671/files - new: https://git.openjdk.org/jdk/pull/13671/files/3cbb11c2..3b299727 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13671=04 - incr: https://webrevs.openjdk.org/?repo=jdk=13671=03-04 Stats: 74 lines in 2 files changed: 71 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/13671.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13671/head:pull/13671 PR: https://git.openjdk.org/jdk/pull/13671
Re: RFR: 8306842: Classfile API performance improvements [v4]
On Mon, 15 May 2023 13:42:05 GMT, Claes Redestad wrote:
>> test/micro/org/openjdk/bench/jdk/classfile/RebuildMethodBodies.java line 57:
>>
>>> 55: public void setup() throws IOException {
>>> 56: shared = new LinkedList<>();
>>> 57: unshared = new LinkedList<>();
>>
>> LinkedList should be replaced by ArrayList, as:
>> 1. LinkedList's node wrapper around each object is too heavy, ArrayList has
>> smaller wrapper
>> 2. LinkedList iteration needs to follow links while ArrayList access can be
>> machine optimized
>> 3. ArrayList addition is amortized O(1), not really worse than LinkedList
>> additions.
>
> I have to side with @liach here: `LinkedList` iteration is significantly
> slower than `ArrayList` even though the computational complexity is
> identical. Often by an integer factor. This is due to the sparseness of the
> linked list data structure on heap and the pointer chasing that entails.
>
> For minimum overhead of iteration over an immutable list then turning the
> list into an immutable via `List.copyOf` might be preferable due the JVM's
> ability to optimize over `@Stable` arrays.
>
> Adhoc [JMH
> benchmark](https://gist.github.com/cl4es/1f21812241c47f32a9deaffcc996e8b3):
>
>
> BenchmarkMode CntScoreError Units
> ListIteration.iterateArrayList thrpt 15 188,724 ± 10,903 ops/ms
> ListIteration.iterateImmutableList thrpt 15 230,901 ± 6,513 ops/ms
> ListIteration.iterateLinkedList thrpt 15 58,289 ± 0,497 ops/ms
>
>
> Is it important to fix this? Perhaps not, but in a microbenchmark like this
> the fewer cycles we spend on "stuff" that isn't really part of the thing
> we're testing, the better. Increasingly so as the thing we're testing is
> getting more and more optimized.
I'm not questioning performance differences between list implementations.
The implementation of top level list for this benchmark is irrelevant because
~10ns difference cannot affect ~100µs benchmark run.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13671#discussion_r1193934410
Re: RFR: 8306842: Classfile API performance improvements [v4]
On Fri, 12 May 2023 13:19:44 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fixed jmh benchmark parameters
>
> test/micro/org/openjdk/bench/jdk/classfile/RebuildMethodBodies.java line 57:
>
>> 55: public void setup() throws IOException {
>> 56: shared = new LinkedList<>();
>> 57: unshared = new LinkedList<>();
>
> LinkedList should be replaced by ArrayList, as:
> 1. LinkedList's node wrapper around each object is too heavy, ArrayList has
> smaller wrapper
> 2. LinkedList iteration needs to follow links while ArrayList access can be
> machine optimized
> 3. ArrayList addition is amortized O(1), not really worse than LinkedList
> additions.
Construction size is irrelevant here as each node holds the whole expanded
`ClassModel` instances.
`LinkedList` iterator `next` speed is O(1) and possible `AraryList`
optimisations of the test data iterator have no effect on this benchmark.
Construction speed of `ArrayList` is not O(1) when the `grow` method is called,
however it is also irrelevant as this is unmeasured setup part of the benchmark.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13671#discussion_r1193614917
Re: RFR: 8306457: Classfile API components implementations should not be exposed [v2]
On Sat, 13 May 2023 14:11:11 GMT, Chen Liang wrote: >> The jdk.internal.classfile.components package's interfaces have >> implementations in their nested classes, which are implicitly public and >> exported with the package. They are now moved to the impl package to avoid >> unwanted exposures. Fixed a few minor javadoc issues in the interfaces as >> well. >> >> In conflict with #13021; one needs updating if the other is merged. > > Chen Liang 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 indy remap fix > - Merge branch 'master' into fix/hide-component-impl > - 8306457: Classfile API components implementations should not be exposed I would prefer to keep formatting when moving large block of code from one source file to another. It is very hard to diff and requires deep review otherwise. - PR Review: https://git.openjdk.org/jdk/pull/13541#pullrequestreview-1426101972
RFR: 8307326: Package jdk.internal.classfile.java.lang.constant become obsolete
Package `jdk.internal.classfile.java.lang.constant` containing `ModuleDesc` and `PackageDesc` become obsolete after [JDK-8306729](https://bugs.openjdk.org/browse/JDK-8306729). All references to `jdk.internal.classfile.java.lang.constant.ModuleDesc` and `jdk.internal.classfile.java.lang.constant.PackageDesc` across all JDK sources, tests and JMH benchmarks are replaced with `java.lang.constant.ModuleDesc` and `java.lang.constant.PackageDesc`. `jdk.internal.classfile.java.lang.constant` package export hooks are removed from java.base module-info, make files and test headers. Content of `jdk.internal.classfile.java.lang.constant` package and related tests under `jdk.classfile` are deleted. Method references renamed in [JDK-8306729](https://bugs.openjdk.org/browse/JDK-8306729) are fixed: - `PackageDesc::packageName` to `PackageDesc::name` - `PackageDesc::packageInternalName` to `PackageDesc::internalName` - `ModuleDesc::moduleName` to `ModuleDesc::name`. Please review this pull request. Thanks, Adam - Commit messages: - 8307326: Package jdk.internal.classfile.java.lang.constant become obsolete Changes: https://git.openjdk.org/jdk/pull/13979/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13979=00 Issue: https://bugs.openjdk.org/browse/JDK-8307326 Stats: 503 lines in 46 files changed: 0 ins; 446 del; 57 mod Patch: https://git.openjdk.org/jdk/pull/13979.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13979/head:pull/13979 PR: https://git.openjdk.org/jdk/pull/13979
Re: RFR: 8291966: SwitchBootstrap.typeSwitch could be faster [v2]
On Tue, 17 Jan 2023 15:55:40 GMT, Jan Lahoda wrote:
>> The pattern matching switches are using a bootstrap method
>> `SwitchBootstrap.typeSwitch` to implement the jumps in the switch.
>> Basically, for a switch like:
>>
>> switch (obj) {
>> case String s when s.isEmpty() -> {}
>> case String s -> {}
>> case CharSequence cs -> {}
>> ...
>> }
>>
>>
>> this method will produce a MethodHandle that will be analyze the provided
>> selector value (`obj` in the example), and will return the case index to
>> which the switch should jump. This method also accepts a (re)start index for
>> the search, which is used to handle guards. For example, if the
>> `s.isEmpty()` guard in the above sample returns false, the matching is
>> restarted on the next case.
>>
>> The current implementation is fairly slow, it basically goes through the
>> labels in a loop. The proposal here is to replace that with a MethodHandle
>> structure like this:
>>
>> obj == null ? -1
>> : switch (restartIndex) {
>> case 0 -> obj instanceof String ? 0 : obj instanceof
>> CharSequence ? 2 : ... ;
>> case 1 -> obj instanceof String ? 1 : obj instanceof
>> CharSequence ? 2 : ... ;
>> case 2 -> obj instanceof CharSequence ? 2 : ... ;
>> ...
>> default -> ;
>> }
>>
>>
>> This appear to run faster than the current implementation, using testcase
>> similar to the one used for https://github.com/openjdk/jdk/pull/9746 , these
>> are the results
>>
>> PatternsOptimizationTest.testLegacyIndyLongSwitch thrpt 25 1515989.562
>> ± 32047.918 ops/s
>> PatternsOptimizationTest.testHandleIndyLongSwitch thrpt 25 2630707.585
>> ± 37202.210 ops/s
>>
>> PatternsOptimizationTest.testLegacyIndyShortSwitch thrpt 25 6789310.900
>> ± 61921.636 ops/s
>> PatternsOptimizationTest.testHandleIndyShortSwitch thrpt 25 10771729.464
>> ± 69607.467 ops/s
>>
>>
>> The "LegacyIndy" is the current implementation, "HandleIndy" is the one
>> proposed here. The translation in javac used is the one from #9746 in all
>> cases.
>
> Jan Lahoda has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains four additional commits since
> the last revision:
>
> - Adding comments
> - Improving performance
> - Merge branch 'master' into JDK-8291966
> - 8291966: SwitchBootstrap.typeSwitch could be faster
I did several experiments with generated lookupswitch over class hashes (for
final and sealed class labels only).
My experiments and benchmark results are actually showing following facts:
- traversal expanded to tableswitch (as proposed here) is significantly faster
than circulating over an array
- expanded lookupswitch over class hashes is unfortunately slower than this
proposal, even when benchmarked on large switches containing final or sealed
class labels only
-
Marked as reviewed by asotona (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/9779#pullrequestreview-1424836407
Re: RFR: 8306842: Classfile API performance improvements [v4]
> Following improvements implemented: > - Switch over `String` replaced with switch single char > - Binary search for frames in `StackMapGenerator` > - `StackMapGenerator.rawHandlers` with pre-calculated offsets > - `ClassEntry` is caching `ClassDesc` symbol > - Caching of type symbols in `NameAndTypeEntry` and `MethodTypeEntry` > - Caching `MethodTypeDesc` in `MethodInfo` implementations > - `StackMapGenerator` and `Utils` delegating to cached `MethodTypeDesc` > instead of custom parsing > > No API change. > > Benchmarks show stack map generation improved by 21% and code generation from > symbols improved by 30%. > > > Benchmark Mode Cnt Score Error Units > GenerateStackMaps.benchmark thrpt 10 407931.202 ± 13101.023 ops/s > RebuildMethodBodies.shared thrpt4 10258.597 ± 383.699 ops/s > RebuildMethodBodies.unshared thrpt47224.543 ± 256.800 ops/s > > > > Benchmark Mode Cnt Score Error Units > GenerateStackMaps.benchmark thrpt 10 495101.110 ± 2389.628 ops/s > RebuildMethodBodies.sharedthrpt 4 13380.272 ± 810.113 ops/s > RebuildMethodBodies.unshared thrpt 49399.863 ± 557.060 ops/s Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: fixed jmh benchmark parameters - Changes: - all: https://git.openjdk.org/jdk/pull/13671/files - new: https://git.openjdk.org/jdk/pull/13671/files/5af9f9c4..3cbb11c2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13671=03 - incr: https://webrevs.openjdk.org/?repo=jdk=13671=02-03 Stats: 46 lines in 5 files changed: 32 ins; 9 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/13671.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13671/head:pull/13671 PR: https://git.openjdk.org/jdk/pull/13671
Re: RFR: 8306842: Classfile API performance improvements [v4]
On Tue, 9 May 2023 16:15:27 GMT, Claes Redestad wrote: >> They are added in the `make/RunTests.gmk`: >> https://github.com/openjdk/jdk/pull/13550/files#diff-041bf69ea79b333b9ce99c1f879e398d698538530a35c361500b72631f059233R599-R608 > > That seems misplaced. Please file an RFE to have this cleaned up. > > Each microbenchmark that has to add opens needs to take responsibility for > that themselves and not change the environment for everything else. And all > micros the benchmarks.jar should be runnable standalone, not rely on quirks > in the make runner. All benchmarks fixed and --add-exports removed from make file. Thank you for pointing this out. - PR Review Comment: https://git.openjdk.org/jdk/pull/13671#discussion_r1189664602
Re: RFR: 8306842: Classfile API performance improvements [v3]
> Following improvements implemented: > - Switch over `String` replaced with switch single char > - Binary search for frames in `StackMapGenerator` > - `StackMapGenerator.rawHandlers` with pre-calculated offsets > - `ClassEntry` is caching `ClassDesc` symbol > - Caching of type symbols in `NameAndTypeEntry` and `MethodTypeEntry` > - Caching `MethodTypeDesc` in `MethodInfo` implementations > - `StackMapGenerator` and `Utils` delegating to cached `MethodTypeDesc` > instead of custom parsing > > No API change. > > Benchmarks show stack map generation improved by 21% and code generation from > symbols improved by 30%. > > > Benchmark Mode Cnt Score Error Units > GenerateStackMaps.benchmark thrpt 10 407931.202 ± 13101.023 ops/s > RebuildMethodBodies.shared thrpt4 10258.597 ± 383.699 ops/s > RebuildMethodBodies.unshared thrpt47224.543 ± 256.800 ops/s > > > > Benchmark Mode Cnt Score Error Units > GenerateStackMaps.benchmark thrpt 10 495101.110 ± 2389.628 ops/s > RebuildMethodBodies.sharedthrpt 4 13380.272 ± 810.113 ops/s > RebuildMethodBodies.unshared thrpt 49399.863 ± 557.060 ops/s Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: fixed StackMapGenerator - Changes: - all: https://git.openjdk.org/jdk/pull/13671/files - new: https://git.openjdk.org/jdk/pull/13671/files/0c1d4eb8..5af9f9c4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13671=02 - incr: https://webrevs.openjdk.org/?repo=jdk=13671=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13671.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13671/head:pull/13671 PR: https://git.openjdk.org/jdk/pull/13671
Re: RFR: 8306842: Classfile API performance improvements [v2]
> Following improvements implemented: > - Switch over `String` replaced with switch single char > - Binary search for frames in `StackMapGenerator` > - `StackMapGenerator.rawHandlers` with pre-calculated offsets > - `ClassEntry` is caching `ClassDesc` symbol > - Caching of type symbols in `NameAndTypeEntry` and `MethodTypeEntry` > - Caching `MethodTypeDesc` in `MethodInfo` implementations > - `StackMapGenerator` and `Utils` delegating to cached `MethodTypeDesc` > instead of custom parsing > > No API change. > > Benchmarks show stack map generation improved by 21% and code generation from > symbols improved by 30%. > > > Benchmark Mode Cnt Score Error Units > GenerateStackMaps.benchmark thrpt 10 407931.202 ± 13101.023 ops/s > RebuildMethodBodies.shared thrpt4 10258.597 ± 383.699 ops/s > RebuildMethodBodies.unshared thrpt47224.543 ± 256.800 ops/s > > > > Benchmark Mode Cnt Score Error Units > GenerateStackMaps.benchmark thrpt 10 495101.110 ± 2389.628 ops/s > RebuildMethodBodies.sharedthrpt 4 13380.272 ± 810.113 ops/s > RebuildMethodBodies.unshared thrpt 49399.863 ± 557.060 ops/s 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/13671/files - new: https://git.openjdk.org/jdk/pull/13671/files/ca4c00d4..0c1d4eb8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13671=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13671=00-01 Stats: 4 lines in 4 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/13671.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13671/head:pull/13671 PR: https://git.openjdk.org/jdk/pull/13671
Re: RFR: 8304425: ClassHierarchyResolver from Reflection [v7]
On Tue, 9 May 2023 15:09:54 GMT, Chen Liang wrote:
>> Add API to explore Class Hierarchy with a `ClassLoader` or a `Lookup` with
>> proper privileges, with tests.
>>
>> This addition is useful in case classes at runtime are not loaded from the
>> system class loader, such as Proxy. This is also useful to APIs that
>> generate bytecode with a `Lookup` object, such as a custom
>> single-abstract-method class implementations from a method handle.
>>
>> See
>> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000249.html
>> as well.
>>
>> Current questions, which I wish to discuss with @asotona:
>> 1. Should the resolver fail fast on `IllegalAccessException` from the
>> lookup? This usually indicates the hierarchy resolver is set up improperly,
>> and proceeding may simply yield verification errors in class loading that
>> are hard to track. For bytecode-generating APIs, throwing access errors for
>> the Lookup eagerly is also more preferable than later silent generation
>> failure.
>> 2. Whether the default resolver should be reading from jrt alone, reflection
>> alone, or jrt then reflection. I personally believe reflection alone is more
>> reliable, for classes may redefined with instrumentation or jfr, which may
>> not be reflected in the system resources.
>> 3. In addition, I don't think chaining system class loader reflection after
>> system resource retrieval is really meaningful: is there any case where
>> reflection always works while the system resource retrieval always fails?
>
> Chen Liang has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains 10 additional commits since
> the last revision:
>
> - rename to ofClassLoading/ofResourceParsing
>convert the default class provider to bypass security manager restrictions
> - Merge branch 'master' into hierarchy-resolve
> - Merge branch 'master' into hierarchy-resolve
> - Test both cached and uncached resolvers
> - Update the class hierarchy resolver api as per mailing list last week
> - Merge branch 'master' into hierarchy-resolve
> - Update
> src/java.base/share/classes/jdk/internal/classfile/impl/ClassHierarchyImpl.java
>
>Co-authored-by: Andrey Turbanov
> - Make lookup based resolver throw on illegal access eagerly
> - 8304425: ClassHierarchyResolver from Reflection
> - ClassHierarchyResolver using Reflection
src/java.base/share/classes/jdk/internal/classfile/impl/ClassHierarchyImpl.java
line 143:
> 141: var sm = System.getSecurityManager();
> 142: if (sm != null) {
> 143: return AccessController.doPrivileged(new
> PrivilegedAction<>() {
This is out of my competency, need another reviewer for use of
`PriviledgedAction` in this context.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13082#discussion_r1188773857
Re: RFR: 8294969: Convert jdk.jdeps javap to use the Classfile API [v2]
> javap uses proprietary com.sun.tools.classfile library to parse class files. > > This patch converts javap to use Classfile API. > > 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 215 commits: - more consolidation on IllegalArgumentException - Merge branch 'master' into JDK-8294969-javap - fixed TestClassNameWarning - Merge branch 'master' into JDK-8294969-javap # Conflicts: #src/java.base/share/classes/module-info.java - consolidated safeguarding of IAE in javap - consolidation of constant pool originating IAEs and IOOBEs into ConstantPoolException extending IAE - Safeguarding CP errors + test - Merge branch 'master' into JDK-8294969-javap - fixed tools/javap/TestClassNameWarning.java - fixed tools/javap/T6866657.java and tools/javap/T7186925.java - ... and 205 more: https://git.openjdk.org/jdk/compare/672bade5...7d18b761 - Changes: https://git.openjdk.org/jdk/pull/11411/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11411=01 Stats: 3766 lines in 39 files changed: 904 ins; 1723 del; 1139 mod Patch: https://git.openjdk.org/jdk/pull/11411.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/11411/head:pull/11411 PR: https://git.openjdk.org/jdk/pull/11411
Re: RFR: 8306842: Classfile API performance improvements
On Wed, 26 Apr 2023 15:04:50 GMT, Adam Sotona wrote: > Following improvements implemented: > - Switch over `String` replaced with switch single char > - Binary search for frames in `StackMapGenerator` > - `StackMapGenerator.rawHandlers` with pre-calculated offsets > - `ClassEntry` is caching `ClassDesc` symbol > - Caching of type symbols in `NameAndTypeEntry` and `MethodTypeEntry` > - Caching `MethodTypeDesc` in `MethodInfo` implementations > - `StackMapGenerator` and `Utils` delegating to cached `MethodTypeDesc` > instead of custom parsing > > No API change. > > Benchmarks show stack map generation improved by 21% and code generation from > symbols improved by 30%. > > > Benchmark Mode Cnt Score Error Units > GenerateStackMaps.benchmark thrpt 10 407931.202 ± 13101.023 ops/s > RebuildMethodBodies.shared thrpt4 10258.597 ± 383.699 ops/s > RebuildMethodBodies.unshared thrpt47224.543 ± 256.800 ops/s > > > > Benchmark Mode Cnt Score Error Units > GenerateStackMaps.benchmark thrpt 10 495101.110 ± 2389.628 ops/s > RebuildMethodBodies.sharedthrpt 4 13380.272 ± 810.113 ops/s > RebuildMethodBodies.unshared thrpt 49399.863 ± 557.060 ops/s Caching of `ClassDesc` in `ClassEntry` is already a part of this PR and the internal name is key for `ClassEntry` from the very beginning, so obtaining internal name from `ClassEntry` was never an issue. However `ClassEntry` is a short-live object, newly created for each generated or transformed class and it exists within a constant pool context only. In order to reach the same performance as with cached internal names in `ClassDesc` instances, we would have to change the user coding schema and not the Classfile API internals. Every code generation or transformation would have to start with static declaration of `ClassEntry` constants (instead of `ClassDesc`), constructed from to `TemporaryConstantPool` (which is for internal purpose yet). I've added new benchmarks measuring repeated transformations rebuilding method bodies (down to the symbols expansion) from already expanded models. I’ve added shared and unshared CP alternatives (for curiosity), however the unshared is closer to simulation of building from symbols. Here are results from the actual code base: Benchmark Mode Cnt Score Error Units RebuildMethodBodies.sharedthrpt4 10258.597 ± 383.699 ops/s RebuildMethodBodies.unshared thrpt4 7224.543 ± 256.800 ops/s And here is already visible approx. 22% improvement in both (without the rest of proposed improvements yet): Benchmark Mode Cnt Score Error Units RebuildMethodBodies.sharedthrpt4 12498.597 ± 309.585 ops/s RebuildMethodBodies.unshared thrpt4 8807.229 ± 167.247 ops/s It would be also interesting to see a difference with cached internal names in `ClassDesc`. Now we have following benchamrk numbers: Benchmark Mode Cnt Score Error Units RebuildMethodBodies.sharedthrpt4 13380.272 ± 810.113 ops/s RebuildMethodBodies.unshared thrpt4 9399.863 ± 557.060 ops/s Which is 30% performance improvement :) I benchmarked merged this PR with #13598 and benefits of caching of internal names in ClassDesc are insignificant (~2% performance boost). Problem with ClassDesc "embedding" into the Utf8Entry is with its ambiguity. Utf8Entry mainly contains some name or descriptor (MethodTypeDesc, ClassDesc, PackageDesc, ModuleDesc, etc...) or signature (Signature, MethodSignature, ClassSignature, etc...). And even for ClassDesc there is ambiguity of the serialized form. When the Utf8Entry is related to ClassEntry it contains descriptor for arrays or internal name for classes. However when the Utf8Entry is related for example to annotation it contains always class descriptor (even for classes). >From this perspective the Utf8Entry should not be responsible for conversions >to and from symbols, because it does not know the context and so the right >serialised form. ClassEntry knows the exact form of its Utf8Entry, Annotation, MethodInfo or (with just a minor ambiguity) also NameAndTypeEntry know which symbols to use and how to serialize/deserialize them. I think that having Utf8Entry as a big central dispatcher for so many types of symbols and serialisation forms is not the best design and will include a lot of complexity and confusion. Current architecture lets interaction with symbols responsibility on the right layer. It is unlikely that one Utf8Entry will hold two different symbols, however still possible (for example when Signature and ClassDesc are the same). And it is still very likely that one symbol describing the same class will be stored in two different Utf8Entri
RFR: 8306842: Classfile API performance improvements
Following improvements implemented: - Switch over `String` replaced with switch single char - Binary search for frames in `StackMapGenerator` - `StackMapGenerator.rawHandlers` with pre-calculated offsets - `ClassEntry` is caching `ClassDesc` symbol - Caching of type symbols in `NameAndTypeEntry` and `MethodTypeEntry` - Caching `MethodTypeDesc` in `MethodInfo` implementations - `StackMapGenerator` and `Utils` delegating to cached `MethodTypeDesc` instead of custom parsing No API change. Benchmarks show stack map generation improved by 21% and code generation from symbols improved by 30%. Benchmark Mode Cnt Score Error Units GenerateStackMaps.benchmark thrpt 10 407931.202 ± 13101.023 ops/s RebuildMethodBodies.shared thrpt4 10258.597 ± 383.699 ops/s RebuildMethodBodies.unshared thrpt47224.543 ± 256.800 ops/s Benchmark Mode Cnt Score Error Units GenerateStackMaps.benchmark thrpt 10 495101.110 ± 2389.628 ops/s RebuildMethodBodies.sharedthrpt 4 13380.272 ± 810.113 ops/s RebuildMethodBodies.unshared thrpt 49399.863 ± 557.060 ops/s - Commit messages: - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8306842-perf-improvements - more use of MethodInfo::methodTypeSymbol and faster ClassDesc slot size calculation - merge from master - StackMapGenerator::processInvokeInstructions uses cached MethodTypeDesc - caching MethodTypeDesc in MethodInfo implementations and improved Util methods - getting symbols from NaTEntry moved to new Util::fieldTypeSymbol and Util::methodTypeSymbol methods - new RebuildMethodBodies benchmark - fixed Util::entryList - Apply suggestions from code review - caching type symbols in NameAndTypeEntry and MethodTypeEntry - ... and 16 more: https://git.openjdk.org/jdk/compare/a05560d9...ca4c00d4 Changes: https://git.openjdk.org/jdk/pull/13671/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13671=00 Issue: https://bugs.openjdk.org/browse/JDK-8306842 Stats: 476 lines in 23 files changed: 283 ins; 83 del; 110 mod Patch: https://git.openjdk.org/jdk/pull/13671.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13671/head:pull/13671 PR: https://git.openjdk.org/jdk/pull/13671
Re: RFR: 8306842: Classfile API performance improvements
On Wed, 26 Apr 2023 16:00:07 GMT, Chen Liang wrote:
> 1. CodeBuilder.invokeInstruction and CodeBuilder.fieldInstruction can pass
> their symbols to the NameAndTypeEntry as well, since it's almost always going
> to be used by stack map generation later.
Yes, they actually do. CodeBuilder conventional methods delegate down to the
NaTEntry construction from symbols. I hope I didn't miss any important path.
> 2. Since the casts to access the field and method type symbols in
> `NameAndTypeEntryImpl` is quite complex, can we move it to a utility method
> in `Util` for cleaner call sites?
> 3. `parameterSlots` `parseParameterSlots` `maxLocals` in `Util` can probably
> operate on a `MethodTypeDesc` than a bitset, especially that the method type
> symbols are available in most scenarios already.
> 4. `StackMapGenerator.processInvokeInstructions` can probably scan through
> the `MethodTypeDesc` instead of using the hand-rolled `while (++pos < descLen
> && (ch = mDesc.charAt(pos)) != ')')` loop. In fact, the whole loop can be
> replaced by `Util.parameterSlots()`
> 5. `StackMapGenerator.isDoubleSlot(ClassDesc)` should be moved to `Util` and
> used by `parameterSlots()` etc (assuming they've migrated to
> `MethodTypeDesc`), and implementation be updated to:
>
> ```java
> public static boolean isDoubleSlot(ClassDesc desc) {
> return desc.isPrimitive() && (desc.charAt(0) == 'D' || desc.charAt(0)
> == 'J');
> }
> ```
>
> to avoid potentially slow string comparisons.
These are great suggestions, I'll work on them.
Thanks!
> If we can hash internal names for constant pool without calling
> Util.toInternalName on ClassDesc instances, can that offset the performance
> penalty of toInternalName?
Yes, that may help. There is already manual hash calculation from the raw
bytes, so it should work the same way.
Good idea, thanks.
-
PR Comment: https://git.openjdk.org/jdk/pull/13671#issuecomment-1525075958
PR Comment: https://git.openjdk.org/jdk/pull/13671#issuecomment-1527577715
Integrated: 8305990: Stripping debug info of ASM 9.5 fails
On Fri, 14 Apr 2023 14:02:46 GMT, Adam Sotona wrote: > Classfile API didn't handle transformations of class files version 50 and > below correctly. > > Proposed fix have two parts: > 1. Inflation of branch targets does not depend on StackMapTable attribute > presence for class file version 50 and below. Alternative fallback > implementation is provided. > 2. StackMapTable attribute is not generated for class file versions below 50. > > StackMapsTest is also extended to test this patch. > > Please review. > > Thanks, > Adam This pull request has now been integrated. Changeset: a05560d9 Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/a05560d99352bd5952f3feef37b56dceb74ede3b Stats: 992 lines in 16 files changed: 816 ins; 31 del; 145 mod 8305990: Stripping debug info of ASM 9.5 fails Reviewed-by: mcimadamore - PR: https://git.openjdk.org/jdk/pull/13478
Re: RFR: 8305990: Stripping debug info of ASM 9.5 fails [v14]
> Classfile API didn't handle transformations of class files version 50 and > below correctly. > > Proposed fix have two parts: > 1. Inflation of branch targets does not depend on StackMapTable attribute > presence for class file version 50 and below. Alternative fallback > implementation is provided. > 2. StackMapTable attribute is not generated for class file versions below 50. > > StackMapsTest is also extended to test this patch. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with two additional commits since the last revision: - empty commit to trigger PR update - renamed StackCounter::stack and local to addStackSlot and ensureLocalSlot - Changes: - all: https://git.openjdk.org/jdk/pull/13478/files - new: https://git.openjdk.org/jdk/pull/13478/files/5db0ed01..7d21663c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13478=13 - incr: https://webrevs.openjdk.org/?repo=jdk=13478=12-13 Stats: 71 lines in 1 file changed: 0 ins; 0 del; 71 mod Patch: https://git.openjdk.org/jdk/pull/13478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13478/head:pull/13478 PR: https://git.openjdk.org/jdk/pull/13478
Re: RFR: 8305990: Stripping debug info of ASM 9.5 fails [v13]
On Tue, 9 May 2023 11:59:08 GMT, Maurizio Cimadamore wrote: >> Adam Sotona has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 16 additional >> commits since the last revision: >> >> - fixed StackCounter >> - Merge branch 'master' into JDK-8305990-debug-info-strip-fail >> - implemented failover stackmap generation for class version 50 >>fixed StackMapGenerator error debug print + added clone constructor to >> SplitConstantPool >>adjusted and extended related tests >> - Apply suggestions from code review >> >>Thanks for review. >> >>Co-authored-by: liach <7806504+li...@users.noreply.github.com> >> - Merge branch 'master' into JDK-8305990-debug-info-strip-fail >> - Update StackCounter.java >> - added comments to StackCounter about maxStack upper bounds calculation >> for JSR/RET instructions >> - fixed stack counting of JSR instructions >> - implemented StackCounter >> - Making some BufWriter fields final >> - ... and 6 more: https://git.openjdk.org/jdk/compare/1302ccc8...5db0ed01 > > Seems like GitHub UI does not let me add comments (as GitHub seems to be > experiencing some issues). Here's what I added: > > * StackCounter: `stack` and `local` are different in spirit. One adds new > stack slots. The other ensure there's at least enough local slots available. > As such I'd suggest a renaming `addStackSlot` and `ensureLocalSlot`. > > * StackMapGeneration/DirectClassBuilder: the exception type seems a bit loose > and we might end up catching more than is thrown by the stackmap generator. @mcimadamore thanks for review. I've fixed `stack` and `local`. Exception consolidation is already a part of #11411 and this catch will be changed to `IAE` only. Actually it would have to catch `ISE`, `IAE`, `IOOBE` and probably also `NPE`. - PR Comment: https://git.openjdk.org/jdk/pull/13478#issuecomment-1540021763
Re: RFR: 8305990: Stripping debug info of ASM 9.5 fails [v13]
> Classfile API didn't handle transformations of class files version 50 and > below correctly. > > Proposed fix have two parts: > 1. Inflation of branch targets does not depend on StackMapTable attribute > presence for class file version 50 and below. Alternative fallback > implementation is provided. > 2. StackMapTable attribute is not generated for class file versions below 50. > > StackMapsTest is also extended to test this patch. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 16 additional commits since the last revision: - fixed StackCounter - Merge branch 'master' into JDK-8305990-debug-info-strip-fail - implemented failover stackmap generation for class version 50 fixed StackMapGenerator error debug print + added clone constructor to SplitConstantPool adjusted and extended related tests - Apply suggestions from code review Thanks for review. Co-authored-by: liach <7806504+li...@users.noreply.github.com> - Merge branch 'master' into JDK-8305990-debug-info-strip-fail - Update StackCounter.java - added comments to StackCounter about maxStack upper bounds calculation for JSR/RET instructions - fixed stack counting of JSR instructions - implemented StackCounter - Making some BufWriter fields final - ... and 6 more: https://git.openjdk.org/jdk/compare/e6c0bde0...5db0ed01 - Changes: - all: https://git.openjdk.org/jdk/pull/13478/files - new: https://git.openjdk.org/jdk/pull/13478/files/795314a2..5db0ed01 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13478=12 - incr: https://webrevs.openjdk.org/?repo=jdk=13478=11-12 Stats: 24213 lines in 591 files changed: 18405 ins; 2515 del; 3293 mod Patch: https://git.openjdk.org/jdk/pull/13478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13478/head:pull/13478 PR: https://git.openjdk.org/jdk/pull/13478
Re: RFR: 8305990: Stripping debug info of ASM 9.5 fails [v12]
> Classfile API didn't handle transformations of class files version 50 and > below correctly. > > Proposed fix have two parts: > 1. Inflation of branch targets does not depend on StackMapTable attribute > presence for class file version 50 and below. Alternative fallback > implementation is provided. > 2. StackMapTable attribute is not generated for class file versions below 50. > > StackMapsTest is also extended to test this patch. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: implemented failover stackmap generation for class version 50 fixed StackMapGenerator error debug print + added clone constructor to SplitConstantPool adjusted and extended related tests - Changes: - all: https://git.openjdk.org/jdk/pull/13478/files - new: https://git.openjdk.org/jdk/pull/13478/files/6fbd2719..795314a2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13478=11 - incr: https://webrevs.openjdk.org/?repo=jdk=13478=10-11 Stats: 54 lines in 5 files changed: 31 ins; 11 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/13478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13478/head:pull/13478 PR: https://git.openjdk.org/jdk/pull/13478
Re: RFR: 8305990: Stripping debug info of ASM 9.5 fails [v11]
On Thu, 4 May 2023 16:19:04 GMT, Adam Sotona wrote: >> Classfile API didn't handle transformations of class files version 50 and >> below correctly. >> >> Proposed fix have two parts: >> 1. Inflation of branch targets does not depend on StackMapTable attribute >> presence for class file version 50 and below. Alternative fallback >> implementation is provided. >> 2. StackMapTable attribute is not generated for class file versions below 50. >> >> StackMapsTest is also extended to test this patch. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > Apply suggestions from code review > > Thanks for review. > > Co-authored-by: liach <7806504+li...@users.noreply.github.com> I'm considering to adjust current default stackmap generation behaviour from "when mandatory" to "by class file version". It will try to generate stackmaps also for class version 50 with failover to basic stack counter (exactly following JVMS-4.10). Later it will allow us to add only third option "always" to satisfy also the very specific use cases going beyond the JVMS boundaries. - PR Comment: https://git.openjdk.org/jdk/pull/13478#issuecomment-1535976554
Re: RFR: 8305990: Stripping debug info of ASM 9.5 fails [v11]
On Fri, 5 May 2023 02:50:45 GMT, Chen Liang wrote: > Since we want an option to toggle stackmap generation, will you add it to the > Classfile options as a temporary measure, before we keep track of these > options in a stateful object (with hierarchy resolver etc.) like brian > envisioned? I would prefer to separate multi-state stackmap generation option into a new RFE (or join it with the Brians proposal) and do not delay this fix by discussions about exact number and naming conventions of the option. Current boolean option handles the critical use cases and here proposed default (generate when mandatory) allows seamlessly transform various class versions. - PR Comment: https://git.openjdk.org/jdk/pull/13478#issuecomment-1535868523
Re: RFR: 8305990: Stripping debug info of ASM 9.5 fails [v11]
> Classfile API didn't handle transformations of class files version 50 and > below correctly. > > Proposed fix have two parts: > 1. Inflation of branch targets does not depend on StackMapTable attribute > presence for class file version 50 and below. Alternative fallback > implementation is provided. > 2. StackMapTable attribute is not generated for class file versions below 50. > > StackMapsTest is also extended to test this patch. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Apply suggestions from code review Thanks for review. Co-authored-by: liach <7806504+li...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/13478/files - new: https://git.openjdk.org/jdk/pull/13478/files/f132f737..6fbd2719 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13478=10 - incr: https://webrevs.openjdk.org/?repo=jdk=13478=09-10 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/13478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13478/head:pull/13478 PR: https://git.openjdk.org/jdk/pull/13478
Re: RFR: 8305990: Stripping debug info of ASM 9.5 fails [v10]
On Thu, 4 May 2023 15:19:02 GMT, Chen Liang wrote: > Also, has brian reviewed the DiscontinuedInstruction API changes? https://mail.openjdk.org/pipermail/classfile-api-dev/2023-April/000292.html - PR Comment: https://git.openjdk.org/jdk/pull/13478#issuecomment-1535028314
Re: RFR: 8304937: BufferedFieldBuilder.Model missing writeTo(DirectClassBuilder)
On Sun, 26 Mar 2023 20:35:20 GMT, Chen Liang wrote: > Please review this simple patch to Classfile API that fixes a missing > override that otherwise affects usage of chained class transforms. A test is > included, that it fails on the missing method without this patch. > > Please review a few other patches fixing bugs affecting normal usage of the > Classfile API as well: > #13167 #13021 #12996 > > I am trying to write a test for my in-progress patch to unify parameter > mapping in core reflection in anticipation of #9862, however this bug > prevents me from dropping method Signature and MethodParameters attributes > with chained class transforms. Marked as reviewed by asotona (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13187#pullrequestreview-1412885846
Re: RFR: 8303669: SelectVersion indexes past the end of the argv array
On Thu, 4 May 2023 06:47:50 GMT, Alan Bateman wrote: > Also are you planning to add a test for this? I've added code to `exeJliLaunchTest.c` (executed by `JliLaunchTest`) to test non null terminated arguments. - PR Comment: https://git.openjdk.org/jdk/pull/13775#issuecomment-1534369861
Re: RFR: 8303669: SelectVersion indexes past the end of the argv array [v2]
> libjli/java.c's SelectVersion method receives argc and argv but ignores argc > in some circumstances an instead checks if *argv == 0 in its while loop, > which results in a segmentation fault if the provided array is not NULL > terminated. > > This patch counts down argc in the while loops instead of looking for zero > termination. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: added test - Changes: - all: https://git.openjdk.org/jdk/pull/13775/files - new: https://git.openjdk.org/jdk/pull/13775/files/2c46bb40..395cf2e7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13775=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13775=00-01 Stats: 8 lines in 2 files changed: 6 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13775.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13775/head:pull/13775 PR: https://git.openjdk.org/jdk/pull/13775
Re: RFR: 8303669: SelectVersion indexes past the end of the argv array
On Thu, 4 May 2023 06:26:53 GMT, David Holmes wrote:
>> libjli/java.c's SelectVersion method receives argc and argv but ignores argc
>> in some circumstances an instead checks if *argv == 0 in its while loop,
>> which results in a segmentation fault if the provided array is not NULL
>> terminated.
>>
>> This patch counts down argc in the while loops instead of looking for zero
>> termination.
>>
>> Please review.
>>
>> Thank you,
>> Adam
>
> src/java.base/share/native/libjli/java.c line 1212:
>
>> 1210: *pret = 0;
>> 1211:
>> 1212: while (argc > 0 && *(arg = *argv) == '-') {
>
> AFAICS this loop terminates at line 1388 and nowhere in the loop body does
> argc get modified. ??
Reference to argc is passed down to GetOpt, where it is updated.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13775#discussion_r1184633184
RFR: 8303669: SelectVersion indexes past the end of the argv array
libjli/java.c's SelectVersion method receives argc and argv but ignores argc in some circumstances an instead checks if *argv == 0 in its while loop, which results in a segmentation fault if the provided array is not NULL terminated. This patch counts down argc in the while loops instead of looking for zero termination. Please review. Thank you, Adam - Commit messages: - 8303669: SelectVersion indexes past the end of the argv array Changes: https://git.openjdk.org/jdk/pull/13775/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13775=00 Issue: https://bugs.openjdk.org/browse/JDK-8303669 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13775.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13775/head:pull/13775 PR: https://git.openjdk.org/jdk/pull/13775
Integrated: 8306729: Add nominal descriptors of modules and packages to Constants API
On Mon, 24 Apr 2023 11:59:03 GMT, Adam Sotona wrote: > Constants API already provides models for all loadable constants to help > programs manipulating class files and modelling bytecode instructions. > However no models of module and package constants are provided by Constants > API. Every program manipulating class files must implement own models and > validation of modules and packages constants. > > This pul request adds `java.lang.constant.ModuleDesc` and > `java.lang.constant.PackageDesc` to the Constants API. > > Classfile API will follow up and remove its internal implementations of > `PackageDesc` and `ModuleDesc`. > > Please review this pull request and attached CSR. > > Thank you, > Adam This pull request has now been integrated. Changeset: c8f37564 Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/c8f37564bf0983f449195434378479e1adfc1466 Stats: 467 lines in 8 files changed: 467 ins; 0 del; 0 mod 8306729: Add nominal descriptors of modules and packages to Constants API Reviewed-by: mchung - PR: https://git.openjdk.org/jdk/pull/13615
Re: RFR: 8305990: Stripping debug info of ASM 9.5 fails [v10]
> Classfile API didn't handle transformations of class files version 50 and > below correctly. > > Proposed fix have two parts: > 1. Inflation of branch targets does not depend on StackMapTable attribute > presence for class file version 50 and below. Alternative fallback > implementation is provided. > 2. StackMapTable attribute is not generated for class file versions below 50. > > StackMapsTest is also extended to test this patch. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision: - Merge branch 'master' into JDK-8305990-debug-info-strip-fail - Update StackCounter.java - added comments to StackCounter about maxStack upper bounds calculation for JSR/RET instructions - fixed stack counting of JSR instructions - implemented StackCounter - Making some BufWriter fields final - Update src/java.base/share/classes/jdk/internal/classfile/impl/AbstractInstruction.java Co-authored-by: liach <7806504+li...@users.noreply.github.com> - Update src/java.base/share/classes/jdk/internal/classfile/Opcode.java Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - fixed ClassPrinterImpl - DiscontinuedInstruction implementation + test - ... and 2 more: https://git.openjdk.org/jdk/compare/9ce19553...f132f737 - Changes: - all: https://git.openjdk.org/jdk/pull/13478/files - new: https://git.openjdk.org/jdk/pull/13478/files/42b96b2c..f132f737 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13478=09 - incr: https://webrevs.openjdk.org/?repo=jdk=13478=08-09 Stats: 285823 lines in 2346 files changed: 252875 ins; 16769 del; 16179 mod Patch: https://git.openjdk.org/jdk/pull/13478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13478/head:pull/13478 PR: https://git.openjdk.org/jdk/pull/13478
Re: RFR: 8306455: Wrong majorVersion for multiple attributes in Classfile API
On Wed, 19 Apr 2023 14:31:36 GMT, Chen Liang wrote: > Spotted the typo for BootstrapMethods, and another review over the API > exposed the typo on PermittedSubclasses. BootstrapMethods is a good catch, however I would rather recommend to remove the `AttributeMapper::validSince` from the API completely. Classfile API has no use of it and it does not perform any checks based on this value. Also the value itself is related to the major version only, so it does not allow to model preview features. PermittedSubclasses attribute is a preview part of JDK 15, see: https://openjdk.org/jeps/360 Real model of attributes and features versioning would have to be more complex and linked with some form of validation. - Changes requested by asotona (Committer). PR Review: https://git.openjdk.org/jdk/pull/13536#pullrequestreview-140887
Re: RFR: 8306697: Add method to obtain String for CONSTANT_Class_info in ClassDesc [v2]
On Sat, 22 Apr 2023 18:40:45 GMT, Chen Liang wrote: >> Add a method `internalName` to `ClassDesc`, and unifies handling of string >> representation of a class constant in CONSTANT_Class_info via >> `ofInternalName` and `internalName` APIs, documented in `ClassDesc` itself. >> In particular, `ofInternalName` now accepts arrays. >> >> The motivation of this API is that avoiding frequent String creations via >> caching (enabled by this new API, will be in a separate patch) would speed >> up Classfile API's [writing of simple class >> files](https://github.com/openjdk/jdk/blob/master/test/micro/org/openjdk/bench/jdk/classfile/Write.java) >> by 1/3. See >> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-April/000296.html >> for more context. >> >> This API is futureproof: for Valhalla's Q-types, it will return their string >> representation in CONSTANT_Class_info, which is most likely their full >> descriptor string. >> >> Javadoc: >> https://cr.openjdk.org/~liach/8306697/java.base/java/lang/constant/ClassDesc.html > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Unify ofInternalName and internalName, document about CONSTANT_Class_info, > remove misleading JVMS 4.4.1 links I benchmarked merged this PR with #13671 and benefits of internal names caching in ClassDesc are insignificant (~2% performance boost). So I'm taking back my disagreement with closing this PR. - PR Comment: https://git.openjdk.org/jdk/pull/13598#issuecomment-1527330005
Re: RFR: 8306697: Add method to obtain String for CONSTANT_Class_info in ClassDesc [v2]
On Thu, 27 Apr 2023 18:49:03 GMT, Brian Goetz wrote: > With the upcoming refactor to make parse/build instance methods, there is a > logical place and lifetime for caches. > […](#) @briangoetz This is a different level of caches than the discussed ClassHierarchyResolver cache. Most of the class generation code starts with static ClassDesc constants (or references to ConstantDescs). Internal caching of class internal name within each single ClassDesc (instead of repeated sub-stringing by an external utility method) has visible performance impact. From each of the static ClassDesc instances there are thousands of calls of their conversions to internal class names and the conversion always create a new sub-string instance. I don't agree with closing this PR, its function is complementary to the other performance improvements. - PR Comment: https://git.openjdk.org/jdk/pull/13598#issuecomment-1527028304
Re: RFR: 8306697: Add method to obtain String for CONSTANT_Class_info in ClassDesc [v2]
On Sat, 22 Apr 2023 18:40:45 GMT, Chen Liang wrote: >> Add a method `internalName` to `ClassDesc`, and unifies handling of string >> representation of a class constant in CONSTANT_Class_info via >> `ofInternalName` and `internalName` APIs, documented in `ClassDesc` itself. >> In particular, `ofInternalName` now accepts arrays. >> >> The motivation of this API is that avoiding frequent String creations via >> caching (enabled by this new API, will be in a separate patch) would speed >> up Classfile API's [writing of simple class >> files](https://github.com/openjdk/jdk/blob/master/test/micro/org/openjdk/bench/jdk/classfile/Write.java) >> by 1/3. See >> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-April/000296.html >> for more context. >> >> This API is futureproof: for Valhalla's Q-types, it will return their string >> representation in CONSTANT_Class_info, which is most likely their full >> descriptor string. >> >> Javadoc: >> https://cr.openjdk.org/~liach/8306697/java.base/java/lang/constant/ClassDesc.html > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Unify ofInternalName and internalName, document about CONSTANT_Class_info, > remove misleading JVMS 4.4.1 links One comment to the topic of if internal name should include array descriptors. It should not include array descriptors if it is called internal name, however even with excluded arrays the method will help a lot by caching the internal names and avoiding repeated substringing. Or it can include also array descriptors if it is named differently ;) Classfile API will benefit from both ways. - PR Comment: https://git.openjdk.org/jdk/pull/13598#issuecomment-1525266128
Re: RFR: 8306697: Add method to obtain String for CONSTANT_Class_info in ClassDesc [v2]
On Sat, 22 Apr 2023 18:40:45 GMT, Chen Liang wrote:
>> Add a method `internalName` to `ClassDesc`, and unifies handling of string
>> representation of a class constant in CONSTANT_Class_info via
>> `ofInternalName` and `internalName` APIs, documented in `ClassDesc` itself.
>> In particular, `ofInternalName` now accepts arrays.
>>
>> The motivation of this API is that avoiding frequent String creations via
>> caching (enabled by this new API, will be in a separate patch) would speed
>> up Classfile API's [writing of simple class
>> files](https://github.com/openjdk/jdk/blob/master/test/micro/org/openjdk/bench/jdk/classfile/Write.java)
>> by 1/3. See
>> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-April/000296.html
>> for more context.
>>
>> This API is futureproof: for Valhalla's Q-types, it will return their string
>> representation in CONSTANT_Class_info, which is most likely their full
>> descriptor string.
>>
>> Javadoc:
>> https://cr.openjdk.org/~liach/8306697/java.base/java/lang/constant/ClassDesc.html
>
> Chen Liang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Unify ofInternalName and internalName, document about CONSTANT_Class_info,
> remove misleading JVMS 4.4.1 links
src/java.base/share/classes/java/lang/constant/ReferenceClassDescImpl.java line
60:
> 58: @Override
> 59: public String internalName() {
> 60: return isArray() ? descriptorString() :
> descriptorString().substring(1, descriptorString().length() - 1);
I suggest to cache the internal name to avoid repeated substring-ing with each
call.
`ClassDesc` instances are frequently used as static constants serving for
generations or transformations of many classes, methods, instructions.
It is highly expected that if this method is invoked at least once on the
particular instance, it will be invoked many times on the same instance.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13598#discussion_r1178855034
Re: RFR: 8306697: Add method to obtain String for CONSTANT_Class_info in ClassDesc [v2]
On Sat, 22 Apr 2023 18:40:45 GMT, Chen Liang wrote: >> Add a method `internalName` to `ClassDesc`, and unifies handling of string >> representation of a class constant in CONSTANT_Class_info via >> `ofInternalName` and `internalName` APIs, documented in `ClassDesc` itself. >> In particular, `ofInternalName` now accepts arrays. >> >> The motivation of this API is that avoiding frequent String creations via >> caching (enabled by this new API, will be in a separate patch) would speed >> up Classfile API's [writing of simple class >> files](https://github.com/openjdk/jdk/blob/master/test/micro/org/openjdk/bench/jdk/classfile/Write.java) >> by 1/3. See >> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-April/000296.html >> for more context. >> >> This API is futureproof: for Valhalla's Q-types, it will return their string >> representation in CONSTANT_Class_info, which is most likely their full >> descriptor string. >> >> Javadoc: >> https://cr.openjdk.org/~liach/8306697/java.base/java/lang/constant/ClassDesc.html > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Unify ofInternalName and internalName, document about CONSTANT_Class_info, > remove misleading JVMS 4.4.1 links I support this addition. Classfile API contains utility method obtaining internal name from `ClassDesc` and it is one of the core pieces. I don't have any exact measured numbers in my hands now, however `String` conversions are significant CPU consumers in our benchmarks. In #13671 we are targeting to support conversion-less paths for class building and transformations (by passing the original `ClassDesc` instances through the process). External utility for conversions from `ClassDesc` to internal name does not allow to effectively cache the names and so every repeated need of internal name of the same `ClassDesc` instance will produce a new substring. - PR Comment: https://git.openjdk.org/jdk/pull/13598#issuecomment-1525168031
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v13]
On Wed, 26 Apr 2023 18:15:34 GMT, Alan Bateman wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> removed prefixes from name methods
>
> src/java.base/share/classes/java/lang/constant/ConstantUtils.java line 133:
>
>> 131: if ((ch >= '\u' && ch <= '\u001F')
>> 132: || ((ch == '\\' || ch == ':' || ch =='@') && (i == 0 ||
>> name.charAt(--i) != '\\')))
>> 133: throw new IllegalArgumentException("Invalid module
>> name: " + name);
>
> test/jdk/java/lang/module/ModuleNames.java has tables of legal and illegal
> module names, including tests that escape backslash, @, and :. It might be
> useful to run these tests on this method.
I've updated `ModuleDescTest` with all the positive and negative cases from
`test/jdk/java/lang/module/ModuleNames.java`, except for the empty name.
Thanks for the review.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1178736304
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v15]
> Constants API already provides models for all loadable constants to help > programs manipulating class files and modelling bytecode instructions. > However no models of module and package constants are provided by Constants > API. Every program manipulating class files must implement own models and > validation of modules and packages constants. > > This pul request adds `java.lang.constant.ModuleDesc` and > `java.lang.constant.PackageDesc` to the Constants API. > > Classfile API will follow up and remove its internal implementations of > `PackageDesc` and `ModuleDesc`. > > Please review this pull request and attached CSR. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: tests update - Changes: - all: https://git.openjdk.org/jdk/pull/13615/files - new: https://git.openjdk.org/jdk/pull/13615/files/39980e02..0456f775 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13615=14 - incr: https://webrevs.openjdk.org/?repo=jdk=13615=13-14 Stats: 26 lines in 2 files changed: 17 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/13615.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13615/head:pull/13615 PR: https://git.openjdk.org/jdk/pull/13615
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v14]
> Constants API already provides models for all loadable constants to help > programs manipulating class files and modelling bytecode instructions. > However no models of module and package constants are provided by Constants > API. Every program manipulating class files must implement own models and > validation of modules and packages constants. > > This pul request adds `java.lang.constant.ModuleDesc` and > `java.lang.constant.PackageDesc` to the Constants API. > > Classfile API will follow up and remove its internal implementations of > `PackageDesc` and `ModuleDesc`. > > Please review this pull request and attached CSR. > > 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: Mandy Chung - Changes: - all: https://git.openjdk.org/jdk/pull/13615/files - new: https://git.openjdk.org/jdk/pull/13615/files/efab745b..39980e02 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13615=13 - incr: https://webrevs.openjdk.org/?repo=jdk=13615=12-13 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13615.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13615/head:pull/13615 PR: https://git.openjdk.org/jdk/pull/13615
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v13]
> Constants API already provides models for all loadable constants to help > programs manipulating class files and modelling bytecode instructions. > However no models of module and package constants are provided by Constants > API. Every program manipulating class files must implement own models and > validation of modules and packages constants. > > This pul request adds `java.lang.constant.ModuleDesc` and > `java.lang.constant.PackageDesc` to the Constants API. > > Classfile API will follow up and remove its internal implementations of > `PackageDesc` and `ModuleDesc`. > > Please review this pull request and attached CSR. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: removed prefixes from name methods - Changes: - all: https://git.openjdk.org/jdk/pull/13615/files - new: https://git.openjdk.org/jdk/pull/13615/files/ca91db48..efab745b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13615=12 - incr: https://webrevs.openjdk.org/?repo=jdk=13615=11-12 Stats: 12 lines in 4 files changed: 0 ins; 0 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/13615.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13615/head:pull/13615 PR: https://git.openjdk.org/jdk/pull/13615
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v10]
On Tue, 25 Apr 2023 21:53:44 GMT, Mandy Chung wrote: > I wonder if `packageName()` and `packageInternalName()` methods can simply be > `name()` and `internalName()` as the type name is `PackageDesc` and `package` > prefix seems to be unnecessary. Similarly, `moduleName()` can be `name()`. > Have this be discussed and rejected? I've searched the history of Classfile API discussions and it actually wasn't discussed. I agree the prefix is unnecessary. Thanks for pointing it out. - PR Comment: https://git.openjdk.org/jdk/pull/13615#issuecomment-1523166326
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v12]
> Constants API already provides models for all loadable constants to help > programs manipulating class files and modelling bytecode instructions. > However no models of module and package constants are provided by Constants > API. Every program manipulating class files must implement own models and > validation of modules and packages constants. > > This pul request adds `java.lang.constant.ModuleDesc` and > `java.lang.constant.PackageDesc` to the Constants API. > > Classfile API will follow up and remove its internal implementations of > `PackageDesc` and `ModuleDesc`. > > Please review this pull request and attached CSR. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Update PackageDesc.java - Changes: - all: https://git.openjdk.org/jdk/pull/13615/files - new: https://git.openjdk.org/jdk/pull/13615/files/11c054b3..ca91db48 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13615=11 - incr: https://webrevs.openjdk.org/?repo=jdk=13615=10-11 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13615.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13615/head:pull/13615 PR: https://git.openjdk.org/jdk/pull/13615
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v11]
> Constants API already provides models for all loadable constants to help > programs manipulating class files and modelling bytecode instructions. > However no models of module and package constants are provided by Constants > API. Every program manipulating class files must implement own models and > validation of modules and packages constants. > > This pul request adds `java.lang.constant.ModuleDesc` and > `java.lang.constant.PackageDesc` to the Constants API. > > Classfile API will follow up and remove its internal implementations of > `PackageDesc` and `ModuleDesc`. > > Please review this pull request and attached CSR. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with two additional commits since the last revision: - Update ModuleDesc.java - Apply suggestions from code review Co-authored-by: Mandy Chung - Changes: - all: https://git.openjdk.org/jdk/pull/13615/files - new: https://git.openjdk.org/jdk/pull/13615/files/b37a2394..11c054b3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13615=10 - incr: https://webrevs.openjdk.org/?repo=jdk=13615=09-10 Stats: 5 lines in 2 files changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/13615.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13615/head:pull/13615 PR: https://git.openjdk.org/jdk/pull/13615
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v10]
On Tue, 25 Apr 2023 21:45:57 GMT, Mandy Chung wrote:
>> Adam Sotona has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Update ModuleDesc.java
>> - Update PackageDesc.java
>
> src/java.base/share/classes/java/lang/constant/ModuleDesc.java line 32:
>
>> 30: * A nominal descriptor for a {@code Module} constant.
>> 31: *
>> 32: * @apiNote
>
> can drop `@apiNote` as it's not needed to be. `` if you want to a new
> paragraph.
Set back to `` as suggested, thanks.
> src/java.base/share/classes/java/lang/constant/PackageDesc.java line 32:
>
>> 30: * A nominal descriptor for a {@code Package} constant.
>> 31: *
>> 32: * @apiNote
>
> can drop `@apiNote` as it's not needed to be. `` if you want to a new
> paragraph.
Set back to `` as suggested, thanks.
> src/java.base/share/classes/java/lang/constant/PackageDesc.java line 70:
>
>> 68: * @throws IllegalArgumentException if the name string is not in the
>> 69: * correct format
>> 70: * @jvms 4.2.1 Binary Class and Interface Names
>
> Worth adding `@jvm 4.2.3 Module and Package Names`
Added, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1177640205
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1177639109
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1177639506
Re: RFR: 8304425: ClassHierarchyResolver from Reflection [v6]
On Mon, 20 Mar 2023 15:21:57 GMT, Adam Sotona wrote: >> Marked as reviewed by asotona (Committer). > >> @asotona So should I simply throw an `IllegalAccessError` when the Lookup >> encounters a `IllegalAccessException`, or should I return null? I favor >> throwing an `IllegalAccessError` as the lookup represents bytecode >> accessibility, and shall it fail to access, the generated bytecode will fail >> to access the specified superclass as well. > > Yes, I also prefer throwing `IllegalAccessError` to directly indicate what is > wrong instead of later indirect exceptions. I think `IllegalAccessError` is > not a case for fallback resolver, so it should not be masked. > @asotona Just curious, what's the current state of our new API model of > caching class hierarchy info in a Classfile context object as we've discussed > on the mailing list? Will you create a patch, or should I update this patch > to that model? In the discussion I tried to fine-tune exact naming of the factory methods and the default. Mainly to differentiate when the class is parsed as a resource `ofResourceParsing(ClassLoader loader)` and when it is loaded `ofClassLoading(ClassLoader loader)` Otherwise this patch looks good. The other part of the discussion with proposed `ClassfileReaderWriter` model is out of the scope of this topic, it has much bigger impact on all existing code and I would deferred it at least until this and other open PRs are merged. - PR Comment: https://git.openjdk.org/jdk/pull/13082#issuecomment-1523118391
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v10]
> Constants API already provides models for all loadable constants to help > programs manipulating class files and modelling bytecode instructions. > However no models of module and package constants are provided by Constants > API. Every program manipulating class files must implement own models and > validation of modules and packages constants. > > This pul request adds `java.lang.constant.ModuleDesc` and > `java.lang.constant.PackageDesc` to the Constants API. > > Classfile API will follow up and remove its internal implementations of > `PackageDesc` and `ModuleDesc`. > > Please review this pull request and attached CSR. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with two additional commits since the last revision: - Update ModuleDesc.java - Update PackageDesc.java - Changes: - all: https://git.openjdk.org/jdk/pull/13615/files - new: https://git.openjdk.org/jdk/pull/13615/files/2d28353e..b37a2394 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13615=09 - incr: https://webrevs.openjdk.org/?repo=jdk=13615=08-09 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/13615.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13615/head:pull/13615 PR: https://git.openjdk.org/jdk/pull/13615
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v4]
On Tue, 25 Apr 2023 16:26:01 GMT, Mandy Chung wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added custom toString() methods
>
> src/java.base/share/classes/java/lang/constant/PackageDesc.java line 44:
>
>> 42: * given the name of the package, such as {@code "java.lang"}.
>> 43: *
>> 44: * {@jls 13.1}
>
> Do you mean to reference JLS 6.7?
>
> Suggest to move this reference after `throws` in the see also section.
Fixed, thanks.
> src/java.base/share/classes/java/lang/constant/PackageDesc.java line 62:
>
>> 60: * such as {@code "java/lang"}.
>> 61: *
>> 62: * {@jvms 4.2.1} In this internal form, the ASCII periods (.) that
>> normally
>
> Suggest not to copy JVMS 4.2.1 here but instead just add it to see also
> section.
Fixed, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1176954431
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1176951889
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v3]
On Mon, 24 Apr 2023 22:05:18 GMT, Mandy Chung wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Doc fixes + added null and empty tests
>
> src/java.base/share/classes/java/lang/constant/ModuleDesc.java line 43:
>
>> 41: * given the name of the module.
>> 42: *
>> 43: * {@jvms 4.2.3} Module names are not encoded in "internal form"
>> like
>
> I would avoid copying JVMS 4.2.3 into the javadoc as it might become
> out-of-sync. In addition, adding `@jvms 4.2.3 Module and Package Names` in
> the see also section should be adequate (like the javadoc of
> `ClassDesc::ofInternalName`).
Fixed, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1176954238
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v9]
> Constants API already provides models for all loadable constants to help > programs manipulating class files and modelling bytecode instructions. > However no models of module and package constants are provided by Constants > API. Every program manipulating class files must implement own models and > validation of modules and packages constants. > > This pul request adds `java.lang.constant.ModuleDesc` and > `java.lang.constant.PackageDesc` to the Constants API. > > Classfile API will follow up and remove its internal implementations of > `PackageDesc` and `ModuleDesc`. > > Please review this pull request and attached CSR. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with two additional commits since the last revision: - Update ModuleDesc.java - Update PackageDesc.java - Changes: - all: https://git.openjdk.org/jdk/pull/13615/files - new: https://git.openjdk.org/jdk/pull/13615/files/ecae26cf..2d28353e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13615=08 - incr: https://webrevs.openjdk.org/?repo=jdk=13615=07-08 Stats: 12 lines in 2 files changed: 3 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/13615.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13615/head:pull/13615 PR: https://git.openjdk.org/jdk/pull/13615
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v3]
On Mon, 24 Apr 2023 20:56:03 GMT, Mandy Chung wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Doc fixes + added null and empty tests
>
> src/java.base/share/classes/java/lang/constant/PackageDesc.java line 30:
>
>> 28:
>> 29: /**
>> 30: * A nominal descriptor for a {@code Package} constant {@jvms 4.4.12}.
>
> I suggest to move `{@jvms 4.4.12}` as see also section before `@since 21`.
> See the suggestion below
Fixed, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1176944315
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v8]
> Constants API already provides models for all loadable constants to help > programs manipulating class files and modelling bytecode instructions. > However no models of module and package constants are provided by Constants > API. Every program manipulating class files must implement own models and > validation of modules and packages constants. > > This pul request adds `java.lang.constant.ModuleDesc` and > `java.lang.constant.PackageDesc` to the Constants API. > > Classfile API will follow up and remove its internal implementations of > `PackageDesc` and `ModuleDesc`. > > Please review this pull request and attached CSR. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Update PackageDesc.java - Changes: - all: https://git.openjdk.org/jdk/pull/13615/files - new: https://git.openjdk.org/jdk/pull/13615/files/5f9664f8..ecae26cf Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13615=07 - incr: https://webrevs.openjdk.org/?repo=jdk=13615=06-07 Stats: 20 lines in 1 file changed: 6 ins; 5 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/13615.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13615/head:pull/13615 PR: https://git.openjdk.org/jdk/pull/13615
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v3]
On Mon, 24 Apr 2023 21:59:29 GMT, Mandy Chung wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Doc fixes + added null and empty tests
>
> src/java.base/share/classes/java/lang/constant/ModuleDesc.java line 30:
>
>> 28:
>> 29: /**
>> 30: * A nominal descriptor for a {@code Module} constant {@jvms 4.4.11}.
>
> This inlined link looks a bit awkward in the output.
>
> I suggest to move `{@jvms 4.4.11}` as see also section before `@since 21`.
> See the suggestion below
Fixed, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1176927427
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v7]
> Constants API already provides models for all loadable constants to help > programs manipulating class files and modelling bytecode instructions. > However no models of module and package constants are provided by Constants > API. Every program manipulating class files must implement own models and > validation of modules and packages constants. > > This pul request adds `java.lang.constant.ModuleDesc` and > `java.lang.constant.PackageDesc` to the Constants API. > > Classfile API will follow up and remove its internal implementations of > `PackageDesc` and `ModuleDesc`. > > Please review this pull request and attached CSR. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Update ModuleDesc.java - Changes: - all: https://git.openjdk.org/jdk/pull/13615/files - new: https://git.openjdk.org/jdk/pull/13615/files/b094cb02..5f9664f8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13615=06 - incr: https://webrevs.openjdk.org/?repo=jdk=13615=05-06 Stats: 27 lines in 1 file changed: 1 ins; 21 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/13615.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13615/head:pull/13615 PR: https://git.openjdk.org/jdk/pull/13615
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v6]
> Constants API already provides models for all loadable constants to help > programs manipulating class files and modelling bytecode instructions. > However no models of module and package constants are provided by Constants > API. Every program manipulating class files must implement own models and > validation of modules and packages constants. > > This pul request adds `java.lang.constant.ModuleDesc` and > `java.lang.constant.PackageDesc` to the Constants API. > > Classfile API will follow up and remove its internal implementations of > `PackageDesc` and `ModuleDesc`. > > Please review this pull request and attached CSR. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Update PackageDesc.java - Changes: - all: https://git.openjdk.org/jdk/pull/13615/files - new: https://git.openjdk.org/jdk/pull/13615/files/2c67f83a..b094cb02 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13615=05 - incr: https://webrevs.openjdk.org/?repo=jdk=13615=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13615.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13615/head:pull/13615 PR: https://git.openjdk.org/jdk/pull/13615
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v5]
> Constants API already provides models for all loadable constants to help > programs manipulating class files and modelling bytecode instructions. > However no models of module and package constants are provided by Constants > API. Every program manipulating class files must implement own models and > validation of modules and packages constants. > > This pul request adds `java.lang.constant.ModuleDesc` and > `java.lang.constant.PackageDesc` to the Constants API. > > Classfile API will follow up and remove its internal implementations of > `PackageDesc` and `ModuleDesc`. > > Please review this pull request and attached CSR. > > 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: Mandy Chung - Changes: - all: https://git.openjdk.org/jdk/pull/13615/files - new: https://git.openjdk.org/jdk/pull/13615/files/7c45b343..2c67f83a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13615=04 - incr: https://webrevs.openjdk.org/?repo=jdk=13615=03-04 Stats: 12 lines in 3 files changed: 2 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/13615.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13615/head:pull/13615 PR: https://git.openjdk.org/jdk/pull/13615
Re: RFR: 8305990: Stripping debug info of ASM 9.5 fails [v9]
> Classfile API didn't handle transformations of class files version 50 and > below correctly. > > Proposed fix have two parts: > 1. Inflation of branch targets does not depend on StackMapTable attribute > presence for class file version 50 and below. Alternative fallback > implementation is provided. > 2. StackMapTable attribute is not generated for class file versions below 50. > > StackMapsTest is also extended to test this patch. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Update StackCounter.java - Changes: - all: https://git.openjdk.org/jdk/pull/13478/files - new: https://git.openjdk.org/jdk/pull/13478/files/bd46c69c..42b96b2c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13478=08 - incr: https://webrevs.openjdk.org/?repo=jdk=13478=07-08 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13478/head:pull/13478 PR: https://git.openjdk.org/jdk/pull/13478
Re: RFR: 8305990: Stripping debug info of ASM 9.5 fails [v8]
> Classfile API didn't handle transformations of class files version 50 and > below correctly. > > Proposed fix have two parts: > 1. Inflation of branch targets does not depend on StackMapTable attribute > presence for class file version 50 and below. Alternative fallback > implementation is provided. > 2. StackMapTable attribute is not generated for class file versions below 50. > > StackMapsTest is also extended to test this patch. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: added comments to StackCounter about maxStack upper bounds calculation for JSR/RET instructions - Changes: - all: https://git.openjdk.org/jdk/pull/13478/files - new: https://git.openjdk.org/jdk/pull/13478/files/c1f3e290..bd46c69c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13478=07 - incr: https://webrevs.openjdk.org/?repo=jdk=13478=06-07 Stats: 5 lines in 1 file changed: 1 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/13478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13478/head:pull/13478 PR: https://git.openjdk.org/jdk/pull/13478
Re: RFR: 8305990: Stripping debug info of ASM 9.5 fails [v3]
On Thu, 20 Apr 2023 08:24:40 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/impl/BufWriterImpl.java
>> line 86:
>>
>>> 84: }
>>> 85:
>>> 86: public void setMajorVersion(int majorVersion) {
>>
>> We should ensure the version is not changed once writing has already
>> happened, and the constant pool builder should have access to the major
>> version as well to prevent writing of invalid entries (like condy before
>> java 11)
>
> The class version is set into the BufWriterImpl at the last stage and user
> cannot affect it later.
>
> For the invalid entries I would like to see a use case where it may happen
> unintentionally. Otherwise the Classfile API is not a spec-enforcing library.
> The library should guide to create valid classfile with minimal effort (using
> defaults), however it should also allow to construct whatever classfile, if
> it is user intention.
the version field is now final
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13478#discussion_r1176237298
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v15]
On Mon, 27 Mar 2023 16:43:17 GMT, Adam Sotona wrote: >> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy >> classes and this patch converts it to use Classfile API. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > StackMapGenerator performance improvements I agree, setting this PR back to draft. This pull request have to take into account all actual and planned improvements in Constants API. And then it should isolate performance improvements of Classfile API into a separate pull request. And then it should be proposed back for review with benchmarks. - PR Comment: https://git.openjdk.org/jdk/pull/10991#issuecomment-1521396913
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v4]
> Constants API already provides models for all loadable constants to help > programs manipulating class files and modelling bytecode instructions. > However no models of module and package constants are provided by Constants > API. Every program manipulating class files must implement own models and > validation of modules and packages constants. > > This pul request adds `java.lang.constant.ModuleDesc` and > `java.lang.constant.PackageDesc` to the Constants API. > > Classfile API will follow up and remove its internal implementations of > `PackageDesc` and `ModuleDesc`. > > Please review this pull request and attached CSR. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: added custom toString() methods - Changes: - all: https://git.openjdk.org/jdk/pull/13615/files - new: https://git.openjdk.org/jdk/pull/13615/files/47aa52d1..7c45b343 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13615=03 - incr: https://webrevs.openjdk.org/?repo=jdk=13615=02-03 Stats: 10 lines in 2 files changed: 10 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13615.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13615/head:pull/13615 PR: https://git.openjdk.org/jdk/pull/13615
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v3]
On Tue, 25 Apr 2023 07:38:41 GMT, Adam Sotona wrote: > Note that other `*Impl` classes in `java.lang.constant` perform validation in > their constructors and provide custom `toString()` formatting and they also > don’t use records. BTW: for example `ClassDesc:of(String name)` performs repeated validations and I see it as a performance bug. `ClassDesc::of` calls `ConstantUtils::validateBinaryClassName`, then it performs conversion `binaryToInternal` and calls `ClassDesc::ofDescriptor`, which checks for `ConstantUtils::arrayDepth` and calls `ReferenceClassDescImpl::new`, which again performns validation by calling `ConstantUtils::skipOverFieldSignature` and checking the result. There is plenty of space for performance improvements here, as all the validation and conversion can be done in one pass only. - PR Comment: https://git.openjdk.org/jdk/pull/13615#issuecomment-1521326927
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v3]
On Mon, 24 Apr 2023 16:33:58 GMT, Chen Liang wrote: > Note that other `*Impl` classes in `java.lang.constant` perform validation in > their constructors and provide custom `toString()` formatting and they also > don’t use records. Performing validation in constructors I see as a blocker for potential performance optimisations from trusted code within the `java.lang.constant` package. Custom `toString()` is a good point, returning for example `ModuleDescImpl[moduleName=mymodule]` is not ideal. I'll fix it, thanks. Records are very well designed exactly for this purpose and I'm not aware of any reason to don't use them. - PR Comment: https://git.openjdk.org/jdk/pull/13615#issuecomment-1521300799
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v3]
> Constants API already provides models for all loadable constants to help > programs manipulating class files and modelling bytecode instructions. > However no models of module and package constants are provided by Constants > API. Every program manipulating class files must implement own models and > validation of modules and packages constants. > > This pul request adds `java.lang.constant.ModuleDesc` and > `java.lang.constant.PackageDesc` to the Constants API. > > Classfile API will follow up and remove its internal implementations of > `PackageDesc` and `ModuleDesc`. > > Please review this pull request and attached CSR. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Doc fixes + added null and empty tests - Changes: - all: https://git.openjdk.org/jdk/pull/13615/files - new: https://git.openjdk.org/jdk/pull/13615/files/0ca0b56e..47aa52d1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13615=02 - incr: https://webrevs.openjdk.org/?repo=jdk=13615=01-02 Stats: 32 lines in 6 files changed: 26 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/13615.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13615/head:pull/13615 PR: https://git.openjdk.org/jdk/pull/13615
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v2]
On Mon, 24 Apr 2023 14:35:30 GMT, Roger Riggs wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added links to JVMS and utility methods moved to ConstantUtils
>
> src/java.base/share/classes/java/lang/constant/package-info.java line 92:
>
>> 90: * reading and writing APIs.
>> 91: *
>> 92: * Another members of this package are {@link
>> java.lang.constant.ModuleDesc}
>
> "Another" -> "Other"
Fixed, thanks.
> test/jdk/java/lang/constant/ModuleDescTest.java line 37:
>
>> 35:
>> 36: class ModuleDescTest {
>> 37:
>
> Add tests for empty and null arguments.
added, thanks.
> test/jdk/java/lang/constant/PackageDescTest.java line 39:
>
>> 37:
>> 38: class PackageDescTest {
>> 39: @ParameterizedTest
>
> Add tests for empty and null arguments.
added, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1175522533
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1175521484
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1175521842
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v2]
On Mon, 24 Apr 2023 14:30:46 GMT, Roger Riggs wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added links to JVMS and utility methods moved to ConstantUtils
>
> src/java.base/share/classes/java/lang/constant/ConstantUtils.java line 80:
>
>> 78: /**
>> 79: * Validates the correctness of a binary package name. In particular
>> checks for the presence of
>> 80: * invalid characters in the name.
>
> It may be useful to note that the empty string are considered valid.
> Also add @throws NPE; there's an implicit null check in checking the length.
> Also in validateBinaryPackageName and validateModuleName.
I'll add that notes, thanks.
> src/java.base/share/classes/java/lang/constant/ConstantUtils.java line 130:
>
>> 128: }
>> 129: return name;
>> 130: }
>
> If these are performance sensitive or get used a lot, should there be an
> array of flags indexed by the byte/char to indicate the valid cases?
It is just a range check and three escaped characters check. The range check
can be implemented bit-wise and escaped check may be by a switch case, however
I don't think there would be any performance benefits.
> src/java.base/share/classes/java/lang/constant/ModuleDescImpl.java line 27:
>
>> 25: package java.lang.constant;
>> 26:
>> 27: record ModuleDescImpl(String moduleName) implements ModuleDesc {
>
> Given the validation is elsewhere, it might be useful to add a comment saying
> the moduleName must have been validated.
I'll add it, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1175483446
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1175482952
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1175484294
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v2]
On Mon, 24 Apr 2023 12:28:37 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added links to JVMS and utility methods moved to ConstantUtils
>
> src/java.base/share/classes/java/lang/constant/PackageDesc.java line 34:
>
>> 32: * To create a {@linkplain PackageDesc} for a package, use {@link
>> #of} or
>> 33: * {@link #ofInternalName(String)}.
>> 34: *
>
> Needs a link to jvms 4.4.12. Same for ModuleDesc (4.4.11)
fixed, thanks.
> src/java.base/share/classes/java/lang/constant/PackageDescImpl.java line 37:
>
>> 35: * @throws IllegalArgumentException if the package name is invalid
>> 36: */
>> 37: public static String validateBinaryPackageName(String name) {
>
> All these utility methods should be moved to ConstantUtils.
fixed, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1175262182
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1175265741
Re: RFR: 8306729: Add nominal descriptors of modules and packages to Constants API [v2]
> Constants API already provides models for all loadable constants to help > programs manipulating class files and modelling bytecode instructions. > However no models of module and package constants are provided by Constants > API. Every program manipulating class files must implement own models and > validation of modules and packages constants. > > This pul request adds `java.lang.constant.ModuleDesc` and > `java.lang.constant.PackageDesc` to the Constants API. > > Classfile API will follow up and remove its internal implementations of > `PackageDesc` and `ModuleDesc`. > > Please review this pull request and attached CSR. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: added links to JVMS and utility methods moved to ConstantUtils - Changes: - all: https://git.openjdk.org/jdk/pull/13615/files - new: https://git.openjdk.org/jdk/pull/13615/files/aede94fd..0ca0b56e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13615=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13615=00-01 Stats: 123 lines in 5 files changed: 54 ins; 62 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/13615.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13615/head:pull/13615 PR: https://git.openjdk.org/jdk/pull/13615
RFR: 8306729: Add nominal descriptors of modules and packages to Constants API
Constants API already provides models for all loadable constants to help programs manipulating class files and modelling bytecode instructions. However no models of module and package constants are provided by Constants API. Every program manipulating class files must implement own models and validation of modules and packages constants. This pul request adds `java.lang.constant.ModuleDesc` and `java.lang.constant.PackageDesc` to the Constants API. Classfile API will follow up and remove its internal implementations of `PackageDesc` and `ModuleDesc`. Please review this pull request and attached CSR. Thank you, Adam - Commit messages: - 8306729: Add nominal descriptors of modules and packages to Constants API Changes: https://git.openjdk.org/jdk/pull/13615/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13615=00 Issue: https://bugs.openjdk.org/browse/JDK-8306729 Stats: 261 lines in 7 files changed: 252 ins; 2 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/13615.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13615/head:pull/13615 PR: https://git.openjdk.org/jdk/pull/13615
Re: RFR: 8305990: Stripping debug info of ASM 9.5 fails [v7]
On Thu, 20 Apr 2023 17:21:49 GMT, Adam Sotona wrote: >> Classfile API didn't handle transformations of class files version 50 and >> below correctly. >> >> Proposed fix have two parts: >> 1. Inflation of branch targets does not depend on StackMapTable attribute >> presence for class file version 50 and below. Alternative fallback >> implementation is provided. >> 2. StackMapTable attribute is not generated for class file versions below 50. >> >> StackMapsTest is also extended to test this patch. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > fixed stack counting of JSR instructions I would like to separate this fix from follow-up enhancements, so I created new RFE [JDK-8306650](https://bugs.openjdk.org/browse/JDK-8306650). This bug should be now fixed and the fix also covers Java 5 and older class files with JSR/RET instructions. - PR Comment: https://git.openjdk.org/jdk/pull/13478#issuecomment-1517348244
Re: RFR: 8305990: Stripping debug info of ASM 9.5 fails [v7]
> Classfile API didn't handle transformations of class files version 50 and > below correctly. > > Proposed fix have two parts: > 1. Inflation of branch targets does not depend on StackMapTable attribute > presence for class file version 50 and below. Alternative fallback > implementation is provided. > 2. StackMapTable attribute is not generated for class file versions below 50. > > StackMapsTest is also extended to test this patch. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: fixed stack counting of JSR instructions - Changes: - all: https://git.openjdk.org/jdk/pull/13478/files - new: https://git.openjdk.org/jdk/pull/13478/files/3d74c2fd..c1f3e290 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13478=06 - incr: https://webrevs.openjdk.org/?repo=jdk=13478=05-06 Stats: 3 lines in 2 files changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13478/head:pull/13478 PR: https://git.openjdk.org/jdk/pull/13478
