Re: RFR: 8331264: Reduce java.lang.constant initialization overhead [v3]
On Mon, 29 Apr 2024 12:39:17 GMT, Claes Redestad wrote: >> I'm looking at ways at reducing/eliminating startup overheads the classfile >> API in preparation of #17108, and have pushed a series of enhancements to >> that effect already. This PR is a collection of minor improvements which add >> up to a 1.5% reduction in retired instructions - or a 5% reduction in >> executed bytecode - on a simple lambda startup test. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Simplified void check Marked as reviewed by mchung (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18991#pullrequestreview-2029946427
Re: RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class [v4]
On Thu, 18 Apr 2024 05:54:17 GMT, Adam Sotona wrote: >> Current implementation of `LambdaMetafactory` does not allow to use lambdas >> in hidden classes. Invocation throws `NoClassDefFoundError` instead. >> >> This patch includes lambda implementation in a hidden class under the >> special handling of `useImplMethodHandle`. >> The patch also fixes `j/l/i/defineHiddenClass/BasicTest::testLambda` to >> correctly cover this test case. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > Update > src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java > > Co-authored-by: Mandy Chung HiddenTest is not a hidden class in this test. So it should be fine. The bottom line is to ensure that the type reference is not a hidden class. - PR Comment: https://git.openjdk.org/jdk/pull/18810#issuecomment-2083352956
Re: RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class [v4]
On Thu, 18 Apr 2024 05:54:17 GMT, Adam Sotona wrote: >> Current implementation of `LambdaMetafactory` does not allow to use lambdas >> in hidden classes. Invocation throws `NoClassDefFoundError` instead. >> >> This patch includes lambda implementation in a hidden class under the >> special handling of `useImplMethodHandle`. >> The patch also fixes `j/l/i/defineHiddenClass/BasicTest::testLambda` to >> correctly cover this test case. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > Update > src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java > > Co-authored-by: Mandy Chung `this::toString` references the hidden class by name [1] which fails to be resolved. The code needs to ensure that hidden class's name is not referenced for example by casting to a supertype whose is a normal class: HiddenTest o = this; Supplier s = o::toString; [1] https://download.java.net/java/early_access/jdk23/docs/api/java.base/java/lang/Class.html#hiddenClasses - PR Comment: https://git.openjdk.org/jdk/pull/18810#issuecomment-2083336824
Re: RFR: 8331264: Reduce java.lang.constant initialization overhead [v3]
On Mon, 29 Apr 2024 12:39:17 GMT, Claes Redestad wrote: >> I'm looking at ways at reducing/eliminating startup overheads the classfile >> API in preparation of #17108, and have pushed a series of enhancements to >> that effect already. This PR is a collection of minor improvements which add >> up to a 1.5% reduction in retired instructions - or a 5% reduction in >> executed bytecode - on a simple lambda startup test. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Simplified void check The changes look good to me but I wonder if the non-zero length check before calling `arraycopy` really needed? That seems to add some noise to the code. - PR Review: https://git.openjdk.org/jdk/pull/18991#pullrequestreview-2029162828
Re: RFR: 8331114: Further improve performance of MethodTypeDesc::descriptorString [v5]
On Fri, 26 Apr 2024 22:27:43 GMT, Claes Redestad wrote: >> When analyzing (startup) performance of the Classfile API I found this >> opportunity to further improve `MethodTypeDescImpl::descriptorString`. >> >> Performance improves across the board in existing microbenchmarks: >> >> Name >> (descString) Cnt Base ErrorTest Error Unit Change >> MethodTypeDescFactories.descriptorString >> (Ljava/lang/Object;Ljava/lang/String;)I 6 55,179 ± 2,027 32,920 ± 1,189 >> ns/op 1,68x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()V 6 17,689 ± 1,871 11,060 ± 0,331 ns/op 1,60x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ([IJLjava/lang/String;Z)Ljava/util/List; 6 86,627 ± 1,646 41,035 ± 0,636 >> ns/op 2,11x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()[Ljava/lang/String; 6 18,305 ± 1,974 13,110 ± 0,089 ns/op 1,40x (p = >> 0,000*) >> * = significant >> >> >> The improvement is even more pronounced when running with `-Xint`, which is >> relevant for reducing startup overheads of early ClassFile API use: >> >> Name >> (descString) Cnt Base Error Test Error Unit Change >> MethodTypeDescFactories.descriptorString >> (Ljava/lang/Object;Ljava/lang/String;)I 6 5122,061 ± 81,335 2626,481 ± >> 101,466 ns/op 1,95x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()V 6 3481,316 ± 258,904 1489,267 ± 15,506 ns/op 2,34x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ([IJLjava/lang/String;Z)Ljava/util/List; 6 7741,081 ± 1628,244 3281,778 ± >> 41,892 ns/op 2,36x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()[Ljava/lang/String; 6 3677,803 ± 63,432 1495,291 ± 8,995 ns/op >> 2,46x (p = 0,000*) >> * = significant >> ``` >> >> I also applied similar approach to `MethodTypeDesc::displayDescriptor`: >> while not performance sensitive I think these are so inter-related that it >> makes sense to implement them in a similar fashion. > > Claes Redestad 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 eight additional > commits since the last revision: > > - Rename parameter, formatting > - Rename Wrapper.primitiveClassDescriptor -> classDescriptor > - Merge branch 'master' into methodtypedesc_descriptorString > - Minor fixes > - Use returnType field directly > - Calculate length precisely > - comma-separated > - Optimize MethodTypeDescImpl::toString Thanks for following up the feedback for 8331187. - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18945#pullrequestreview-2025970705
Re: RFR: 8331187: Optimize MethodTypeDesc and ClassDesc.ofDescriptor for primitive types [v3]
On Fri, 26 Apr 2024 18:41:18 GMT, Claes Redestad wrote:
>> src/java.base/share/classes/sun/invoke/util/Wrapper.java line 384:
>>
>>> 382:
>>> 383: /** A nominal descriptor of the primitive type */
>>> 384: public ClassDesc primitiveClassDescriptor() { return
>>> primitiveTypeDesc; }
>>
>> As this method is named as `primitiveClassDescriptor`, I expect this should
>> either throw or return null for `OBJECT` and even `VOID` wrapper. Or
>> should be named "classDescriptor".
>
> While I agree it may seem non-sensical to define `CD_Object` for `Object`,
> there's precedent in that `Wrapper.primitiveType` is `Object.class` - so for
> consistency I wired it up the same way. For the usage patterns introduced
> with this patch we'll never call `primitiveClassDescriptor()` on
> `Wrapper.OBJECT`, though - and `Wrapper.forPrimitiveType` will throw an `IAE`
> if you try - so we can always revisit this.
>
> `void.class` is the primitive type for `Void.class`, allowed as a return type
> and recognized by `Wrapper.forPrimitiveType`. There are checks in for example
> `ConstantUtils.parseMethodDescriptor` which disallows `void.class` anywhere
> but as a return type.
I know it'll never call `primitiveClassDescriptor()` besides primitive types.
While `void.class.isPrimitive()` returns true, `void` is not a primitive type
and called out explicitly in the spec.
My point is about the behavior does not match the method name. It may be
better to rename it to `classDescriptor` instead. It's okay to clean up in
your other performance fix (perhaps #18945).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18971#discussion_r1581507425
Re: RFR: 8331114: Further improve performance of MethodTypeDesc::descriptorString [v4]
On Fri, 26 Apr 2024 08:07:04 GMT, Claes Redestad wrote: >> When analyzing (startup) performance of the Classfile API I found this >> opportunity to further improve `MethodTypeDescImpl::descriptorString`. >> >> Performance improves across the board in existing microbenchmarks: >> >> Name >> (descString) Cnt Base ErrorTest Error Unit Change >> MethodTypeDescFactories.descriptorString >> (Ljava/lang/Object;Ljava/lang/String;)I 6 55,179 ± 2,027 32,920 ± 1,189 >> ns/op 1,68x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()V 6 17,689 ± 1,871 11,060 ± 0,331 ns/op 1,60x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ([IJLjava/lang/String;Z)Ljava/util/List; 6 86,627 ± 1,646 41,035 ± 0,636 >> ns/op 2,11x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()[Ljava/lang/String; 6 18,305 ± 1,974 13,110 ± 0,089 ns/op 1,40x (p = >> 0,000*) >> * = significant >> >> >> The improvement is even more pronounced when running with `-Xint`, which is >> relevant for reducing startup overheads of early ClassFile API use: >> >> Name >> (descString) Cnt Base Error Test Error Unit Change >> MethodTypeDescFactories.descriptorString >> (Ljava/lang/Object;Ljava/lang/String;)I 6 5122,061 ± 81,335 2626,481 ± >> 101,466 ns/op 1,95x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()V 6 3481,316 ± 258,904 1489,267 ± 15,506 ns/op 2,34x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ([IJLjava/lang/String;Z)Ljava/util/List; 6 7741,081 ± 1628,244 3281,778 ± >> 41,892 ns/op 2,36x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()[Ljava/lang/String; 6 3677,803 ± 63,432 1495,291 ± 8,995 ns/op >> 2,46x (p = 0,000*) >> * = significant >> ``` >> >> I also applied similar approach to `MethodTypeDesc::displayDescriptor`: >> while not performance sensitive I think these are so inter-related that it >> makes sense to implement them in a similar fashion. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Use returnType field directly I am glad that this only focuses on the descriptor string as `MethodTypeDesc::displayDescriptor` is typically used for debugging and exception message and not performance-sensitive. - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18945#pullrequestreview-2025627526
Re: RFR: 8331187: Optimize MethodTypeDesc and ClassDesc.ofDescriptor for primitive types [v3]
On Fri, 26 Apr 2024 11:51:51 GMT, Claes Redestad wrote:
>> This PR makes ClassDesc.ofDescriptor return the shared constant for
>> primitive descriptor strings ("I" etc..), and leverages this further by
>> refactoring `MethodTypeDescImpl.ofDescriptor` to avoid the intermediate
>> substring for primitives.
>>
>> Microbenchmarks results look good with expected speedups and allocation
>> reductions any time a primitive is part of the descriptor string, and a
>> non-significant cost otherwise:
>>
>>
>> Name
>> (descString) Cnt Base Error Test Error Unit Change
>> ClassDescFactories.ofDescriptor
>> Ljava/lang/Object; 6 13,941 ± 1,643 14,071 ± 1,333 ns/op
>> 0,99x (p = 0,681 )
>> :gc.alloc.rate.norm
>>16,000 ± 0,000 16,000 ± 0,000 B/op 1,00x (p = 0,617 )
>> ClassDescFactories.ofDescriptor
>> V 69,212 ± 1,045 1,405 ± 0,049 ns/op 6,55x (p = 0,000*)
>> :gc.alloc.rate.norm
>>48,000 ± 0,000 0,000 ± 0,000 B/op 0,00x (p = 0,000*)
>> ClassDescFactories.ofDescriptor
>> I 69,009 ± 0,035 1,431 ± 0,192 ns/op 6,30x (p = 0,000*)
>> :gc.alloc.rate.norm
>>48,000 ± 0,000 0,000 ± 0,000 B/op 0,00x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> (Ljava/lang/Object;Ljava/lang/String;)I 6 182,050 ± 4,333141,644 ±
>> 2,685 ns/op 1,29x (p = 0,000*)
>> :gc.alloc.rate.norm
>> 360,001 ± 0,000264,001 ± 0,000 B/op 0,73x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ()V 6 17,169 ± 2,008 9,915 ± 0,018 ns/op 1,73x (p = 0,000*)
>> :gc.alloc.rate.norm
>> 120,000 ± 0,000104,000 ± 0,000 B/op 0,87x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ([IJLjava/lang/String;Z)Ljava/util/List; 6 270,372 ± 3,624217,050 ±
>> 3,170 ns/op 1,25x (p = 0,000*)
>> :gc.alloc.rate.norm
>> 520,002 ± 0,000328,001 ± 0,000 B/op 0,63x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ()[Ljava/lang/String; 6 35,036 ± 0,351 36...
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Copyrights and other comments from @JornVernee
src/java.base/share/classes/sun/invoke/util/Wrapper.java line 384:
> 382:
> 383: /** A nominal descriptor of the primitive type */
> 384: public ClassDesc primitiveClassDescriptor() { return
> primitiveTypeDesc; }
As this method is named as `primitiveClassDescriptor`, I expect this should
either throw or return null for `OBJECT` and even `VOID` wrapper. Or should
be named "classDescriptor".
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18971#discussion_r1581352160
Re: RFR: 8331134: Port SimpleStringBuilderStrategy to use ClassFile API [v10]
On Fri, 26 Apr 2024 14:57:07 GMT, Claes Redestad wrote: >> This patch suggests a workaround to an issue with huge SCF MH expression >> trees taking excessive JIT compilation resources by reviving (part of) the >> simple bytecode-generating strategy that was originally available as an >> all-or-nothing strategy choice. >> >> Instead of reintroducing a binary strategy choice I propose a threshold >> parameter, controlled by >> `-Djava.lang.invoke.StringConcat.highArityThreshold=`: For expressions >> below or at this threshold there's no change, for expressions with an arity >> above it we use the `StringBuilder`-chain bytecode generator. >> >> There are a few trade-offs at play here which influence the choice of >> threshold. The simple high arity strategy will for example not see any reuse >> of LambdaForms but strictly always generate a class per indy callsite, which >> means we might end up with a higher total number of classes generated and >> loaded in applications if we set this value too low. It may also produce >> worse performance on average. On the other hand there is the observed >> increase in C2 resource usage as expressions grow unwieldy. On the other >> other hand high arity expressions are likely rare to begin with, with less >> opportunities for sharing than the more common low-arity expressions. >> >> I turned the submitted test case into a few JMH benchmarks and did some >> experiments with `-XX:CompileCommand=MemStat,StringConcat::concat,print`: >> >> Baseline strategy: >> 13 args: 6.3M >> 23 args: 18M >> 123 args: 868M >> >> `-Djava.lang.invoke.StringConcat.highArityThreshold=0`: >> 13 args: 2.11M >> 23 args: 3.67M >> 123 args: 4.75M >> >> For 123 args the memory overhead of the baseline strategy is 180x, but for >> 23 args we're down to a 5x memory overhead, and down to a 3x overhead for 13 >> args. Since the absolute overhead for 23 is borderline acceptable (+15Mb) >> I've conservatively chosen a threshold at arity 20. This keeps C2 resource >> pressure at a reasonable level (< 18M) while avoiding perturbing performance >> at the vast majority of call sites. >> >> I was asked to use the new class file API for mainline. There's a version of >> this patch implemented using ASM in 7c52a9f which might be a reasonable >> basis for a backport. > > Claes Redestad has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 12 commits: > > - Rebase after integration of #18953 > - Nits, clean up constant, introduce missing constant MethodTypeDesc for > toString > - Make Set.of(STRONG) a constant, fix compilation, minor code improvements > - Update > src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java > >Co-authored-by: Mandy Chung > - Minor SimpleStringBuilderStrategy:: overhead reduction > - Merge master, resolve conflicts due pr/18688 > - Dump using the hidden class name > - Pre-size stringbuilders based on constant size and a small argument factor > - @liach feedback > - Bump threshold after experiments > - ... and 2 more: https://git.openjdk.org/jdk/compare/5e2ced4b...f87fa1d7 LGTM - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18690#pullrequestreview-2025374446
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v2]
On Thu, 25 Apr 2024 19:53:46 GMT, Claes Redestad wrote:
>> Splitting out the ASM-based version from #18690 to push that first under the
>> JBS (to help backporting). Keeping #18690 open to rebase and follow-up on
>> this as a subtask. See discussion in that #18690 for more details,
>> discussion and motivation for this.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove accidental use of java.lang.classfile
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1057:
> 1055: */
> 1056: private static final class SimpleStringBuilderStrategy {
> 1057: static final int CLASSFILE_VERSION = 52; // JDK 8
Alternatively, this can use `ClassFileFormatVersion.latest().major()` which
exists since JDK 20.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18953#discussion_r1580432036
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v9]
On Wed, 24 Apr 2024 19:13:43 GMT, Claes Redestad wrote: >> This patch suggests a workaround to an issue with huge SCF MH expression >> trees taking excessive JIT compilation resources by reviving (part of) the >> simple bytecode-generating strategy that was originally available as an >> all-or-nothing strategy choice. >> >> Instead of reintroducing a binary strategy choice I propose a threshold >> parameter, controlled by >> `-Djava.lang.invoke.StringConcat.highArityThreshold=`: For expressions >> below or at this threshold there's no change, for expressions with an arity >> above it we use the `StringBuilder`-chain bytecode generator. >> >> There are a few trade-offs at play here which influence the choice of >> threshold. The simple high arity strategy will for example not see any reuse >> of LambdaForms but strictly always generate a class per indy callsite, which >> means we might end up with a higher total number of classes generated and >> loaded in applications if we set this value too low. It may also produce >> worse performance on average. On the other hand there is the observed >> increase in C2 resource usage as expressions grow unwieldy. On the other >> other hand high arity expressions are likely rare to begin with, with less >> opportunities for sharing than the more common low-arity expressions. >> >> I turned the submitted test case into a few JMH benchmarks and did some >> experiments with `-XX:CompileCommand=MemStat,StringConcat::concat,print`: >> >> Baseline strategy: >> 13 args: 6.3M >> 23 args: 18M >> 123 args: 868M >> >> `-Djava.lang.invoke.StringConcat.highArityThreshold=0`: >> 13 args: 2.11M >> 23 args: 3.67M >> 123 args: 4.75M >> >> For 123 args the memory overhead of the baseline strategy is 180x, but for >> 23 args we're down to a 5x memory overhead, and down to a 3x overhead for 13 >> args. Since the absolute overhead for 23 is borderline acceptable (+15Mb) >> I've conservatively chosen a threshold at arity 20. This keeps C2 resource >> pressure at a reasonable level (< 18M) while avoiding perturbing performance >> at the vast majority of call sites. >> >> I was asked to use the new class file API for mainline. There's a version of >> this patch implemented using ASM in 7c52a9f which might be a reasonable >> basis for a backport. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Nits, clean up constant, introduce missing constant MethodTypeDesc for > toString It's a little confusing for JDK-8327247 to have 2 PRs but it's okay. We can add a note in JDK-8327247 to clarify. I assume you plan to use PR to convert to use ClassFile API after you pull from JDK-8327247 once integrated? - PR Comment: https://git.openjdk.org/jdk/pull/18690#issuecomment-2078512445
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v2]
On Thu, 25 Apr 2024 19:53:46 GMT, Claes Redestad wrote: >> Splitting out the ASM-based version from #18690 to push that first under the >> JBS (to help backporting). Keeping #18690 open to rebase and follow-up on >> this as a subtask. See discussion in that #18690 for more details, >> discussion and motivation for this. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove accidental use of java.lang.classfile Nit: the copyright header needs update before integration. - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18953#pullrequestreview-2023964506
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v5]
On Wed, 24 Apr 2024 14:49:55 GMT, Sonia Zaldana Calles wrote: >> Hi folks, >> >> This PR aims to fix >> [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581). >> >> I think the regression got introduced in >> [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458). >> >> In the issue linked above, >> [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226) >> got removed to simplify launcher code. >> >> Previously, we used ```getMainType``` to do the appropriate main method >> invocation in ```JavaMain```. However, we currently attempt to do all types >> of main method invocations at the same time >> [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623). >> >> >> Note how all of these invocations clear the exception reported with >> [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390). >> >> >> Therefore, if a legitimate exception comes up during one of these >> invocations, it does not get reported. >> >> I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking >> forward to your suggestions. >> >> Cheers, >> Sonia > > Sonia Zaldana Calles has updated the pull request incrementally with one > additional commit since the last revision: > > Using new macro to avoid reporting JNI error Looks good. Can you add a new test for this? You can reference MainClassCantBeLoadedTest.java which does something similar to what you need. src/java.base/share/native/libjli/java.c line 621: > 619: helperClass = GetLauncherHelperClass(env); > 620: isStaticMainField = > 621: (*env)->GetStaticFieldID(env, helperClass, "isStaticMain", "Z"); Nit: this line isn't long. Can do in 1 line. Same for line 623-624, 626-627. - PR Review: https://git.openjdk.org/jdk/pull/18786#pullrequestreview-2020321074 PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1578158252
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v8]
On Wed, 24 Apr 2024 10:08:42 GMT, Claes Redestad wrote:
>> This patch suggests a workaround to an issue with huge SCF MH expression
>> trees taking excessive JIT compilation resources by reviving (part of) the
>> simple bytecode-generating strategy that was originally available as an
>> all-or-nothing strategy choice.
>>
>> Instead of reintroducing a binary strategy choice I propose a threshold
>> parameter, controlled by
>> `-Djava.lang.invoke.StringConcat.highArityThreshold=`: For expressions
>> below or at this threshold there's no change, for expressions with an arity
>> above it we use the `StringBuilder`-chain bytecode generator.
>>
>> There are a few trade-offs at play here which influence the choice of
>> threshold. The simple high arity strategy will for example not see any reuse
>> of LambdaForms but strictly always generate a class per indy callsite, which
>> means we might end up with a higher total number of classes generated and
>> loaded in applications if we set this value too low. It may also produce
>> worse performance on average. On the other hand there is the observed
>> increase in C2 resource usage as expressions grow unwieldy. On the other
>> other hand high arity expressions are likely rare to begin with, with less
>> opportunities for sharing than the more common low-arity expressions.
>>
>> I turned the submitted test case into a few JMH benchmarks and did some
>> experiments with `-XX:CompileCommand=MemStat,StringConcat::concat,print`:
>>
>> Baseline strategy:
>> 13 args: 6.3M
>> 23 args: 18M
>> 123 args: 868M
>>
>> `-Djava.lang.invoke.StringConcat.highArityThreshold=0`:
>> 13 args: 2.11M
>> 23 args: 3.67M
>> 123 args: 4.75M
>>
>> For 123 args the memory overhead of the baseline strategy is 180x, but for
>> 23 args we're down to a 5x memory overhead, and down to a 3x overhead for 13
>> args. Since the absolute overhead for 23 is borderline acceptable (+15Mb)
>> I've conservatively chosen a threshold at arity 20. This keeps C2 resource
>> pressure at a reasonable level (< 18M) while avoiding perturbing performance
>> at the vast majority of call sites.
>>
>> I was asked to use the new class file API for mainline. There's a version of
>> this patch implemented using ASM in 7c52a9f which might be a reasonable
>> basis for a backport.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Make Set.of(STRONG) a constant, fix compilation, minor code improvements
Looks fine to me. Indeed, splitting this with ASM and then convert it to
ClassFile API would help the backport.
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1077:
> 1075:
> 1076: /**
> 1077: * Ensure a capacity in the initial StringBuilder to
> accommodate all constants plus this factor times the number
Nit: wrap long line.
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1085:
> 1083:
> 1084: static {
> 1085: DUMPER =
> ClassFileDumper.getInstance("java.lang.invoke.StringConcatFactory.dump",
> "stringConcatClasses");
Nit: this static block isn't strictly needed. Can assign at the declaration of
the static variable.
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1112:
> 1110: return hiddenLookup.findStatic(innerClass, METHOD_NAME,
> args);
> : } catch (Exception e) {
> 1112: DUMPER.dumpFailedClass(className, classBytes);
This line is no longer needed. The bytes will be dumped if it's enabled for
both success and failing case.
-
Marked as reviewed by mchung (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18690#pullrequestreview-2020345792
PR Review Comment: https://git.openjdk.org/jdk/pull/18690#discussion_r1578178759
PR Review Comment: https://git.openjdk.org/jdk/pull/18690#discussion_r1578176295
PR Review Comment: https://git.openjdk.org/jdk/pull/18690#discussion_r1578173742
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v6]
On Thu, 18 Apr 2024 14:50:30 GMT, Claes Redestad wrote: >> This patch suggests a workaround to an issue with huge SCF MH expression >> trees taking excessive JIT compilation resources by reviving (part of) the >> simple bytecode-generating strategy that was originally available as an >> all-or-nothing strategy choice. >> >> Instead of reintroducing a binary strategy choice I propose a threshold >> parameter, controlled by >> `-Djava.lang.invoke.StringConcat.highArityThreshold=`: For expressions >> below or at this threshold there's no change, for expressions with an arity >> above it we use the `StringBuilder`-chain bytecode generator. >> >> There are a few trade-offs at play here which influence the choice of >> threshold. The simple high arity strategy will for example not see any reuse >> of LambdaForms but strictly always generate a class per indy callsite, which >> means we might end up with a higher total number of classes generated and >> loaded in applications if we set this value too low. It may also produce >> worse performance on average. On the other hand there is the observed >> increase in C2 resource usage as expressions grow unwieldy. On the other >> other hand high arity expressions are likely rare to begin with, with less >> opportunities for sharing than the more common low-arity expressions. >> >> I turned the submitted test case into a few JMH benchmarks and did some >> experiments with `-XX:CompileCommand=MemStat,StringConcat::concat,print`: >> >> Baseline strategy: >> 13 args: 6.3M >> 23 args: 18M >> 123 args: 868M >> >> `-Djava.lang.invoke.StringConcat.highArityThreshold=0`: >> 13 args: 2.11M >> 23 args: 3.67M >> 123 args: 4.75M >> >> For 123 args the memory overhead of the baseline strategy is 180x, but for >> 23 args we're down to a 5x memory overhead, and down to a 3x overhead for 13 >> args. Since the absolute overhead for 23 is borderline acceptable (+15Mb) >> I've conservatively chosen a threshold at arity 20. This keeps C2 resource >> pressure at a reasonable level (< 18M) while avoiding perturbing performance >> at the vast majority of call sites. >> >> I was asked to use the new class file API for mainline. There's a version of >> this patch implemented using ASM in 7c52a9f which might be a reasonable >> basis for a backport. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Minor SimpleStringBuilderStrategy:: overhead reduction src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1108: > 1106: DUMPER.dumpClass(innerClass.getName(), innerClass, > classBytes); > 1107: MethodHandle mh = hiddenLookup.findStatic(innerClass, > METHOD_NAME, args); > 1108: return mh; An alternative way to define a hidden class that will detect if the dumper is enabled and dump the classfile. Suggestion: Lookup hiddenLookup = lookup.makeHiddenClassDefiner(className, classBytes, Set.of(STRONG), DUMPER) .defineClassAsLookup(true); Class innerClass = hiddenLookup.lookupClass(); return hiddenLookup.findStatic(innerClass, METHOD_NAME, args); - PR Review Comment: https://git.openjdk.org/jdk/pull/18690#discussion_r1576880600
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v4]
On Tue, 23 Apr 2024 18:21:42 GMT, Sonia Zaldana Calles
wrote:
>> Hi folks,
>>
>> This PR aims to fix
>> [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581).
>>
>> I think the regression got introduced in
>> [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458).
>>
>> In the issue linked above,
>> [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226)
>> got removed to simplify launcher code.
>>
>> Previously, we used ```getMainType``` to do the appropriate main method
>> invocation in ```JavaMain```. However, we currently attempt to do all types
>> of main method invocations at the same time
>> [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623).
>>
>>
>> Note how all of these invocations clear the exception reported with
>> [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390).
>>
>>
>> Therefore, if a legitimate exception comes up during one of these
>> invocations, it does not get reported.
>>
>> I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking
>> forward to your suggestions.
>>
>> Cheers,
>> Sonia
>
> Sonia Zaldana Calles has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Changes based on feedback
I missed that `NULL_CHECK0` prints that JNI error, sorry. We should avoid
using it then. What do you think having a check macro to return 0 if
exception occurred or the given object is null? No exception cleared as it
will be described in JavaMain.
#define CHECK_EXCEPTION_NULL_FAIL(obj) \
do { \
if ((*env)->ExceptionOccurred(env)) { \
return 0; \
} else if (obj == NULL) { \
return 0; \
} \
} while (JNI_FALSE)
-
PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2073184282
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v6]
On Mon, 22 Apr 2024 08:59:33 GMT, Claes Redestad wrote: > @mlchung @asotona: Alan asked me to use the ClassFile API here. As it's only > used in what's effectively a slow path it shouldn't be blocked by the startup > investigations we're doing there, right? I agree that this should not be blocked by the startup investigation for ClassFile API. It only affects high arity expressions. - PR Comment: https://git.openjdk.org/jdk/pull/18690#issuecomment-2073085437
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v3]
On Fri, 19 Apr 2024 19:39:09 GMT, Sonia Zaldana Calles
wrote:
>> Hi folks,
>>
>> This PR aims to fix
>> [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581).
>>
>> I think the regression got introduced in
>> [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458).
>>
>> In the issue linked above,
>> [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226)
>> got removed to simplify launcher code.
>>
>> Previously, we used ```getMainType``` to do the appropriate main method
>> invocation in ```JavaMain```. However, we currently attempt to do all types
>> of main method invocations at the same time
>> [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623).
>>
>>
>> Note how all of these invocations clear the exception reported with
>> [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390).
>>
>>
>> Therefore, if a legitimate exception comes up during one of these
>> invocations, it does not get reported.
>>
>> I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking
>> forward to your suggestions.
>>
>> Cheers,
>> Sonia
>
> Sonia Zaldana Calles has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fixing space formatting
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 908:
> 906:
> 907: private static boolean isStatic = false;
> 908: private static boolean noArgs = false;
Suggestion:
private static boolean isStaticMain = false;
private static boolean noArgMain = false;
src/java.base/share/native/libjli/java.c line 396:
> 394: int
> 395: invokeStaticMainWithArgs(JNIEnv *env, jclass mainClass, jobjectArray
> mainArgs,
> 396: JavaVM *vm, int ret) {
As `CHECK_EXCEPTION_xxx_LEAVE` assumes to be used within JavaMain and it
changes the value of the local `ret` variable, it should call
`CHECK_EXCEPTION_RETURN_VALUE` and `NULL_CHECK_RETURN_VALUE` instead.
JavaMain should call `CHECK_EXCEPTION_LEAVE(1);` after this method returns to
print any exception and exit.
src/java.base/share/native/libjli/java.c line 399:
> 397: jmethodID mainID = (*env)->GetStaticMethodID(env, mainClass, "main",
> 398: "([Ljava/lang/String;)V");
> 399: CHECK_EXCEPTION_LEAVE(1);
Is this needed? `invokeStaticMainWithArgs` should only be called if the main
method is validated as a static main with args.
A sanity check `NULL_CHECK0(mainID)` may be adequate (to use existing macro and
so keeping the return value 0 to indicate error).
src/java.base/share/native/libjli/java.c line 409:
> 407: */
> 408: int
> 409: invokeInstanceMainWithArgs(JNIEnv *env, jclass mainClass, jobjectArray
> mainArgs,
int
invokeInstanceMainWithArgs(JNIEnv *env, jclass mainClass, jobjectArray
mainArgs) {
jmethodID mainID = (*env)->GetMethodID(env, mainClass, "main",
"([Ljava/lang/String;)V");
NULL_CHECK0(mainID);
jmethodID constructor = (*env)->GetMethodID(env, mainClass, "",
"()V");
NULL_CHECK0(constructor);
jobject mainObject = (*env)->NewObject(env, mainClass, constructor);
CHECK_EXCEPTION_RETURN_VALUE(0);
NULL_CHECK0(mainObject);
(*env)->CallVoidMethod(env, mainObject, mainID, mainArgs);
return 1;
}
src/java.base/share/native/libjli/java.c line 431:
> 429: jmethodID mainID = (*env)->GetStaticMethodID(env, mainClass, "main",
> 430:"()V");
> 431: CHECK_EXCEPTION_LEAVE(1);
Same comment as `invokeStaticMainWithArgs`.
Suggestion:
NULL_CHECK0(mainID);
src/java.base/share/native/libjli/java.c line 441:
> 439: */
> 440: int
> 441: invokeInstanceMainWithoutArgs(JNIEnv *env, jclass mainClass,
See the suggestion for `invokeInstanceMainWithArgs`
src/java.base/share/native/libjli/java.c line 610:
> 608:
> 609:
> 610: jclass helperClass = GetLauncherHelperClass(env);
Follow this file convention, declare all local variables at the beginning of
this function.
src/java.base/share/native/libjli/java.c line 614:
> 612: (*env)->GetStaticFieldID(env, helperClass, "isStatic", "Z");
> 613: jboolean isStatic =
> 614: (*env)->GetStaticBooleanField(env, helperClass, isStaticField);
Check exception. Local variable declared at the beginning of the function.
Suggestion:
isStaticMainField = (*env)->GetStaticFieldID(env, helperClass,
"isStaticMain", "Z");
CHECK_EXCEPTION_NULL_LEAVE(isStaticMain);
isStaticMain = (*env)->GetStaticBooleanField(env, helperClass,
isStaticMainField);
src/java.base/share/native/libjli/java.c line 619:
> 617: (*env)->GetStaticFieldID(env, helperClass, "noArgs", "Z");
> 618: jboolean noArgs =
> 619:
Re: RFR: 8330802: Desugar switch in Locale::createLocale [v2]
On Mon, 22 Apr 2024 14:11:41 GMT, Claes Redestad wrote: >> This switch expression in `Locale::createLocale` is causing a somewhat large >> startup regression on my local system. Desugaring to if statements seem like >> the right thing to do while we investigate ways to further reduce overheads >> in `SwitchBootstraps`. >> >> These numbers are against a baseline which include #18865 and #18845, which >> already improved the situation: >> >> >> Name Cnt Base Error Test >> Error Unit Change >> Perfstartup-Noop 2040,500 ± 1,942 31,000 ± >> 2,673ms/op 1,31x (p = 0,000*) >> :.cycles 143254849,000 ± 3398321,355 102205427,650 ± >> 2192784,853 cycles 0,71x (p = 0,000*) >> :.instructions 307138448,850 ± 2095834,550 219415574,800 ± >> 376992,067 instructions 0,71x (p = 0,000*) >> :.taskclock 39,500 ± 1,942 22,500 ± >> 3,858 ms 0,57x (p = 0,000*) >> * = significant >> >> >> Comparing to a baseline without those recent improvements the overhead was >> almost the double: >> >> Name Cnt Base Error Test >> Error Unit Change >> Perfstartup-Noop 2050,000 ± 0,000 31,000 ± >> 2,673ms/op 1,61x (p = 0,000*) >> :.cycles 187047932,000 ± 3330400,381 102205427,650 ± >> 2192784,853 cycles 0,55x (p = 0,000*) >> :.instructions 408219060,350 ± 4031173,140 219415574,800 ± >> 376992,067 instructions 0,54x (p = 0,000*) >> :.taskclock 53,500 ± 4,249 22,500 ± >> 3,858 ms 0,42x (p = 0,000*) >> * = significant > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove InternalError in favor of CCE Looks good. - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18882#pullrequestreview-2015399334
Re: RFR: 8330595: Invoke ObjectMethods::bootstrap method exactly [v3]
On Fri, 19 Apr 2024 07:42:19 GMT, Claes Redestad wrote: >> Investigating a recent regression in mainline I realized we have an >> opportunity to improve the bootstrap overheads of ObjectMethods::bootstrap >> by using `invokeExact` in a way similar to what we already do for LMF and >> SCF BSMs. This avoids generating type checking adapters and equates to a >> one-off startup win of around 5ms in any app that ever has the need to spin >> up a toString, equals or hashCode method on a record. > > Claes Redestad has updated the pull request incrementally with two additional > commits since the last revision: > > - Copyright > - Formatting LGTM - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18845#pullrequestreview-2011967709
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Thu, 18 Apr 2024 20:41:05 GMT, Sonia Zaldana Calles wrote: > Just to clarify, this would still mean converting “isStatic” and “noArgs” > from local variables to fields so I am able to read them on the C side of > things. Did I understand this correctly? I'm okay with adding static boolean fields and read by the native code and the name can be explicit like `isStaticMain` and `mainWithNoArg`. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2066958511
Re: RFR: 8330681: Explicit hashCode and equals for java.lang.runtime.SwitchBootstraps$TypePairs
On Fri, 19 Apr 2024 13:23:53 GMT, Claes Redestad wrote: > We can reduce overhead of first use of a switch bootstrap by moving > `typePairToName` into `TypePairs` and by explicitly overriding `hashCode` and > `equals`. The first change avoids loading and initializing the `TypePairs` > class in actual cases, the second remove some excess code generation from > happening on first use. LGTM - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18865#pullrequestreview-2011881379
Re: RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class [v3]
On Wed, 17 Apr 2024 16:08:31 GMT, Adam Sotona wrote: >> Current implementation of `LambdaMetafactory` does not allow to use lambdas >> in hidden classes. Invocation throws `NoClassDefFoundError` instead. >> >> This patch includes lambda implementation in a hidden class under the >> special handling of `useImplMethodHandle`. >> The patch also fixes `j/l/i/defineHiddenClass/BasicTest::testLambda` to >> correctly cover this test case. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with two additional > commits since the last revision: > > - typo in comment fix > - applied suggested changes Looks fine to me. A hidden class cannot be resolved by name. I tweaked the comment to make it clear. src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java line 182: > 180: // live implementation method handle to the proxy class to invoke > 181: // directly. (javac prefers to avoid this situation by generating > 182: // bridges in the target class) Suggestion: // lambda class has no access to the resolved method, or does // 'invokestatic' on a hidden class which cannot be resolved by name. // Instead, we need to pass the live implementation method handle to // the proxy class to invoke directly. (javac prefers to avoid this // situation by generating bridges in the target class) - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18810#pullrequestreview-2007006920 PR Review Comment: https://git.openjdk.org/jdk/pull/18810#discussion_r1569431448
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v6]
On Wed, 17 Apr 2024 15:21:32 GMT, Adam Sotona wrote: >> java.base java.lang.invoke package heavily uses ASM to generate lambdas and >> method handles. >> >> This patch converts ASM calls to Classfile API. >> >> This PR is continuation of https://github.com/openjdk/jdk/pull/12945 >> >> Any comments and suggestions are welcome. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request 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: > > - applied suggested changes > - Merge branch 'master' into JDK-8294960-invoke > - Reversion of ClassBuilder changes > - Merge branch 'master' into JDK-8294960-invoke > - Apply suggestions from code review > >Co-authored-by: liach <7806504+li...@users.noreply.github.com> > - Apply suggestions from code review > >Co-authored-by: liach <7806504+li...@users.noreply.github.com> > - added missing comment > - fixed InnerClassLambdaMetafactory for hidden caller classes > - performance improvements > - consolidation of descriptors handling > - ... and 2 more: https://git.openjdk.org/jdk/compare/6af94676...304054b9 @cl4es and @kuksenko can you advice what performance benchmarks to run to approve this change? - PR Comment: https://git.openjdk.org/jdk/pull/17108#issuecomment-2061720876
Re: RFR: 8319516: AIX System::loadLibrary needs support to load a shared library from an archive object [v29]
On Wed, 17 Apr 2024 11:20:24 GMT, Suchismith Roy wrote: >> Allow support for both .a and .so files in AIX. >> If .so file is not found, allow fallback to .a extension. >> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516) > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > Update LoadAIXLibraryFromArchiveObject.java > > Nits,coding style and variable name Marked as reviewed by mchung (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17945#pullrequestreview-2006514775
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v5]
On Thu, 4 Apr 2024 09:20:39 GMT, Adam Sotona wrote:
>> java.base java.lang.invoke package heavily uses ASM to generate lambdas and
>> method handles.
>>
>> This patch converts ASM calls to Classfile API.
>>
>> This PR is continuation of https://github.com/openjdk/jdk/pull/12945
>>
>> Any comments and suggestions are welcome.
>>
>> Please review.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request 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:
>
> - Reversion of ClassBuilder changes
> - Merge branch 'master' into JDK-8294960-invoke
> - Apply suggestions from code review
>
>Co-authored-by: liach <7806504+li...@users.noreply.github.com>
> - Apply suggestions from code review
>
>Co-authored-by: liach <7806504+li...@users.noreply.github.com>
> - added missing comment
> - fixed InnerClassLambdaMetafactory for hidden caller classes
> - performance improvements
> - consolidation of descriptors handling
> - InvokerBytecodeGenerator::emit... improvements
> - 8294960: Convert java.base/java.lang.invoke package to use the Classfile
> API to generate lambdas and method handlers
Overall looks good to me. How is the performance result compared to using ASM?
src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java line 32:
> 30: import java.lang.classfile.attribute.SourceFileAttribute;
> 31: import java.lang.constant.ClassDesc;
> 32: import static java.lang.constant.ConstantDescs.*;
Nit: this static imports can be moved to line 50 grouped with other static
imports.
src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java line 53:
> 51: import static java.lang.invoke.MethodHandleStatics.*;
> 52: import static java.lang.invoke.MethodHandles.Lookup.IMPL_LOOKUP;
> 53: import static java.lang.classfile.ClassFile.*;
Nit: nice to sort these in alphabetical order - this before line 50.
src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java line 606:
> 604: TRANSFORM_MODS = List.of(tms.toArray(new Integer[0]));
> 605: }
> 606: private static final MethodTypeDesc MTD_transformHelper =
> MethodTypeDesc.of(CD_MethodHandle, CD_int);
Suggestion:
private static final MethodTypeDesc MTD_TRANFORM_HELPER =
MethodTypeDesc.of(CD_MethodHandle, CD_int);
src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java line
72:
> 70: static final String INVOKERS_HOLDER_CLASS_NAME =
> INVOKERS_HOLDER.replace('/', '.');
> 71: static final String BMH_SPECIES_PREFIX =
> "java.lang.invoke.BoundMethodHandle$Species_";
> 72: private static final ClassFile CC = ClassFile.of();
`ClassFile.of()` returns the ClassFile with default context which is a
singleton object. Is this static variable needed?
src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java line
526:
> 524: clb.withFlags(ACC_PRIVATE | ACC_FINAL | ACC_SUPER)
> 525:
> .withSuperclass(InvokerBytecodeGenerator.INVOKER_SUPER_DESC)
> 526:
> .with(SourceFileAttribute.of(clb.constantPool().utf8Entry(className.substring(className.lastIndexOf('/')
> + 1;
Is it because of performance reason to use `SourceFileAttribute.of(Utf8Entry)`
rather than `SourceFileAttribute.of(String)`? If so, a comment to explain
would help.
src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
line 36:
> 34: import java.lang.classfile.ClassBuilder;
> 35: import java.lang.classfile.ClassFile;
> 36: import static java.lang.classfile.ClassFile.*;
Nit: group static imports in line 52 following this file convention.
src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line
41:
> 39: import java.lang.constant.ClassDesc;
> 40: import java.lang.constant.ConstantDesc;
> 41: import static java.lang.constant.ConstantDescs.*;
Nit: group static imports following this file convention
src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line
57:
> 55: import java.lang.classfile.constantpool.FieldRefEntry;
> 56: import java.lang.classfile.constantpool.InterfaceMethodRefEntry;
> 57: import java.lang.classfile.constantpool.MethodRefEntry;
Nit: group these imports with the above import java.lang.classfile...
src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 28:
> 26: package java.lang.invoke;
> 27:
> 28: import static java.lang.classfile.ClassFile.*;
Nit: move this static import to line 64 following this file convention.
src/java.base/share/classes/java/lang/invoke/TypeConvertingMethodAdapter.java
line 75:
> 73: UNBOX_DOUBLE = unbox(CD_Number,
> "doubleValue", CD_double);
> 74:
> 75: static private TypeKind primitiveTypeKindFromClass(Class type) {
Suggestion:
private static
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Wed, 3 Apr 2024 22:31:39 GMT, Mandy Chung wrote: >> Severin Gehwolf has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Move CreateLinkableRuntimePlugin to build folder >> >> Keep runtime link supporting classes in package >> jdk.tools.jlink.internal.runtimelink > > Thanks for the investigation w.r.t. extending jimage. It does not seem worth > the effort in pursuing the support of adding resources to an existing jimage > file. To me, putting the diff data under `lib` directory as a private file > is a simpler and acceptable solution. It allows the second jlink invocation > to avoid the plugin pipeline if the per-module metadata is generated in > normal jlink invocation. > > (An observation: The current implementation requires the diffs to be > generated as a plugin. For this approach, it may be possible to implement > with a separate main class that calls JLink API and generates the diffs.) > What would you (and @mlchung) think of having jlink generate > lib/runtime-image-link.delta as a step between the pipeline and image > generation? I have similar thought before I went on vacation last week and also think that this would allow this be further simplified to one single jlink invocation to produce a linkable image. In `ImageFileCreator::generateJImage`, after the plugin pipeline, it already adds a step to record the per-module meta information as additional resources. The delta file can be generated after that comparing the original resource pool with the transformed resource pool. I believe `allContent` has the original content. If it is a single jlink step to produce a linkable image (generate the delta file after the plugin pipeline before generating the image), we might be able to get rid of `--ignore-modified-runtime` by storing a copy of conf files in the delta file and the original conf files will be used when the linkable image is used to create a new runtime image. [An observation from the above, it might be possible to generate the delta file and insert in the jimage :-) This might be a bonus. ] - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2059631414
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Mon, 15 Apr 2024 18:25:02 GMT, Sonia Zaldana Calles wrote: > Hi folks, > > This PR aims to fix > [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581). > > I think the regression got introduced in > [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458). > > In the issue linked above, > [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226) > got removed to simplify launcher code. > > Previously, we used ```getMainType``` to do the appropriate main method > invocation in ```JavaMain```. However, we currently attempt to do all types > of main method invocations at the same time > [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623). > > > Note how all of these invocations clear the exception reported with > [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390). > > > Therefore, if a legitimate exception comes up during one of these > invocations, it does not get reported. > > I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking > forward to your suggestions. > > Cheers, > Sonia src/java.base/share/classes/sun/launcher/LauncherHelper.java line 954: > 952: > 953: int mods = mainMethod.getModifiers(); > 954: boolean isStatic = Modifier.isStatic(mods); Can get the value for `isStatic` and `noArgs` from the main type. src/java.base/share/native/libjli/java.c line 645: > 643: res = invokeInstanceMainWithoutArgs(env, mainClass); > 644: break; > 645: } Only one of invokeXXXMainYYYArgs is called. Looks like `CHECK_EXCEPTION_FAIL` and `CHECK_EXCEPTION_NULL_FAIL` are not needed and can use `CHECK_EXCEPTION_LEAVE` and `CHECK_EXCEPTION_NULL_LEAVE` instead? - PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1566502390 PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1566545886
Re: RFR: 8319516: AIX System::loadLibrary needs support to load a shared library from an archive object [v27]
On Mon, 15 Apr 2024 16:10:38 GMT, Suchismith Roy wrote:
>> Allow support for both .a and .so files in AIX.
>> If .so file is not found, allow fallback to .a extension.
>> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)
>
> Suchismith Roy has updated the pull request incrementally with three
> additional commits since the last revision:
>
> - new line
> - spaces fix
> - spaces fix
Copying the library from JDK and renamed as `.a` is not the best idea. I am
not sure how much work to execute the tool chain on AIX to generate a proper
archive for a shared object.It's okay with me with this approach provided
that the AIX Reviewer approves it.
test/jdk/java/lang/RuntimeTests/loadLibrary/aix/LoadAIXLibraryFromArchiveObject.java
line 43:
> 41: // launches a java application passing this directory through
> "-Djava.library.path".
> 42: // the java application then attempts to load the library using
> System.loadLibrary()
> 43: public static void main(final String[] args) throws Exception {
Suggestion:
public static void main(String[] args) throws Exception {
You can drop `final` in the method signature and local variables below in the
method body.
test/jdk/java/lang/RuntimeTests/loadLibrary/aix/LoadAIXLibraryFromArchiveObject.java
line 53:
> 51: final Path testNativeLibDir = Path.of("native").toAbsolutePath();
> 52: Files.createDirectories(testNativeLibDir);
> 53: final Path libDummyArchive =
> testNativeLibDir.resolve(archiveFileName);
Suggestion:
Path libFooBarArchive = testNativeLibDir.resolve(archiveFileName);
Nit: match the expected library name.
-
PR Review: https://git.openjdk.org/jdk/pull/17945#pullrequestreview-2001874277
PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1566259916
PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1566262563
Re: RFR: 8330196: Make java/lang/invoke/defineHiddenClass/BasicTest release agnostic
On Fri, 12 Apr 2024 23:55:01 GMT, Joe Darcy wrote: > Straightforward test update so it doesn't have to be trivially updated for > each JDK version. LGTM. Thanks for fixing this. - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18769#pullrequestreview-1999383137
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v13]
On Fri, 5 Apr 2024 18:14:36 GMT, Suchismith Roy wrote: >> Allow support for both .a and .so files in AIX. >> If .so file is not found, allow fallback to .a extension. >> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516) > > Suchismith Roy has updated the pull request incrementally with three > additional commits since the last revision: > > - update tests > - update tests > - update tests Thanks for the update. Looks good. I updated the synopsis of the JBS issue for better description. Please update the title to match it. It's good to get another reviewer who can validate this on AIX to sponsor this change. - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17945#pullrequestreview-1984018341
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v12]
On Fri, 5 Apr 2024 17:44:22 GMT, Suchismith Roy wrote: >> Allow support for both .a and .so files in AIX. >> If .so file is not found, allow fallback to .a extension. >> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516) > > Suchismith Roy has updated the pull request incrementally with two additional > commits since the last revision: > > - update tests > - update tests If it turns out that "libperfstat.so" exists on some system, we can update the test to allow that scenario. It's good to confirm before we allow that. - PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2040326628
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v12]
On Fri, 5 Apr 2024 17:41:36 GMT, Suchismith Roy wrote:
>> Allow support for both .a and .so files in AIX.
>> If .so file is not found, allow fallback to .a extension.
>> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)
>
> Suchismith Roy has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - update tests
> - update tests
test/jdk/java/lang/RuntimeTests/loadLibrary/aix/LoadAIXLibraryFromArchiveObject.java
line 37:
> 35: File perfstatSharedObjectPath = new
> File("/usr/lib/libperfstat.so");
> 36: // If .a file is not present and .so file is present.
> 37: if (!perfstatPath.exists() || !perfstatSharedObjectPath.exists())
This test does not compile. Please run it before and after the fix to validate
the fix.
Suggestion:
if (!perfstatArchivePath.exists() || !perfstatSharedObjectPath.exists())
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1554046285
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v11]
On Fri, 5 Apr 2024 08:58:34 GMT, Suchismith Roy wrote: >> Allow support for both .a and .so files in AIX. >> If .so file is not found, allow fallback to .a extension. >> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516) > > Suchismith Roy has updated the pull request incrementally with two additional > commits since the last revision: > > - spaces > - nits and move file to aix directory Yes while it's just sanity test and documents what this test expects and validates this specific code path. Ideally the test should generate its own ".so" and ".a" to validate the code paths so that it does not depend on a system library. - PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2040313136
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v11]
On Fri, 5 Apr 2024 08:58:34 GMT, Suchismith Roy wrote:
>> Allow support for both .a and .so files in AIX.
>> If .so file is not found, allow fallback to .a extension.
>> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)
>
> Suchismith Roy has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - spaces
> - nits and move file to aix directory
test/jdk/java/lang/RuntimeTests/loadLibrary/aix/LoadAIXLibraryFromArchiveObject.java
line 28:
> 26: * @bug 8319516
> 27: * @requires os.family == "aix"
> 28: * @run main/othervm -Djava.library.path=/usr/lib LoadLibraryTestAIX
Suggestion:
* @run main/othervm -Djava.library.path=/usr/lib
LoadAIXLibraryFromArchiveObject
test/jdk/java/lang/RuntimeTests/loadLibrary/aix/LoadAIXLibraryFromArchiveObject.java
line 33:
> 31: public class LoadAIXLibraryFromArchiveObject {
> 32: public static void main(String[] args) throws Exception {
> 33: String libraryName = "perfstat";
I suggest to check if `/usr/lib/libperstat.a` is present and
`/usr/lib/libperfstat.so` is not present; otherwise, the test should throw
RuntimeException.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1553959163
PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1553962707
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v10]
On Fri, 5 Apr 2024 08:08:27 GMT, Suchismith Roy wrote: >> The test should verify if `/usr/lib/libperstat.a` is present and also >> `/usr/lib/libperfstat.so` is not present. >> >> Do you expect all AIX machines do not have `/usr/lib/libperfstat.so`? > > Yes it is expected. the hotspot code has harcoded it to .a file. In this case, the test can verify if `/usr/lib/libperstat.a` is present and `/usr/lib/libperfstat.so` is not present; otherwise, the test should throw `RuntimeException`. This also serves a good documentation purpose showing what this test expects. - PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1553961921
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v10]
On Thu, 4 Apr 2024 17:35:43 GMT, Mandy Chung wrote:
>> Suchismith Roy has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add jtreg test case
>
> test/jdk/java/lang/RuntimeTests/loadLibrary/LoadLibraryTestAIX.java line 38:
>
>> 36: } catch (Exception e) {
>> 37: throw new RuntimeException("LoadLibraryTestAIX : could not
>> load libperfstat.a"+e);
>> 38: }
>
> Nit: the catch clause is not strictly needed. `UnsatisfiedLinkError` is an
> unchecked exception. The test should fail if you run it with JDK without
> this fix.
The test should verify if `/usr/lib/libperstat.a` is present and also
`/usr/lib/libperfstat.so` is not present.
Do you expect all AIX machines do not have `/usr/lib/libperfstat.so`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1552140344
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v10]
On Thu, 4 Apr 2024 11:13:43 GMT, Suchismith Roy wrote:
>> Allow support for both .a and .so files in AIX.
>> If .so file is not found, allow fallback to .a extension.
>> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)
>
> Suchismith Roy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add jtreg test case
src/java.base/aix/classes/jdk/internal/loader/ClassLoaderHelper.java line 47:
> 45: * If loading of the given library name with ".so" suffix fails, it
> will attempt to load the library of
> 46: * the same name with ".a" suffix as the alternate name.
> 47: * This method simply returns null. It could implement the alternate
> name converting ".so" with ".a" suffix but redundant.
Formatting nit: please wrap the comment with approx same width (~80 column).
Same for line 37-39.
test/jdk/java/lang/RuntimeTests/loadLibrary/LoadLibraryTestAIX.java line 28:
> 26: * @bug 8319516
> 27: * @requires os.family == "aix"
> 28: * @run main/native/othervm -Djava.library.path=/usr/lib
> LoadLibraryTestAIX
Suggestion:
* @run main/othervm -Djava.library.path=/usr/lib LoadLibraryTestAIX
This test does not build native library. I think it does not need "main/native"
test/jdk/java/lang/RuntimeTests/loadLibrary/LoadLibraryTestAIX.java line 31:
> 29: */
> 30:
> 31: public class LoadLibraryTestAIX {
what about `LoadAIXLibraryFromArchiveObject`?
test/jdk/java/lang/RuntimeTests/loadLibrary/LoadLibraryTestAIX.java line 33:
> 31: public class LoadLibraryTestAIX {
> 32: public static void main(String[] args) throws Exception {
> 33: String libraryName="perfstat";
Suggestion:
String libraryName = "perfstat";
test/jdk/java/lang/RuntimeTests/loadLibrary/LoadLibraryTestAIX.java line 38:
> 36: } catch (Exception e) {
> 37: throw new RuntimeException("LoadLibraryTestAIX : could not
> load libperfstat.a"+e);
> 38: }
Nit: the catch clause is not strictly needed. `UnsatisfiedLinkError` is an
unchecked exception. The test should fail if you run it with JDK without this
fix.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1552158568
PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1552146115
PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1552145197
PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1552135081
PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1552137303
Re: RFR: 8328366: Thread.setContextClassloader from thread in FJP commonPool task no longer works after JDK-8327501
On Tue, 19 Mar 2024 12:16:29 GMT, Viktor Klang wrote: > This is a draft PR with a potential solution to 8328366 without regressing > 8327501. > > @DougLea To avoid regressions in the future, where would regression tests for > this ideally be located? Marked as reviewed by mchung (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18374#pullrequestreview-1980469001
Re: RFR: 8328366: Thread.setContextClassloader from thread in FJP commonPool task no longer works after JDK-8327501
On Thu, 4 Apr 2024 14:46:11 GMT, Viktor Klang wrote:
>> src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 1143:
>>
>>> 1141: @SuppressWarnings("removal")
>>> 1142: SecurityManager sm = System.getSecurityManager();
>>> 1143: if (sm != null && isCommon)
>>
>> For common thread pool, if SM is not enabled, it will create
>> `ForkJoinWorkerThread` that does not clear thread locals which is different
>> than JDK 18 behavior. Is this intentional?
>
> @mlchung I think we should proceed with merging this as-is and look to
> address the ThreadLocal situation (or not at all, based on @DougLea's input).
> Are you OK with reviewing this as-is?
Yes it's fine to separate it as a follow up.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18374#discussion_r1552027346
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v10]
On Thu, 4 Apr 2024 11:13:43 GMT, Suchismith Roy wrote: >> Allow support for both .a and .so files in AIX. >> If .so file is not found, allow fallback to .a extension. >> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516) > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > Add jtreg test case Under `test/jdk/java/lang/RuntimeTests/loadLibrary` directory or a subdirectory under it would work - PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2037641298
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Tue, 19 Mar 2024 16:55:14 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Move CreateLinkableRuntimePlugin to build folder > > Keep runtime link supporting classes in package > jdk.tools.jlink.internal.runtimelink Thanks for the investigation w.r.t. extending jimage. It does not seem worth the effort in pursuing the support of adding resources to an existing jimage file. To me, putting the diff data under `lib` directory as a private file is a simpler and acceptable solution. It allows the second jlink invocation to avoid the plugin pipeline if the per-module metadata is generated in normal jlink invocation. (An observation: The current implementation requires the diffs to be generated as a plugin. For this approach, it may be possible to implement with a separate main class that calls JLink API and generates the diffs.) - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2035719255
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v9]
On Wed, 3 Apr 2024 12:32:21 GMT, Suchismith Roy wrote: >> Allow support for both .a and .so files in AIX. >> If .so file is not found, allow fallback to .a extension. >> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516) > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > coding style libsysteminfo.a is another option. - PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2035024141
Integrated: 8326979: (jdeps) improve the error message for FindException caused by InvalidModuleDescriptorException
On Thu, 14 Mar 2024 17:35:22 GMT, Mandy Chung wrote: > Trivial fix. Improve the error message to print the cause of the module > resolution failure if present. This pull request has now been integrated. Changeset: 044f4ed5 Author: Mandy Chung URL: https://git.openjdk.org/jdk/commit/044f4ed55dfce7f1aed9e10accf459b4af9b975e Stats: 89 lines in 2 files changed: 88 ins; 0 del; 1 mod 8326979: (jdeps) improve the error message for FindException caused by InvalidModuleDescriptorException Reviewed-by: jpai, alanb - PR: https://git.openjdk.org/jdk/pull/18308
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v8]
On Mon, 1 Apr 2024 17:07:47 GMT, Suchismith Roy wrote: >> Allow support for both .a and .so files in AIX. >> If .so file is not found, allow fallback to .a extension. >> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516) > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > spaces Without the alternate name, "liblibname.a.so" would work but not for the case with alternate names though. It would be confusing printing "liblibname.a.so" or "liblibname.a.a" either way.Since it's a user error, printing the argument value is better IMO. - PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2030354793
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v8]
On Mon, 1 Apr 2024 17:07:47 GMT, Suchismith Roy wrote:
>> Allow support for both .a and .so files in AIX.
>> If .so file is not found, allow fallback to .a extension.
>> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)
>
> Suchismith Roy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> spaces
The message is correct because `System.loadLibrary("libname.a")` is asking to
load a library name with "libname.a" which will be prepended with "lib" and
appended with ".so" which is the platform-specific behavior.No library
named "liblibname.a.so" exists
-
PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2030335079
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v8]
On Mon, 1 Apr 2024 18:02:26 GMT, Martin Doerr wrote:
> Now, I'm getting "java.lang.UnsatisfiedLinkError: no libname.a in
> java.library.path" when trying `System.loadLibrary("libname.a")` even though
> the file exists in the library path. Is this intended?
To load a library file, you should use `System.load()`
instead.
For `System::loadLibrary` API, it loads the native library specified by the
`libname` argument. The `libname` argument must not contain any platform
specific prefix, file extension or path. See the javadoc [1].
[1]
https://download.java.net/java/early_access/jdk23/docs/api/java.base/java/lang/System.html#loadLibrary(java.lang.String)
-
PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2030310318
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v8]
On Mon, 1 Apr 2024 18:24:53 GMT, Mandy Chung wrote: >> Suchismith Roy has updated the pull request incrementally with one >> additional commit since the last revision: >> >> spaces > > src/java.base/aix/classes/jdk/internal/loader/ClassLoaderHelper.java line 48: > >> 46: * file may be located at the alternate location. >> 47: * For most platforms, this behavior is not supported and returns >> null. >> 48: */ > > Suggestion: > > /** > * AIX implementation of JVM_LoadLibrary handles the alternate path name > mapping. > * If loading of the given library name with ".so" suffix fails, it will > attempt to load the library of > * the same name with ".a" suffix as the alternate name. > * > * This method simply returns null. It could implement the alternate name > converting ".so" with ".a" suffix but redundant. Nit: please add a new line above line 43. - PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1546683634
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v8]
On Mon, 1 Apr 2024 17:07:47 GMT, Suchismith Roy wrote: >> Allow support for both .a and .so files in AIX. >> If .so file is not found, allow fallback to .a extension. >> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516) > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > spaces I adjust the comment which also answer your question. Please add a AIX-only test to verify this behavior. src/java.base/aix/classes/jdk/internal/loader/ClassLoaderHelper.java line 2: > 1: /* > 2: * Copyright (c) 2012, 2024, Oracle and/or its affiliates. All rights > reserved. Suggestion: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. New file. Copyright header start year should be 2024. src/java.base/aix/classes/jdk/internal/loader/ClassLoaderHelper.java line 38: > 36: /** > 37: * Returns true if loading a native library only if > 38: * it's present on the file system. Suggestion: * Shared objects may be loaded from .a archive object on AIX and .so may not exist. * This method returns false so that loading of shared library continues if libname.so is not present. src/java.base/aix/classes/jdk/internal/loader/ClassLoaderHelper.java line 48: > 46: * file may be located at the alternate location. > 47: * For most platforms, this behavior is not supported and returns null. > 48: */ Suggestion: /** * AIX implementation of JVM_LoadLibrary handles the alternate path name mapping. * If loading of the given library name with ".so" suffix fails, it will attempt to load the library of * the same name with ".a" suffix as the alternate name. * * This method simply returns null. It could implement the alternate name converting ".so" with ".a" suffix but redundant. - PR Review: https://git.openjdk.org/jdk/pull/17945#pullrequestreview-1971830272 PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1546680293 PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1546662648 PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1546679104
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v6]
On Wed, 27 Mar 2024 17:23:50 GMT, Suchismith Roy wrote:
>> Allow support for both .a and .so files in AIX.
>> If .so file is not found, allow fallback to .a extension.
>> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)
>
> Suchismith Roy 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:
>
> - remove member check
> - update comment
> - coding style
> - set false
> - restore fil
> - remove member check code
> - trraling spaces
> - trailing spaces
> - remove space
> - remove debug print lines
> - ... and 6 more: https://git.openjdk.org/jdk/compare/4434d52f...4c068e78
`System::loadLibrary("systeminfo")` should call `JVM_LoadLibrary` with
"/usr/lib/libsysteminfo.so" argument (let the .a file exists under "/usr/lib")
which in turn calls `os::dll_load`.JDK-8320005 changed `os::dll_load` to
first load the given filename; if fails, it will call `dll_load_library` with
"/usr/lib/libsysteminfo.a".
You can turn on `-Xlog:library` and it should print log message loading
"/usr/lib/libsysteminfo.so" if you have `loadLibraryOnlyIfPresent` to return
false. If you don't see log message loading "/usr/lib/libsysteminfo.so",
`loadLibraryOnlyIfPresent` still returns true.
Turn on "-Xlog:os" should print "attempting shared library load of
/usr/lib/libsysteminfo.so" and "attempting shared library load of
/usr/lib/libsysteminfo.a" log message.
-
PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2025690507
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v5]
On Wed, 27 Mar 2024 17:40:09 GMT, Suchismith Roy wrote:
>> AFAICT from your fix for
>> [JDK-8320005](https://bugs.openjdk.org/browse/JDK-8320005) commit
>> [e85355ad](https://github.com/openjdk/jdk/commit/e85355ada4ac1061c49ee9f1247d37a437c7b5ab).
>>
>> But it needs verification as I suggest (see
>> https://github.com/openjdk/jdk/pull/17945#issuecomment-2019965401).
>
> @mlchung [JDK-8320005](https://bugs.openjdk.org/browse/JDK-8320005) is
> solving the .so to .a mapping at hotspot level. But if we still call
> loadLibrary() , with a .so, which has an equivalent .a file, it will fail.
>
> What kind of verification are you mentioning ? We do have use cases where we
> have pure .a files. libsystemInfo.a being one of them, as i confirmed from
> our teams.
You can try with a simple program calling `System.loadLibrary("systeminfo")`
with just `loadLibraryOnlyIfPresent` change.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1541570578
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v5]
On Wed, 27 Mar 2024 17:30:13 GMT, Suchismith Roy wrote: >>> Because of the VM support, we can remove `mapAlternativeName` completely. >> >> Does VM provide support for mapping .so to .a files ? We still have cases >> where the entire .a file is shared and don't need to mention the member. > > libsystemInfo.a is one use case ,i got after discussion with our teams, which > actually raised this issue in J9. So there are cases where pure .a files > exist. AFAICT from your fix for [JDK-8320005](https://bugs.openjdk.org/browse/JDK-8320005) commit [e85355ad](https://github.com/openjdk/jdk/commit/e85355ada4ac1061c49ee9f1247d37a437c7b5ab). But it needs verification as I suggest (see https://github.com/openjdk/jdk/pull/17945#issuecomment-2019965401). - PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1541555855
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v5]
On Wed, 27 Mar 2024 17:18:10 GMT, Mandy Chung wrote:
>> I think we both mean that the `if (name.contains("("))` block should get
>> removed.
>
>> We are not supporting that. Are you referring to the comment in the code ?
>> Yeah it should be resconstruction of libname(member_name).so , which is the
>> first filename the classLoader constructs.
>
> Note that `System.mapLibraryName` and `mapAlternativeName` are called for
> `System.loadLibrary` (i.e. prepending `lib` and appending `.so`).
> "libname(member_name)" is not a valid name and no reason for
> `System.loadLibrary` to support it.
> I think we both mean that the if (name.contains("(")) block should get
> removed.
Because of the VM support, we can remove `mapAlternativeName` completely.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1541531589
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v5]
On Wed, 27 Mar 2024 17:06:49 GMT, Martin Doerr wrote:
>>> > So we should keep the mapAlternativeName for atleast .so to .a
>>> > mapping(without any members mentioned).
>>>
>>> "libname.so(member_name)" is not a valid library name. No reason why
>>> `System.load` has to support it.
>>
>> We are not supporting that. Are you referring to the comment in the code ?
>> Yeah it should be resconstruction of libname(member_name).so , which is the
>> first filename the classLoader constructs.
>
> I think we both mean that the `if (name.contains("("))` block should get
> removed.
> We are not supporting that. Are you referring to the comment in the code ?
> Yeah it should be resconstruction of libname(member_name).so , which is the
> first filename the classLoader constructs.
Note that `System.mapLibraryName` and `mapAlternativeName` are called for
`System.loadLibrary` (i.e. prepending `lib` and appending `.so`).
"libname(member_name)" is not a valid name and no reason for
`System.loadLibrary` to support it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1541525247
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v5]
On Wed, 27 Mar 2024 08:47:10 GMT, Martin Doerr wrote:
>> @mlchung The first name constructed by Classloader is always lib.so.
>> So we need a way to map it to lib.a . Else it will search for .so and
>> fail.
>
> Right, the `loadLibraryOnlyIfPresent` change is sufficient for
> `System.load("libname.a(member)")` support. I don't know how common ".a"
> without member specification ist, but that may make sense. It sounds similar
> to what was done for MacOS with
> https://github.com/openjdk/jdk/commit/1e4dfcfbf00a9b17418bccc0dc746fd9a71f3a06.
> So we should keep the mapAlternativeName for atleast .so to .a
> mapping(without any members mentioned).
"libname.so(member_name)" is not a valid library name. No reason why
`System.load` has to support it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1541477181
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v5]
On Mon, 25 Mar 2024 09:46:50 GMT, Suchismith Roy wrote:
>> Allow support for both .a and .so files in AIX.
>> If .so file is not found, allow fallback to .a extension.
>> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)
>
> Suchismith Roy has updated the pull request incrementally with four
> additional commits since the last revision:
>
> - coding style
> - set false
> - restore fil
> - remove member check code
I think just changing `loadLibraryOnlyIfPresent` to return false should be
sufficient to make `System.loadLibrary("foo")` loading from "libfoo.a" and
`System.load("/usr/lib/libfoo.a(libfoo.so)")` to work.
-
PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2021488820
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v5]
On Tue, 26 Mar 2024 20:59:54 GMT, Martin Doerr wrote:
>> Suchismith Roy has updated the pull request incrementally with four
>> additional commits since the last revision:
>>
>> - coding style
>> - set false
>> - restore fil
>> - remove member check code
>
> src/java.base/aix/classes/jdk/internal/loader/ClassLoaderHelper.java line 68:
>
>> 66: int dotIndex = name.lastIndexOf('.');
>> 67: String memberName =
>> name.substring(openBracketIndex,dotIndex);
>> 68: //Reconstruct .so() as
>> .a()
>
> Do we really need to support libname.so(member)? Isn't it always
> libname.a(member)?
I think `mapAlternativeName` isn't needed at all. If
`loadLibraryOnlyIfPresent` returns false, `System.load("libname.a(member)")`
should be passed to dlopen directly. @suchismith1993 can verify it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1540117924
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v5]
On Mon, 25 Mar 2024 18:56:21 GMT, Suchismith Roy wrote: > And it has been in J9 for long time for the exact same reason, which > actually brought out the issue for dcstartup in OpenJDK. dcstartup fails because it fails to load an agent library specified via `-agentlib:am_ibm_16` that was fixed by JDK-8320005. I assume that's what you referred to "J9 had for a long time". This does not use `System::loadLibrary`.It's unclear if any JNI native library is changed to load from an archive object. Any customer reporting this issue? > I just feel keeping load functionality is still applicable, which does not > expect a deterministic mapping (or does it ? ). if loadLibrary expects this > to be deterministic, then i agree, we should drop that. It's not about deterministic mapping or not. `System::load` is intended for loading JNI native libraries but not for providing equivalent functionality to `dlopen`. - PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2018810383
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v5]
On Mon, 25 Mar 2024 09:46:50 GMT, Suchismith Roy wrote:
>> Allow support for both .a and .so files in AIX.
>> If .so file is not found, allow fallback to .a extension.
>> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)
>
> Suchismith Roy has updated the pull request incrementally with four
> additional commits since the last revision:
>
> - coding style
> - set false
> - restore fil
> - remove member check code
JDK-8313616 changes HotSpot VM to support loading member objects from .a on AIX
dlopen. That RFE was a reasonable change as AIX JDK may load native libraries
from an archive file of a named member object e.g.
`/usr/lib/libperfstat.a(shr_64.o)`
JDK-8320005 was the first attempt to fix `dcstartup` and second attempt by this
issue in JDK 23. This changes HotSpot VM to load from an alternate path if
`os::dll_load(lib_path)`, which in turn calls dlopen` on AIX, fails to load
`lib_path` with `.so` suffix, it tries to load with an alternatve path by
replacing `.so` with `.a`.
This issue (JDK-8319516) attempts to change the library implementation as
follows:
1. `System::loadLibrary("foo")` to call `dlopen("/usr/lib/libfoo.so")` first;
if fails, `dlopen("/usr/lib/libfoo.a")`
2. `System::load("/usr/lib/libfoo.so(libfoo.so.1)"` that will call (1)
`dlopen("/usr/lib/libfoo.so((libfoo.so.1)")`, if fails (2)
`dlopen("/usr/lib/libfoo.a((libfoo.so.1)")`.
On AIX, loading shared objects from an archive object via `System::loadLibrary`
and `System::load` never work before.
Do you expect if developers start to package shared objects in the format of
archive objects? If so, it would be reasonable to support #1 for compatibility.
For #2, `System::loadLibrary("foo(libfoo.so.1)"`,
`System::load("/usr/lib/libfoo.so((libfoo.so.1)")` and
`System::load("/usr/lib/libfoo.a((libfoo.so.1)")` are never supported. I
think applications should use `SymbolLookup::libraryLookup` instead on Java 22
and later. I don't use `libclang` as the example here because that's related
to jextract and has nothing to do with `System::load` as Maurizio said.
-
PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2018669567
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v4]
On Mon, 18 Mar 2024 17:43:45 GMT, Suchismith Roy wrote: >> Allow support for both .a and .so files in AIX. >> If .so file is not found, allow fallback to .a extension. >> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516) > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > trraling spaces `os::dll_load` already handles loading from .a on AIX when attempting loading .so (see JDK-8320005). For example, if loading `libclang.so` fails, it attempts to load from `libclang.a`.This issue is essentially revisiting the fix for JDK-8320005 and needs to understand the right mapping. - PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2013811988
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v4]
On Mon, 18 Mar 2024 17:43:45 GMT, Suchismith Roy wrote:
>> Allow support for both .a and .so files in AIX.
>> If .so file is not found, allow fallback to .a extension.
>> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)
>
> Suchismith Roy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> trraling spaces
JDK-8313616 added the support of loading library members on AIX in os::dll_load
in JDK 22. On AIX, is a shared object a member object loaded from
`libname.a(libname.so)`? If so, changing `mapLibraryName` implementation
seems to be a proper fix. If you want to load a specific version, it should
call `loadLibrary("libclang.a(libclang.so.16)")` instead.
As @jaikiran suggests, `loadLibraryOnlyIfPresent()` should return false for AIX
as the file does not exist. The library implementation may need to adjust as
the current implementation uses a file path name.
-
PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2013669909
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Wed, 20 Mar 2024 10:24:23 GMT, Severin Gehwolf wrote: > What we really want is some form of API to extend/patch an existing jimage > preserving everything else. Perhaps I should look into that. Would that be > worth doing? I think avoiding the plugin pipeline in creating a linkable image is simpler and more reliable design. I think it's worth understanding how big effort to support that. The reason why I proposed the alternative putting the diffs data as a side file under `lib` is an interim solution that would allow this work to continue while the API for extending jimage can be done as a follow up. > it seems almost too easy to change if it was a file outside the image. Only a few files in the `lib` directory such as libjvm.so or libjvm.dylib, src.zip and a few other files are intended for external use. All other files and directories under it must be treated are private implementation details and their names, format, and content are subject to change without notice. See JEP 220 for details. > Thirdly, it would go against where the hermetic java work is going (one > effort of this project is to try to include most of the conf/resource files > outside the jimage inside the jimage, next to the classes that use them). I agree that putting the diffs file in the jimage is a good direction. I am also ok with the interim solution. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2010349483
Re: RFR: 8328366: Thread.setContextClassloader from thread in FJP commonPool task no longer works after JDK-8327501
On Tue, 19 Mar 2024 12:16:29 GMT, Viktor Klang wrote:
> This is a draft PR with a potential solution to 8328366 without regressing
> 8327501.
>
> @DougLea To avoid regressions in the future, where would regression tests for
> this ideally be located?
src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 1143:
> 1141: @SuppressWarnings("removal")
> 1142: SecurityManager sm = System.getSecurityManager();
> 1143: if (sm != null && isCommon)
For common thread pool, if SM is not enabled, it will create
`ForkJoinWorkerThread` that does not clear thread locals which is different
than JDK 18 behavior. Is this intentional?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18374#discussion_r1532497712
Re: CFV: New Core Libraries Group Member: Per-Ake Minborg
Vote: yes Mandy On 3/19/24 8:19 AM, Daniel Fuchs wrote: Hi, I hereby nominate Per-Ake Minborg (pminborg) [1] to Membership in the Core Libraries Group [4]. Per-Ake is an OpenJDK Reviewer, a committer in the Leyden and Panama projects, and a member of Oracle’s Java Core Libraries team. Per-Ake has been actively participating in the development of the JDK and Panama projects for several years, and is one of the co-author of the Implementation of Foreign Function and Memory API (Third Preview) [2]. His contributions include more than 80 commits in the JDK [3] Votes are due by 16:00 UTC on April 2, 2024. Only current Members of the Core Libraries Group [4] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list. For Lazy Consensus voting instructions, see [5]. best regards, -- daniel [1] https://openjdk.org/census#pminborg [2] https://github.com/openjdk/jdk/commit/cbccc4c8172797ea2f1b7c301d00add3f517546d [3] https://github.com/search?q=repo%3Aopenjdk%2Fjdk+author%3Aminborg=commits [4] https://openjdk.org/census#core-libs [5] https://openjdk.org/groups/#member-vote
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Tue, 19 Mar 2024 16:55:14 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Move CreateLinkableRuntimePlugin to build folder > > Keep runtime link supporting classes in package > jdk.tools.jlink.internal.runtimelink Thanks for the details.I feel the pain in extending jlink for this work as the current jlink implementation is not easily understandable and has been yearning for rewrite in my perspective (looking forward to Project Leyden's condenser model). Really appreciate your effort for this. IIUC, the main issue is regarding extending an jimage with additional entries in the same compression level. The other option is to add the diffs as a separate file in the JDK image under `lib` instead of part of the jimage. The per-module mapping metadata files can be generated as part of the normal linking as the footprint is small. Would that simplify it? I think the other issues may be solvable. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2007823095
Integrated: 8328261: public lookup fails with IllegalAccessException when used while module system is being initialized
On Mon, 18 Mar 2024 17:40:26 GMT, Mandy Chung wrote: > A simple fix. This is caused by a bug in `VerifyAccess::isClassAccessible` > that checks if a class is exported from `java.base` before the exports are > fully setup.It should check if the module system is fully initialized > before checking the module exports instead. This pull request has now been integrated. Changeset: 13292168 Author: Mandy Chung URL: https://git.openjdk.org/jdk/commit/132921683bc9860ce2ba89729dcd989b10b89aa2 Stats: 9 lines in 1 file changed: 1 ins; 5 del; 3 mod 8328261: public lookup fails with IllegalAccessException when used while module system is being initialized Reviewed-by: rriggs, alanb - PR: https://git.openjdk.org/jdk/pull/18356
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v21]
On Fri, 15 Mar 2024 15:19:17 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with two > additional commits since the last revision: > > - Fix comment. > - Review feedback and Windows build fix (FixPath) This is what I understand from your implementation: 1. create a regular JDK image under `support/images/runtime-link-initial-jdk` using jlink from the exploded JDK build 2. create a linkable JDK image under `images/jdk` using jlink from the exploded JDK build with jlink command-line options used in step 1 plus -create-linkable-runtime jimage=support/images/runtime-link-initial-jdk/lib/modules:module-path=support/images/jmods jlink in step 2 has to run through the plugin pipeline as in step 1 and also the `create-linkable-runtime` plugin to generate the diffs and record the metadata for reconstituting the modules from the images (without the packaged modules). Step 2 re-creates a new jimage file from the input packaged modules while the diffs are generated from `runtime-link-initial-jdk/lib/modules`. In this approach, step 1 and step 2 assume that the same set of modules are linked and set set of plugin transformation is applied except `--create-linkable-runtime` as in step 1, i.e. it relies on the makefile to pass proper commands to step 1 and step 2 to result in the same `lib/modules` minus the new metadata and diffs added by `--create-linkable-runtime`. The original idea we discussed is that step 1 creates a normal JDK image (as step 1 above) and then step 2 solely creates a new JDK linkable image without involving plugin pipeline that would be a simpler model.The idea was something like this: $ runtime-link-initial-jdk/bin/jlink -create-linkable-runtime support/images/jmods --output images/jdk jlink `--create-linkable-runtime` will copy everything under `runtime-link-initial-jdk` (the runtime executing jlink) except `lib/modules` to `images/jdk` and create a new jimage `lib/modules` from `runtime-link-initial-jdk/lib/modules` (include diffs and metadata files). Have you seen any issues with this approach over your current approach which involves the plugin transformation in both step 1 and 2? - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2004888430
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v20]
On Mon, 18 Mar 2024 13:01:10 GMT, Severin Gehwolf wrote: > Yes we do. The main reason being that the `jdk` image has more to it than > just the image. `src.zip`, CDS data, `demo` and so on. We don't want to > duplicate that. To us, including the `jmods` folder is something that comes > into play when actually producing the bundles of the JDK. The linkable > runtime option allows for a more flexible distribution of the resulting JDK. > With or without packaged modules without limiting `jlink` usage (for the > common use-cases). I'm not sure how keeping packaged modules is related to `src.zip`, CDS data, `demo` and so on as they are the targets after `images/jdk` is built. However, I got your point that users should configure what they want the JDK image to produce as a linkable runtime image and packaged modules are orthogonal. Okay with me. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2004605507
RFR: 8328261: public lookup fails with IllegalAccessException when used while module system is being initialized
A simple fix. This is caused by a bug in `VerifyAccess::isClassAccessible` that checks if a class is exported from `java.base` before the exports are fully setup.It should check if the module system is fully initialized before checking the module exports instead. - Commit messages: - 8328261: public lookup fails with IllegalAccessException when used while module system is being initialized Changes: https://git.openjdk.org/jdk/pull/18356/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18356=00 Issue: https://bugs.openjdk.org/browse/JDK-8328261 Stats: 9 lines in 1 file changed: 1 ins; 5 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18356.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18356/head:pull/18356 PR: https://git.openjdk.org/jdk/pull/18356
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v20]
On Fri, 15 Mar 2024 10:53:29 GMT, Severin Gehwolf wrote: > Wasn't the intention to make "creating a linkable runtime image" a build only > decision and make the relevant infrastructure classes build-only artefacts? Build-only decision means that the linkable runtime image is only produced by JDK build. Supporting classes of this tool like the code generating the diffs can reside in `src/jdk.jlink` if possible as that helps keeping in other jlink code. I misinterpreted that this is no longer a build tool approach as I was confused when seeing the source files are moved to `src/jdk.jlink`. The build tool can stay in unnamed module as before but the tool would need to run on the newly built or interim JDK (not the boot JDK) which explains why the code of generating the diffs can be part of jlink. Regarding my comment to "only compile those classes when JDK is configured to build linkable images" was premature. I thought of some complexity last night and if it could be filtered easily, it would be a bonus. If not, also no issue. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2000188399
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v20]
On Fri, 15 Mar 2024 09:55:15 GMT, Severin Gehwolf wrote: > > If `--enable-runtime-link-image` is enabled, the JDK image does not include > > the packaged modules. > > That's not true. Right now `--enable-runtime-link-image` modifies how the > `lib/modules` image looks like (adds a bunch of extra resources). That's it. > It doesn't modify the setup of packaged modules. It is true that they are orthogonal. jlink does allow to produce a linkable image with `--keep-packaged-modules` and the resulting JDK image would work. However, the goal of this work is to produce a JDK image with smaller footprint. This is a question to JDK build to allow configuring building a linkable image with packaged modules. In addition, `--enable-keep-packaged-modules` is enabled by default.Do you want the linkable image includes `jmods` as it's currently implemented in this PR? I assume not. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2000322485
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v20]
On Thu, 14 Mar 2024 14:24:57 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Fix comment in autoconf file > The updated patch uses a **build-only** `jlink` plugin, still called > `--create-linkable-runtime` which is _a)_ added only at build time with > `--add-modules` and _b)_ now generates the diff and prepares the image for > runtime linking as before in one step. The code for this now lives in > `src/jdk.jlink/build/classes`. This patch introduces a new module `jdk.unsupported_jlink_runtime` . `src/jdk.jlink` should only contain one module (see JEP 201). If it ought to have another module, its source files should be placed under `src/$MODULE`. I would suggest to simplify it - drop the new module `jdk.unsupported_jlink_runtime` and simply include all its classes in `jdk.jlink` module; for example in `jdk.tools.jlink.internal.runtimelink` package to follow the existing jlink package name hierarchy. This package can only be compiled when JDK is configured to build linkable images. The build can simply invoke `jlink --create-linkable-runtime` and avoids all the setup to add exports to this build tool. If jlink is running from a linkable image, --create-linkable-runtime` plugin should be disabled and hidden. What do you think? About the configure options, --enable-keep-packaged-modules enable keeping of packaged modules in jdk image [enabled] --enable-runtime-link-image enable producing an image suitable for runtime linking [disabled] If `--enable-runtime-link-image` is enabled, the JDK image does not include the packaged modules. If `--enable-runtime-link-image --enable-keep-packaged-modules` are both enabled, it should probably fail? @erikj79 should it? - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-1998467918
RFR: 8326979: (jdeps) improve the error message for FindException caused by InvalidModuleDescriptorException
Trivial fix. Improve the error message to print the cause of the module resolution failure if present. - Commit messages: - 8326979: (jdeps) improve the error message for FindException caused by InvalidModuleDescriptorException Changes: https://git.openjdk.org/jdk/pull/18308/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18308=00 Issue: https://bugs.openjdk.org/browse/JDK-8326979 Stats: 89 lines in 2 files changed: 88 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18308.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18308/head:pull/18308 PR: https://git.openjdk.org/jdk/pull/18308
Re: RFR: 8327786: Add fluent setAccessible()
On Thu, 25 Jan 2024 21:35:45 GMT, Sergey wrote: > The feature allows to extract a private field value in a single expression, > like so: > > object.getClass().getDeclaredField().setAccessible().get(object) I agree with the evaluation of [JDK-8327786](https://bugs.openjdk.org/browse/JDK-8327786) that `setAccessible(true)` isn't for most developers and this proposal is not worth doing. - PR Comment: https://git.openjdk.org/jdk/pull/17578#issuecomment-1989681305
Re: RFR: 8327729: Remove deprecated xxxObject methods from jdk.internal.misc.Unsafe [v3]
On Sun, 10 Mar 2024 13:47:06 GMT, Eirik Bjørsnøs wrote: >> Please review this PR which removes the 19 deprecated `xxObject*` alias >> methods from `jdk.internal.misc.Unsafe`. >> >> These methods were added in JDK-8213043 (JDK 12), presumably to allow >> `jsr166.jar` to be used across JDK versions. This was a follow-up fix after >> JDK-8207146 had renamed these methods to `xxReference*'. >> >> Since OpenJDK is now the single source of truth for `java.util.concurrent`, >> time has come to remove these deprecated alias methods. >> >> This change was initially discussed here: >> https://mail.openjdk.org/pipermail/core-libs-dev/2024-March/119993.html >> >> Testing: This is a pure deletion of deprecated methods, so the PR includes >> no test changes and the `noreg-cleanup` label is added in the JBS. I have >> verified that all `test/jdk/java/util/concurrent/*` tests pass. >> >> Tagging @DougLea and @Martin-Buchholz to verify that this removal is timely. > > Eirik Bjørsnøs has updated the pull request incrementally with two additional > commits since the last revision: > > - Replace calls to Unsafe.putObject with Unsafe.putReference > - Update a code comment referencing Unsafe.compareAndSetObject to reference > Unsafe.compareAndSetReference instead This should wait for @jaikiran approval confirming the test result is good. - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18176#pullrequestreview-1928982510
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v12]
On Fri, 8 Mar 2024 17:19:41 GMT, Severin Gehwolf wrote: >> I tried out the latest commit (a797ea69). >> >> The output "The default module path, '$java.home/jmods' not present. Use >> --verbose to show the full list of plugin options applied" is bit confusing >> as it looks like jlink failed but it actually succeeded. Blowing away the >> generated image and retrying with --verbose tripped this assert >> >> java.lang.AssertionError: handling of scratch options failed >> at >> jdk.jlink/jdk.tools.jlink.internal.JlinkTask.logPackagedModuleEquivalent(JlinkTask.java:675) >> at >> jdk.jlink/jdk.tools.jlink.internal.JlinkTask.createImageProvider(JlinkTask.java:581) >> at >> jdk.jlink/jdk.tools.jlink.internal.JlinkTask.createImage(JlinkTask.java:430) >> at jdk.jlink/jdk.tools.jlink.internal.JlinkTask.run(JlinkTask.java:302) >> at jdk.jlink/jdk.tools.jlink.internal.Main.run(Main.java:56) >> at jdk.jlink/jdk.tools.jlink.internal.Main.main(Main.java:34) >> Caused by: jdk.tools.jlink.internal.TaskHelper$BadArgs: >> (...my-original-jdk-directory..)/build/linux-x64/images/jdk/jmods already >> exists >> at >> jdk.jlink/jdk.tools.jlink.internal.TaskHelper.newBadArgs(TaskHelper.java:730) >> at >> jdk.jlink/jdk.tools.jlink.internal.JlinkTask.lambda$static$12(JlinkTask.java:183) >> at >> jdk.jlink/jdk.tools.jlink.internal.TaskHelper$Option.process(TaskHelper.java:177) >> at >> jdk.jlink/jdk.tools.jlink.internal.TaskHelper$OptionsHelper.handleOptions(TaskHelper.java:600) >> at >> jdk.jlink/jdk.tools.jlink.internal.JlinkTask.logPackagedModuleEquivalent(JlinkTask.java:672) >> ... 5 more >> >> I haven't dug into this yet but I'm puzzled that the file path to where the >> original build was created is in the exception messages, is that recorded? > >> > @AlanBateman @mlchung I've now pushed an update of this patch which now >> > uses a build-time approach as discussed elsewhere. In order to produce a >> > linkable runtime JDK image, one needs to set --enable-runtime-link-image >> > at configure time. >> >> What is the rationale for introducing a special configure flag for this >> functionality? I've tried to look though all comments in this PR, and read >> the JBS issue and CSR, but I have not found any motivation for this. I'm >> sorry if it's there and I missed it, but this is a huge PR with a lot of >> discussion. > > Sorry, yes this was part of a meeting discussion we had outside this PR. My > understanding is that by default the produced jimage isn't runtime-link > enabled. We (Red Hat) would turn it on in our builds, though. @AlanBateman or > @mlchung might have more details. I think it was a requirement to get this > patch in. At least for the initial contribution. > >> In general, it is better not to introduce variants of the build like this. >> The testing matrix just explodes a bit more. And my understanding from the >> discussion is that this functionality is likely to be turned on anyway, >> otherwise you'll build a crippled jlink without full functionality. > > I would be happy if this could be on by default. For now, I think though we > need to work on the premise that whether or not the resulting JDK image is > suitable for runtime linking (without jmods) is a build-time config decision. > Therefore we need the configure option. @jerboaa thanks for the update. First to recap the revised proposal (based on Alan's notes shared with me earlier): The JDK build is capable of producing a JDK run-time image that does not include packaged modules and the new JDK image is capable to create custom run-time images (call it "linkable" JDK image for now). To reconstitute to the original module content, the new JDK image conceptually needs to contain the "diffs" from the original packaged packaged. This makes it possible for the jlink plugins to run "as if" the resources for each module were coming from the original packaged module. The new image also has the checksums of at least the user-editable configuration files so that edits can be detected. The revised proposal has a few limitations: 1. The "linkable" JDK image can only be created by the JDK build. 2. The "linkable" JDK image is created from the JDK image produced by the JDK build today. It contains the same set of modules, there is no possibility to combine its generation with other jlink options or code transformations. 3. The "linkable" JDK image cannot create a run-time image that contains the "jdk.jlink" module. 4. The "linkable" JDK image only reconstitutes classes/resources to the original module bits. Other plugins such as man pages and dedup legal files are not reconstituted. These limitations are trade-off to make for a much simpler approach. It removes the issues of creating a linkable JDK, or creating another image using a linkable JDK, while at the same time executing a pipeline of plugins that do other transformations. Here
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v18]
On Tue, 27 Feb 2024 15:23:09 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Only show runtime image suffix for JDK modules This PR is about having jlink to produce a run-time image without needing packaged modules (jmod files). The motivation is that jmod files are huge in particular when native debug symbols are included. The previous proposal to support jlink to create a run-time image without jmod files got too complicated and how plugins should revert/apply the transformation and how the users know what transformations are done in the resulting image. The primary motivation for this is to produce OpenJDK run-time image without packaged modules (right now we called it _linkable JDK image_ but need to ponder on the name more) and jlink from such _linkable JDK image_ is capable of creating custom run-time image. Per our offline discussion, the revised proposal is to add an "option" in the JDK build that can produce the linkable JDK image that does not include the packaged modules. The build option can be a configure option or a make target that would need advice from the build team. Such build option would create a run-time image adjacent to `/images/jdk` (for example `/images/linkable-jdk`). No proposal to change the default, i.e. the linkable image is not built by default. This PR is to place the infrastructure and support in place. @magicus is a make target more appropriate for this? - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-1986252706
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Wed, 6 Mar 2024 19:00:19 GMT, Alan Bateman wrote: >> Native access is needed for modules which calls restricted methods [1]. >> AFAICT, java.base, java.desktop and jdk.incubator.vector use >> java.lang.foreign but I don't know if they call restricted methods or not. >> >> https://download.java.net/java/early_access/jdk23/docs/api/java.base/java/lang/foreign/package-summary.html#restricted > >> Native access is needed for modules which calls restricted methods [1]. > > In time, we'll need it for modules using JNI too. We can do this trimming in > another PR to avoid Jan getting pulled into deeply. Doing it in another PR is fine with me. - PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1515009659
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Wed, 6 Mar 2024 15:02:07 GMT, Jan Lahoda wrote: >>> Many of these modules do not need native access in the current >>> implementation. >> >> In addition this will eventually need jlink support. I view the change to >> ModuleBootstrap initialiser to use ModuleLoaderMap.nativeAccessModules() as >> very temporary. It may include many standard/JDK modules that aren't in the >> image. In addition we'll need some way to grant native access at link-time. >> The workaround for the latter right now is to configure default options. > >> Many of these modules do not need native access in the current >> implementation. Should this list be trimmed to list the ones that need >> native access in the current implementation? > > Not sure if I know enough to do the pruning, so I was hoping that could be > done separately (I'd file a bug as Alan suggests). But I can try to prune the > list, if you prefer. Native access is needed for modules which calls restricted methods [1]. AFAICT, java.base, java.desktop and jdk.incubator.vector use java.lang.foreign but I don't know if they call restricted methods or not. https://download.java.net/java/early_access/jdk23/docs/api/java.base/java/lang/foreign/package-summary.html#restricted - PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1514932944
Re: RFD: Can we remove the deprecated internal Unsafe *Object* methods?
These deprecated methods were added to make jsr166.jar to run on different JDK releases. I think it's time to remove these deprecated xxxObject* methods as the renames have been done since JDK 12 for 5 years. I added a comment [1] to confirm with Doug and Martin. Mandy https://bugs.openjdk.org/browse/JDK-8207146?focusedId=14654891=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14654891 On 3/5/24 10:22 AM, Eirik Bjørsnøs wrote: Hi, Now that the JDK repo is the single source of truth for java.util.concurrent [1], I'm wondering if time has come to remove the deprecated *Object* methods introduced in JDK-8213043 [2] These alias methods were introduced because jsr166 used them, but there no longer seems to be any use within the OpenJDK (verified by removing them and running java.util.concurrent tests). Hearing no objections I'll prepare a PR to remove these 19 deprecated, for removal methods. Eirik. [1] https://mail.openjdk.org/pipermail/core-libs-dev/2024-February/119047.html [2] https://bugs.openjdk.org/browse/JDK-8213043
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Tue, 5 Mar 2024 18:43:44 GMT, Alan Bateman wrote: >> Jan Lahoda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Apply suggestions from code review >> >> Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> >> Co-authored-by: Maurizio Cimadamore >> <54672762+mcimadam...@users.noreply.github.com> > > make/conf/module-loader-map.conf line 139: > >> 137: jdk.unsupported \ >> 138: jdk.xml.dom \ >> 139: jdk.zipfs \ > > We should create a JBS issue to prune this. Many of these modules do not need native access in the current implementation. Should this list be trimmed to list the ones that need native access in the current implementation? - PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1513588480
Re: CFV: New Core Libraries Group Member: Raffaello Giulietti
Vote: yes Mandy On 2/13/24 12:25 PM, Brian Burkhalter wrote: I hereby nominate Raffaello Giulietti to Membership in the Core Libraries Group.
Re: RFR: JDK-8322218: Better escaping of single and double quotes in annotation toString() results
On Thu, 1 Feb 2024 01:32:42 GMT, Joe Darcy wrote: > A double quote character doesn't need to be escaped when it is a char literal > and single quote character doesn't need to be escaped when it is in a string. > This change updates the toString() output of annotations to account for the > different escaping requirements of strings and characters. > > Corresponding issue filed for javac's compile-time annotations: JDK-8325078. This looks good to me. - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17664#pullrequestreview-1866468044
Re: RFR: 8321703: jdeps generates illegal dot file containing nodesep=0,500000
On Mon, 5 Feb 2024 20:11:43 GMT, Mandy Chung wrote: > Trivial fix. Call `PrintWriter::format` with null `Locale` to format with no > localization. > > This PR also fixes JDK-8325262 to print `FindException` message without the > stack trace to indicate clearer that the given module path is incorrect. Thanks for the review. - PR Comment: https://git.openjdk.org/jdk/pull/17713#issuecomment-1930373370
Integrated: 8321703: jdeps generates illegal dot file containing nodesep=0,500000
On Mon, 5 Feb 2024 20:11:43 GMT, Mandy Chung wrote: > Trivial fix. Call `PrintWriter::format` with null `Locale` to format with no > localization. > > This PR also fixes JDK-8325262 to print `FindException` message without the > stack trace to indicate clearer that the given module path is incorrect. This pull request has now been integrated. Changeset: b814c318 Author:Mandy Chung URL: https://git.openjdk.org/jdk/commit/b814c3184e5975e2556911c3a386e6d9bc114d24 Stats: 5 lines in 2 files changed: 1 ins; 0 del; 4 mod 8321703: jdeps generates illegal dot file containing nodesep=0,50 8325262: jdeps can drop printing stack trace when FindException is thrown due to modules not found Reviewed-by: jpai, alanb - PR: https://git.openjdk.org/jdk/pull/17713
RFR: 8321703: jdeps generates illegal dot file containing nodesep=0,500000
Trivial fix. Call `PrintWriter::format` with null `Locale` to format with no localization. This PR also fixes JDK-8325262 to print `FindException` message without the stack trace to indicate clearer that the given module path is incorrect. - Commit messages: - update copyright header - 8321703: jdeps generates illegal dot file containing nodesep=0,50 Changes: https://git.openjdk.org/jdk/pull/17713/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17713=00 Issue: https://bugs.openjdk.org/browse/JDK-8321703 Stats: 5 lines in 2 files changed: 1 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/17713.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17713/head:pull/17713 PR: https://git.openjdk.org/jdk/pull/17713
Re: RFR: 8323835: Updating ASM to 9.6 for JDK 23
On Tue, 16 Jan 2024 21:18:55 GMT, Vicente Romero wrote: > Updating ASM to version 9.6, > > Thanks in advance for the reviews, > Vicente Looks okay to me. I would rely on your testing for verification. - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17453#pullrequestreview-1844965253
Re: RFR: 8323835: Updating ASM to 9.6 for JDK 23
On Tue, 16 Jan 2024 21:18:55 GMT, Vicente Romero wrote: > Updating ASM to version 9.6, > > Thanks in advance for the reviews, > Vicente The change looks okay to me. Most of the changes are doc change. I see many files are updated just to remove the empty line at the end of the file - is that intentional, imported from this upgrade? Any need to update the copyright header? The new file CheckFrameAnalyzer.java still has the same copyright header years as other files. I assume no need then? - PR Comment: https://git.openjdk.org/jdk/pull/17453#issuecomment-1904588248
Integrated: 8159927: Add a test to verify JMOD files created in the images do not have debug symbols
On Wed, 17 Jan 2024 20:51:23 GMT, Mandy Chung wrote: > The build excludes the native debug symbols in JMOD files created for JDK > modules (see make/CreateJmods.gmk). This PR adds a test to verify that > native debug symbols are excluded as expected. This pull request has now been integrated. Changeset: 6c0bebcc Author: Mandy Chung URL: https://git.openjdk.org/jdk/commit/6c0bebccb0092d9726eb89a054e023e92edf7ca6 Stats: 76 lines in 1 file changed: 76 ins; 0 del; 0 mod 8159927: Add a test to verify JMOD files created in the images do not have debug symbols Reviewed-by: jlaskey - PR: https://git.openjdk.org/jdk/pull/17467
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v13]
On Mon, 11 Dec 2023 16:37:38 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with two > additional commits since the last revision: > > - Disallow packaged modules and run-time image link > - Only check for existing path when not a scratch task > >When using a run-time image link and the initial build was produced with >the --keep-packaged-modules option, we don't need to check existing >paths to the location where packaged modules need to be copied. This >breaks the --verbose output validation. > Which filtering plugins do you see with category TRANSFORMER? perhaps? Happy > to move them to the FILTER category. Yes `strip-java-debug-attributes` and `strip-native-debug-symbols` are the ones. `dedup-legal-notices` is another one. > > The use of `--exclude-resources` plugin to exclude transformed data to > > restore the data back to original form is clever and works for plugins that > > add new resources. But it does not work for plugins that modifying existing > > data (`--vendor-bug-url` etc plugins). The change in > > `TaskHelper::getPluginsConfig` raises the question that it isn't the right > > way to support this feature. I think we need to explore a better way to add > > this support. One possibility I consider is to run non-filtering plugins of > > ADDER and TRANSFORMER categories always (similar to auto-enabled). It has > > to determine if it's requested by the user or to restore the data to > > original form via `Plugin::configure` time. `Plugin::transform` will handle > > if the data is from packaged-modules and from runtime image which may > > contain the data transformed by this plugin. I haven't explored this fully > > yet. More discussion is needed and Alan may have opinions. > > So would it be acceptable if I changed those plugins to restore the old > behaviour if they were not requested by the user with the respective cli > option? Basically, my thinking would be that `Plugin::configure` would gain > extra argument so as to determined whether this is a runtime-image based link > and triggered by the current jlink run with its CLI option. This should be > sufficient to configure for `transform` appropriately. My intent is to bring up my observation for discussion. The plugin authors are the ones who should decide if the
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v13]
On Mon, 11 Dec 2023 16:37:38 GMT, Severin Gehwolf wrote:
>> Please review this patch which adds a jlink mode to the JDK which doesn't
>> need the packaged modules being present. A.k.a run-time image based jlink.
>> Fundamentally this patch adds an option to use `jlink` even though your JDK
>> install might not come with the packaged modules (directory `jmods`). This
>> is particularly useful to further reduce the size of a jlinked runtime.
>> After the removal of the concept of a JRE, a common distribution mechanism
>> is still the full JDK with all modules and packaged modules. However,
>> packaged modules can incur an additional size tax. For example in a
>> container scenario it could be useful to have a base JDK container including
>> all modules, but without also delivering the packaged modules. This comes at
>> a size advantage of `~25%`. Such a base JDK container could then be used to
>> `jlink` application specific runtimes, further reducing the size of the
>> application runtime image (App + JDK runtime; as a single image *or*
>> separate bundles, depending on the app
being modularized).
>>
>> The basic design of this approach is to add a jlink plugin for tracking
>> non-class and non-resource files of a JDK install. I.e. files which aren't
>> present in the jimage (`lib/modules`). This enables producing a `JRTArchive`
>> class which has all the info of what constitutes the final jlinked runtime.
>>
>> Basic usage example:
>>
>> $ diff -u <(./bin/java --list-modules --limit-modules java.se)
>> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules
>> --limit-modules java.se)
>> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink)
>> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules
>> --limit-modules jdk.jlink)
>> $ ls ../linux-x86_64-server-release/images/jdk/jmods
>> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod
>> jdk.crypto.ec.jmod jdk.internal.opt.jmod
>> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod
>> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod
>> jdk.dynalink.jmod jdk.internal.vm.ci.jmod
>> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod
>> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod
>> jdk.editpad.jmod jdk.internal.vm.compiler.jmod
>> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod
>> java.desktop.jmod java.scripting.jmod java.xml.jmod
>> jdk.hotspot.agent.jmod jdk.i...
>
> Severin Gehwolf has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Disallow packaged modules and run-time image link
> - Only check for existing path when not a scratch task
>
>When using a run-time image link and the initial build was produced with
>the --keep-packaged-modules option, we don't need to check existing
>paths to the location where packaged modules need to be copied. This
>breaks the --verbose output validation.
`JRTArchive::collectFiles` is the relevant code:
// add/persist a special, empty file for jdk.jlink so as to support
// the single-hop-only run-time image jlink
if (singleHop && JDK_JLINK_MODULE.equals(module)) {
files.add(createRuntimeImageSingleHopStamp());
}
-
PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-1899240758
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v13]
On Thu, 18 Jan 2024 13:37:12 GMT, Severin Gehwolf wrote: > > If I read the code correctly, the image created with this option will > > enable multi-hops unconditionally? i.e. no timestamp file created and > > disable the check completely. I think the .stamp file should be present in > > any image created without packaged modules. > > The option is currently used in tests (and can also be used to verify binary > equivalence of jlinking Java SE with and without packaged modules), which is > a nice property to have. If the stamp file is present in one, but not the > other this is sufficient to fail the equivalence test. What I tried to point out is the following: 1. run `myruntime/bin/jlink` to create `image1` without packaged module 2. run `image1/bin/jlink` to create a runtime image will fail 3. run `image1/bin/jlink --ignore-modified-runtime` will successfully to create `image2` I expect `image2/bin/jlink` should fail creating a runtime image since it's multi-hop. Another scenario: If I modify `image2/conf/net.properties` , `image2/bin/jlink` should fail. In both cases, `image2/bin/jlink --ignore-modified-runtime` will ignore the error and create the runtime image. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-1899238507
RFR: 8159927: Add a test to verify JMOD files created in the images do not have debug symbols
The build excludes the native debug symbols in JMOD files created for JDK modules (see make/CreateJmods.gmk). This PR adds a test to verify that native debug symbols are excluded as expected. - Commit messages: - 8159927: Add a test to verify JMOD files created in the images do not have debug symbols Changes: https://git.openjdk.org/jdk/pull/17467/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17467=00 Issue: https://bugs.openjdk.org/browse/JDK-8159927 Stats: 76 lines in 1 file changed: 76 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17467.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17467/head:pull/17467 PR: https://git.openjdk.org/jdk/pull/17467
Re: RFR: 8323794: Remove unused jimage compressor plugin configuration [v3]
On Tue, 16 Jan 2024 18:42:59 GMT, Claes Redestad wrote: >> There's an unused concept of a pluginConfig that is passed into the jlink >> compress plugins, however we always pass null here and the code seems broken >> (the pluginConfig wouldn't have been stored properly). This seem to be a >> left-over from a more generic plugin design that never came to fruition. >> >> This PR cleans out this code from the plugins and decompressors, while >> keeping the compressed header format intact for backwards compatibility (and >> in case we want to revisit this in the future) > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Copyrights, unused imports Marked as reviewed by mchung (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17443#pullrequestreview-1824981642
Re: RFR: 8323794: Remove unused jimage compressor plugin configuration [v2]
On Tue, 16 Jan 2024 18:03:34 GMT, Claes Redestad wrote: >> There's an unused concept of a pluginConfig that is passed into the jlink >> compress plugins, however we always pass null here and the code seems broken >> (the pluginConfig wouldn't have been stored properly). This seem to be a >> left-over from a more generic plugin design that never came to fruition. >> >> This PR cleans out this code from the plugins and decompressors, while >> keeping the compressed header format intact for backwards compatibility (and >> in case we want to revisit this in the future) > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Named offsets Looks good. - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17443#pullrequestreview-1824528708
Re: RFR: 8321620: Optimize JImage decompressors
On Fri, 12 Jan 2024 18:45:39 GMT, Glavo wrote:
> I generated runtime images using `jlink --compress 2 --add-modules
> java.se,jdk.unsupported,jdk.management` and then ran the following JMH
> benchmark:
>
>
> @Warmup(iterations = 10, time = 2)
> @Measurement(iterations = 5, time = 3)
> @Fork(value = 1, jvmArgsAppend = {"-XX:+UseG1GC", "-Xms8g", "-Xmx8g",
> "--add-exports=java.base/jdk.internal.jimage=ALL-UNNAMED"})
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @State(Scope.Benchmark)
> public class Decompress {
>
> private static final ImageReader READER =
> ImageReaderFactory.getImageReader();
> private static final ImageLocation LOC = READER.findLocation("java.base",
> "java/lang/String.class");
>
> @Benchmark
> public ByteBuffer test() {
> return READER.getResourceBuffer(LOC);
> }
>
> }
>
>
>
> This is the result:
>
>
> Zip
>
> Benchmark Mode Cnt Score Error Units
>Score Error Units
> Decompress.test avgt5 194237.534 ± 1026.180 ns/op
> 152855.728 ± 16058.780 ns/op (-21.30%)
> Decompress.test:gc.alloc.rate avgt51197.700 ± 6.306 MB/sec
> 464.278 ±47.465 MB/sec
> Decompress.test:gc.alloc.rate.norm avgt5 243953.338 ± 5.810B/op
>74376.291 ± 2.175B/op (-69.51%)
> Decompress.test:gc.countavgt5 2.000 counts
>1.000 counts
> Decompress.test:gc.time avgt5 4.000 ms
>3.000 ms
>
>
> The results show that memory allocation is reduced by 70% while decompression
> speed is significantly improved.
Looks good to me. Should wait for @cl4es review as he looked at the previous
version.
-
Marked as reviewed by mchung (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17405#pullrequestreview-1818948375
Re: RFR: 8321620: Optimize JImage decompressors
On Wed, 8 Nov 2023 11:55:22 GMT, Glavo wrote: > This PR significantly speeds up decompressing resources in Jimage while > significantly reducing temporary memory allocations in the process. > > This will improve startup speed for runtime images generated using `jlink > --compress 1` and `jlink --compress 2` . > > I generated a runtime image containing javac using `jlink --compress 1 > --add-modules jdk.compiler` and tested the time it took to compile a simple > HelloWorld program 20 times using `perf stat -r20 javac > /dev/shm/HelloWorld.java`, this PR reduces the total time taken from 17830ms > to 13598ms (31.12% faster). The plan [1] is to remove the old compression values `0`, `1`, `2` in the future and only support the zip compression `--compress zip-[0-9]`, i.e. the string sharing plugin will be removed as there isn't any known customer usage of this plugin. If you are aware of any customer using it, we can reevaluate. [1] https://bugs.openjdk.org/browse/JDK-8301124?focusedId=14562770=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14562770 - PR Comment: https://git.openjdk.org/jdk/pull/16556#issuecomment-1889753527
[jdk22] Integrated: 8323547: tools/jlink/plugins/SystemModuleDescriptors/ModuleMainClassTest.java fails to compile
On Wed, 10 Jan 2024 20:43:19 GMT, Mandy Chung wrote: > `jdk.test.lib.process.ProcessTools::executeTools` was modified to throw > Exception by JDK-8322920 that has only been in the main line. It's a > mistake to backport JDK-8322809 without catching this. Trivial fix - update > the signature to throw Throwable instead. This pull request has now been integrated. Changeset: 34222839 Author: Mandy Chung URL: https://git.openjdk.org/jdk22/commit/34222839e8a9094a12cf4a2a9acc9e7b228e6c40 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod 8323547: tools/jlink/plugins/SystemModuleDescriptors/ModuleMainClassTest.java fails to compile Reviewed-by: naoto - PR: https://git.openjdk.org/jdk22/pull/58
[jdk22] RFR: 8323547: tools/jlink/plugins/SystemModuleDescriptors/ModuleMainClassTest.java fails to compile
`jdk.test.lib.process.ProcessTools::executeTools` was modified to throw Exception by JDK-8322920 that has only been in the main line. It's a mistake to backport JDK-8322809 without catching this. Trivial fix - update the signature to throw Throwable instead. - Commit messages: - 8323547: tools/jlink/plugins/SystemModuleDescriptors/ModuleMainClassTest.java fails to compile Changes: https://git.openjdk.org/jdk22/pull/58/files Webrev: https://webrevs.openjdk.org/?repo=jdk22=58=00 Issue: https://bugs.openjdk.org/browse/JDK-8323547 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk22/pull/58.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/58/head:pull/58 PR: https://git.openjdk.org/jdk22/pull/58
[jdk22] Integrated: 8322809: SystemModulesMap::classNames and moduleNames arrays do not match the order
On Tue, 9 Jan 2024 22:16:59 GMT, Mandy Chung wrote: > This pull request contains a backport of commit > [f3be138e](https://github.com/openjdk/jdk/commit/f3be138eb80c9e7f6cc21afb75cda9e49b667c8a) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Mandy Chung on 9 Jan 2024 and was > reviewed by Alan Bateman. This pull request has now been integrated. Changeset: 71cc879b Author:Mandy Chung URL: https://git.openjdk.org/jdk22/commit/71cc879bd46ece4979e7beaed5461d373bdb196f Stats: 290 lines in 6 files changed: 282 ins; 0 del; 8 mod 8322809: SystemModulesMap::classNames and moduleNames arrays do not match the order Reviewed-by: alanb Backport-of: f3be138eb80c9e7f6cc21afb75cda9e49b667c8a - PR: https://git.openjdk.org/jdk22/pull/46
