RFR: 8321622: ClassFile.verify(byte[] bytes) throws unexpected ConstantPoolException, IAE

[Adam Sotona]

Class-File API `VerifierImpl` contains debugging stack trace print causing 
unexpected output and confusion in specific verification cases.

This patch removes the stack trace print.

Please review.

Thanks,
Adam

-

Commit messages:
 - 8321622: ClassFile.verify(byte[] bytes) throws unexpected 
ConstantPoolException, IAE

Changes: https://git.openjdk.org/jdk/pull/19258/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19258&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8321622
  Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/19258.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19258/head:pull/19258

PR: https://git.openjdk.org/jdk/pull/19258

Re: Deterministic naming of subclasses of `java/lang/reflect/Proxy`

[-]

Hi Aman,
I think you meant CVE-2021-42392 instead of 2022.

For your approach of an "allowlist" for Java runtime, project Leyden is
looking to generate a static image [1], that
> At run time it cannot load classes from outside the image, nor can it
create classes dynamically.
Leyden mainly avoids this unstable generation by performing a training run
to collect classes loaded and even object graphs; I am not familiar with
the details unfortunately.

Otherwise, the Proxy discussion belongs better to core-libs-dev, as
java.lang.reflect.Proxy is part of Java's core libraries. I am replying
this thread to core-libs-dev.

For your perceived problem that classes don't have unique names, your
description sounds dubious: GeneratedConstructorAccessor is already retired
by JEP 416 [2] in Java 18, and there are many other cases in which JDK
generates classes without stable names, notoriously LambdaMetafactory
(Gradle wished for cacheable Lambdas); the same applies for the generated
classes for MethodHandle's LambdaForms (which carries implementation code
for LambdaForm). How are you checking the classes? It seems you are not
checking hidden classes. Proxy and Lambda classes are defined by the
caller's class loader, while LambdaForms are under JDK's system class
loader I think. We need to ensure you are correctly finding all unstable
classes before we can proceed.

[1]: https://openjdk.org/projects/leyden/notes/01-beginnings
[2]: https://openjdk.org/jeps/416

On Wed, May 15, 2024 at 7:00 PM Aman Sharma  wrote:

> Hi,
>
>
> My name is Aman and I am a PhD student at KTH Royal Institute of
> Technology, Stockholm, Sweden. I research as part of CHAINS
>  project to strengthen the software supply
> chain of multiple ecosystem. I particularly focus on runtime integrity in
> Java. In this email, I want to write about an issue I have discovered with 
> *dynamic
> generation of `java.lang.reflect.Proxy`classes*. I will propose a
> solution and would love to hear the feedback from the community. Let me
> know if this is the correct mailing-list for such discussions. It seemed
> the most relevant from this list
> .
>
>
> *My research*
>
>
> Java has features to load class on the fly - it can either download or
> generate a class at runtime. These features are useful for inner workings
> of JDK. For example, implementing annotations, reflective access, etc.
> However, these features have also contributed to critical vulnerabilities
> in the past - CVE-2021-44228  (log4shell), CVE-2022-33980, CVE-2022-42392.
> All of these vulnerabilities have one thing in common - *a class that was
> not known during build time was downloaded/generated at runtime and loaded
> into JVM.*
>
>
> To defend against such vulnerabilities, we propose a solution to *allowlist
> classes for runtime*. This allowlist will contain an exhaustive list of
> classes that can be loaded by the JVM and it will be enforced at runtime.
> We build this allowlist from three sources:
>
>1. All classes of all modules provided by the Java Standard Library.
>We use ClassGraph  to scan
>the JDK.
>2. We can take the source code and all dependencies of an application.
>We use a software bill of materials to get all the data.
>3. Finally, we use run the test suite to include any runtime
>downloaded/generated classes.
>
> Such a list is able to prevent the above 3 CVEs because it does not let
> the "unknown" bytecode to be loaded.
>
> *Problem with generating such an allowlist*
>
> The first two parts of the allowlist are easy to get. The problem is with
> the third step where we want to allowlist all the classes that could be
> downloaded or generated. Upon running the test suite and hooking to the
> classes it loads, we observer that the list consists of classes that are
> called "com/sun/proxy/$Proxy2", "
> jdk/internal/reflect/GeneratedConstructorAccessor3" among many more. The
> purpose of these classes can be identifed. The proxy class is created for
> to implement an annotation. The accessor gives access to constructor of a
> class to the JVM.
>
> When enforcing this allowlist at runtime, we see that the bytecode content
> for "com/sun/proxy/$Proxy2" differs in the allowlist and at runtime. In
> our case, we we are experimenting with pdfbox
>  so we created the allowlist using its
> test suite. Then we enforced this allowlist while running some of its
> subcommands. However, there was some other proxy class say 
> "com/sun/proxy/$Proxy5"
> at runtime that implemented the same interfaces and had the same methods as
> "com/sun/proxy/$Proxy2" in the allowlist. They only differed in the name
> of the class, order of fields, and types for fields references. This could
> happen because the order of the loading of class is workload dependent, but
> it causes problem to generate such an allowlist.
>
>
> *Sol

Re: RFR: 8332340: Add JavacBench as a test case for CDS

[Chen Liang]

On Thu, 16 May 2024 02:37:02 GMT, Ioi Lam  wrote:

> JavacBench is a test program that compiles 90 Java source files. It uses a 
> fair amount of invokedynamic callsites, so it's good for testing CDS support 
> for indy and lambda expressions.
> 
> This test was first integrated into the 
> [leyden](https://github.com/openjdk/leyden/tree/premain) repo. Hence some of 
> the files have copyrights in 2023.

test/lib/jdk/test/lib/StringArrayUtils.java line 33:

> 31: }
> 32: 
> 33: public static String[] concat(String prefix[], String... extra) {

Suggestion:

public static String[] concat(String[] prefix, String... extra) {

test/lib/jdk/test/lib/StringArrayUtils.java line 42:

> 40: }
> 41: 
> 42: return list.toArray(new String[list.size()]);

I thought we have been preferring ot use `new String[0]` for toArray calls. 
Also for simplicity, we can change the implementation to:

var list = new ArrayList<>(Arrays.asList(prefix));
Collections.addAll(list, extra);
return list.toArray(new String[0]);

or for performance:

String[] ret = new String[prefix.length + extra.length];
System.arraycopy(prefix, 0, ret, 0, prefix.length);
System.arraycopy(extra, 0, ret, prefix.length, extra.length);
return ret;

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19256#discussion_r1602560570
PR Review Comment: https://git.openjdk.org/jdk/pull/19256#discussion_r1602561754

RFR: 8332340: Add JavacBench as a test case for CDS

[Ioi Lam]

JavacBench is a test program that compiles 90 Java source files. It uses a fair 
amount of invokedynamic callsites, so it's good for testing CDS support for 
indy and lambda expressions.

This test was first integrated into the 
[leyden](https://github.com/openjdk/leyden/tree/premain) repo. Hence some of 
the files have copyrights in 2023.

-

Commit messages:
 - 8332340: Add JavacBench as a test case for CDS

Changes: https://git.openjdk.org/jdk/pull/19256/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19256&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8332340
  Stats: 592 lines in 4 files changed: 592 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/19256.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19256/head:pull/19256

PR: https://git.openjdk.org/jdk/pull/19256

Re: [External] : Re: New candidate JEP: 471: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal

[-]

Indeed, ReflectionFactory. newConstructorForSerialization can be used to
access otherwise-private constructors. Before JDK-8315810, it could even
allocate one class and call the constructor of another class.

I strongly support your proposal to restrict ReflectionFactory.

Regards, Chen

On Wed, May 15, 2024 at 6:23 PM David Lloyd  wrote:

>
>
> On Tue, May 14, 2024 at 10:01 AM David Lloyd 
> wrote:
>
>> (Moving to core-libs-dev)
>>
>> On Tue, May 14, 2024 at 9:40 AM Alan Bateman 
>> wrote:
>>
>>> On 14/05/2024 14:42, David Lloyd wrote:
>>>
>>> ReflectionFactory allows access to serialization facilities without any
>>> access checking (other than the defunct SecurityManager checks). Perhaps
>>> this class could gain some more methods, like this:
>>>
>>> * `newGetterForSerialization(Field field)` - includes ability to access
>>> `objectStreamFields` and `serialVersionUID`, or these could be separate
>>> methods
>>> * `newSetterForSerialziation(Field field)`
>>>
>>> Does this seem workable?
>>>
>>> The intention with ReflectionFactory is that serialization libraries go
>>> through the readObject/writeObject and other magic methods, to avoid field
>>> access.
>>>
>>> Probably best to being this to core-libs-dev for further discussion.
>>>
>>
>> This doesn't match my recollection. The `readObject` and `writeObject`
>> methods are optional private methods which serializable classes *may*
>> provide when they want a bespoke serialization strategy (and the other
>> methods that are accessed via this special class are similar in this
>> regard). They are not in any way magical in that they do not provide the
>> means to perform this part of the serialization process, and thus they are
>> not a substitute for field access in serialization. According to
>> JDK-8164908, these methods were added because at the time, Unsafe was still
>> state of the art for custom serialization and IIOP to access fields (with
>> libraries using Field actively moving to Unsafe instead). However Unsafe
>> did not and does not provide access to methods, so in order to avoid the
>> aforementioned `add-opens` explosion, these methods were added as a
>> compromise. Now that Unsafe is being deprecated, it is my opinion that this
>> does need to be revisited because at the time, Unsafe was the recommended
>> approach.
>>
>> [1] https://bugs.openjdk.org/browse/JDK-8164908
>>
> It seems to me that it might be a good idea (as part of 8305968 (Integrity
> by default)) to put the `ReflectionFactory` API behind a restricted method
> call somehow, even considered separately from the changes suggested above.
> Maybe in conjunction with a non-standard switch that works similarly to
> `--enable-native-access`, e.g.
> `--enable-serialization=org.serial.framework`? That would also somewhat
> mitigate the security risk of adding the aforementioned methods or
> something like them to this class.
>
> Should I open a bug for either (or both) of these suggestions? Or perhaps
> there is a better way to proceed?
>
> --
> - DML • he/him
>

Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`

[Jaikiran Pai]

On Fri, 10 May 2024 01:25:45 GMT, xiaotaonan  wrote:

>> Clean up non-standard use of /// comments in `java.base`
>
> @mdinacci @hns  @landonf

Hello @xiaotaonan, like Jon noted in his comment, there's already another PR 
addressing this change. So I think this current PR can be closed.

-

PR Comment: https://git.openjdk.org/jdk/pull/19151#issuecomment-2113730374

Re: Generalizing binary search

[Louis Wasserman]

If you forget to implement RandomAccess, you'll get a very subtle
performance bug.

Just because it's easy if you know how to do it doesn't make it easy for a
random developer to do correctly.

On Wed, May 15, 2024 at 3:21 PM Mateusz Romanowski <
romanowski.mate...@gmail.com> wrote:

> Hi,
> I would say it is not worth any effort.
>
> One can easily write an ad-hoc *local* adapter extending
> `java.util.AbstractList`..
> .. and immediately invoke existing `java.util.Collections::binarySearch`
> method.
>
> Cheers,
> MR
>
> On Thu, Apr 25, 2024 at 9:09 PM Pavel Rappo 
> wrote:
>
>>
>>
>> > On 25 Apr 2024, at 19:41, David Lloyd  wrote:
>> >
>> > The JDK contains a few collection- and array-oriented implementations
>> of binary search. For the most part, they all work the same way: search the
>> target "thing" by index using the obvious bisection strategy, returning
>> either /index/ or /-index-1/ depending on whether it was found at the end
>> of the search.
>> >
>> > However, if you're doing a binary search on a thing that is not a list
>> or an array, you have two choices: try to make the thing you're searching
>> on implement List (often infeasible) or write your own binary search.
>> >
>> > I'm really tired of writing my own binary search. I've probably done it
>> 50 times by now, each one using a slightly different access and/or compare
>> strategy, and every time is a new chance to typo something or get something
>> backwards, leading to irritating debugging sessions and higher dentist
>> bills.
>>
>> Can we safely say that it sets your teeth on edge?
>>
>> > It got me to thinking that it wouldn't be too hard to make a "general"
>> binary search which can search on anything, so that's what I did. I was
>> thinking that it might be interesting to try adding this into the JDK
>> somehow.
>> >
>> > This implementation is more or less what I now copy & paste to
>> different projects at the moment:
>> >
>> > public static  int binarySearch(C collection, int start, int
>> end, T key, Comparator cmp, IntGetter getter) {
>> > int low = start;
>> > int high = end - 1;
>> > while (low <= high) {
>> > int mid = low + high >>> 1;
>> > int res = cmp.compare(getter.get(collection, mid), key);
>> > if (res < 0) {
>> > low = mid + 1;
>> > } else if (res > 0) {
>> > high = mid - 1;
>> > } else {
>> > return mid;
>> > }
>> > }
>> > return -low - 1;
>> > }
>> > // (Plus variations for `Comparable` keys and long indices)
>> >
>> > A key problem with this approach is that in the JDK, there is no
>> `ObjIntFunction` or `ObjLongFunction` which would be necessary
>> to implement the "getter" portion of the algorithm (despite the existence
>> of the analogous `ObjIntConsumer` and `ObjLongConsumer`). So, I also
>> have to copy/paste a `IntGetter`/`LongGetter` as well, which is
>> annoying.
>> >
>> > A slightly less-good approach is for the general `binarySearch` method
>> to accept a `IntFunction`/`LongFunction`, and drop the `C collection`
>> parameter, like this:
>> >
>> > public static  int binarySearch(int start, int end, T key,
>> Comparator cmp, IntFunction getter) { ... }
>> >
>> > In this case, we can use the function types that the JDK already
>> provides, but we would very likely have to also create a capturing lambda
>> (e.g. `myList::get` instead of `List::get`). Maybe this isn't that bad of a
>> compromise.
>> >
>> > It would be possible to replace the existing `binarySearch`
>> implementations with delegating calls to a generalized implementation. For
>> `Collections`, the indexed version uses `List::get` and the iterator
>> version uses a helper lambda to move the iterator and get the result. For
>> arrays, a lambda would be provided which gets the corresponding array
>> element. If the non-capturing variant is used, then (on paper at least)
>> this version should perform similarly to the existing implementations, with
>> less code needed overall. However, if a capturing lambda is required (due
>> to the aforementioned lack of `ObjXFunction`), then this could be slightly
>> worse-performing than the existing implementation due to the construction
>> (and maybe dispatch) overhead of the lambda. Some real-world benchmarks
>> would have to be written with various-sized data sets.
>> >
>> > It would also be possible to produce primitive variations which operate
>> on int, float, long, and double values, using existing functions if
>> capturing is deemed "OK". It is also possible to produce a variation which
>> uses a `long` for the index, for huge data sets (for example, very large
>> mapped files using `MemorySegment`).
>> >
>> > Also unclear is: where would it live? `Collections`? Somewhere else?
>> >
>> > Any thoughts/opinions would be appreciated (even if they are along the
>> lines of "it's not worth the effort"). Particularly, any insight

Re: Generalizing binary search

[Mateusz Romanowski]

Hi,
I would say it is not worth any effort.

One can easily write an ad-hoc *local* adapter extending
`java.util.AbstractList`..
.. and immediately invoke existing `java.util.Collections::binarySearch`
method.

Cheers,
MR

On Thu, Apr 25, 2024 at 9:09 PM Pavel Rappo  wrote:

>
>
> > On 25 Apr 2024, at 19:41, David Lloyd  wrote:
> >
> > The JDK contains a few collection- and array-oriented implementations of
> binary search. For the most part, they all work the same way: search the
> target "thing" by index using the obvious bisection strategy, returning
> either /index/ or /-index-1/ depending on whether it was found at the end
> of the search.
> >
> > However, if you're doing a binary search on a thing that is not a list
> or an array, you have two choices: try to make the thing you're searching
> on implement List (often infeasible) or write your own binary search.
> >
> > I'm really tired of writing my own binary search. I've probably done it
> 50 times by now, each one using a slightly different access and/or compare
> strategy, and every time is a new chance to typo something or get something
> backwards, leading to irritating debugging sessions and higher dentist
> bills.
>
> Can we safely say that it sets your teeth on edge?
>
> > It got me to thinking that it wouldn't be too hard to make a "general"
> binary search which can search on anything, so that's what I did. I was
> thinking that it might be interesting to try adding this into the JDK
> somehow.
> >
> > This implementation is more or less what I now copy & paste to different
> projects at the moment:
> >
> > public static  int binarySearch(C collection, int start, int
> end, T key, Comparator cmp, IntGetter getter) {
> > int low = start;
> > int high = end - 1;
> > while (low <= high) {
> > int mid = low + high >>> 1;
> > int res = cmp.compare(getter.get(collection, mid), key);
> > if (res < 0) {
> > low = mid + 1;
> > } else if (res > 0) {
> > high = mid - 1;
> > } else {
> > return mid;
> > }
> > }
> > return -low - 1;
> > }
> > // (Plus variations for `Comparable` keys and long indices)
> >
> > A key problem with this approach is that in the JDK, there is no
> `ObjIntFunction` or `ObjLongFunction` which would be necessary
> to implement the "getter" portion of the algorithm (despite the existence
> of the analogous `ObjIntConsumer` and `ObjLongConsumer`). So, I also
> have to copy/paste a `IntGetter`/`LongGetter` as well, which is
> annoying.
> >
> > A slightly less-good approach is for the general `binarySearch` method
> to accept a `IntFunction`/`LongFunction`, and drop the `C collection`
> parameter, like this:
> >
> > public static  int binarySearch(int start, int end, T key,
> Comparator cmp, IntFunction getter) { ... }
> >
> > In this case, we can use the function types that the JDK already
> provides, but we would very likely have to also create a capturing lambda
> (e.g. `myList::get` instead of `List::get`). Maybe this isn't that bad of a
> compromise.
> >
> > It would be possible to replace the existing `binarySearch`
> implementations with delegating calls to a generalized implementation. For
> `Collections`, the indexed version uses `List::get` and the iterator
> version uses a helper lambda to move the iterator and get the result. For
> arrays, a lambda would be provided which gets the corresponding array
> element. If the non-capturing variant is used, then (on paper at least)
> this version should perform similarly to the existing implementations, with
> less code needed overall. However, if a capturing lambda is required (due
> to the aforementioned lack of `ObjXFunction`), then this could be slightly
> worse-performing than the existing implementation due to the construction
> (and maybe dispatch) overhead of the lambda. Some real-world benchmarks
> would have to be written with various-sized data sets.
> >
> > It would also be possible to produce primitive variations which operate
> on int, float, long, and double values, using existing functions if
> capturing is deemed "OK". It is also possible to produce a variation which
> uses a `long` for the index, for huge data sets (for example, very large
> mapped files using `MemorySegment`).
> >
> > Also unclear is: where would it live? `Collections`? Somewhere else?
> >
> > Any thoughts/opinions would be appreciated (even if they are along the
> lines of "it's not worth the effort"). Particularly, any insight would be
> appreciated as to whether or not this kind of hypothetical enhancement
> would warrant a JEP (I wouldn't think so, but I'm no expert at such
> assessments).
> >
> > --
> > - DML • he/him
>
> Have a look at this recently filed issue:
> https://bugs.openjdk.org/browse/JDK-8326330
>
> -Pavel
>
>
>

Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v19]

[Sandhya Viswanathan]

On Sat, 4 May 2024 19:35:21 GMT, Scott Gibbons  wrote:

>> Re-write the IndexOf code without the use of the pcmpestri instruction, only 
>> using AVX2 instructions.  This change accelerates String.IndexOf on average 
>> 1.3x for AVX2.  The benchmark numbers:
>> 
>> 
>> Benchmark   Score
>> Latest  
>> StringIndexOf.advancedWithMediumSub   343.573317.934 
>> 0.925375393x
>> StringIndexOf.advancedWithShortSub11039.081  1053.96 
>> 1.014319384x
>> StringIndexOf.advancedWithShortSub255.828110.541 
>> 1.980027943x
>> StringIndexOf.constantPattern9.361   11.906  
>> 1.271872663x
>> StringIndexOf.searchCharLongSuccess  4.216   4.218   
>> 1.000474383x
>> StringIndexOf.searchCharMediumSuccess3.133   3.216   
>> 1.02649218x
>> StringIndexOf.searchCharShortSuccess 3.763.761   
>> 1.000265957x
>> StringIndexOf.success9.186   
>> 9.713   1.057369911x
>> StringIndexOf.successBig   14.34146.343  
>> 3.231504079x
>> StringIndexOfChar.latin1_AVX2_String   6220.918  12154.52
>> 1.953814533x
>> StringIndexOfChar.latin1_AVX2_char 5503.556  5540.044
>> 1.006629895x
>> StringIndexOfChar.latin1_SSE4_String   6978.854  6818.689
>> 0.977049957x
>> StringIndexOfChar.latin1_SSE4_char 5657.499  5474.624
>> 0.967675646x
>> StringIndexOfChar.latin1_Short_String  7132.541  
>> 6863.3590.962260014x
>> StringIndexOfChar.latin1_Short_char  16013.389 16162.437 
>> 1.009307711x
>> StringIndexOfChar.latin1_mixed_String  7386.12314771.622 
>> 1.15517x
>> StringIndexOfChar.latin1_mixed_char9901.671  9782.245
>> 0.987938803
>
> Scott Gibbons has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Rearrange; add lambdas for clarity

src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1476:

> 1474:_masm);
> 1475: 
> 1476:   __ movq(r11, -1);

There doesn't seem to be a use of r11 below in this function.

src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1493:

> 1491:   // Assume r10 is n - k
> 1492:   __ leaq(last, Address(haystack, r10, Address::times_1, isU ? -30 : 
> -31));
> 1493:   __ jmpb(temp);

Need to pass r10 as parameter. Also temp label could be given a better name.

src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1502:

> 1500: 
> 1501:   __ cmpq(hsPtrRet, last);
> 1502:   __ cmovq(Assembler::aboveEqual, hsPtrRet, last);

cmovq is expensive, better sequence would be:

__ cmpq(hsPtrRet, last);
__ jb_b(temp);
__ movq(hsPtrRet, last);

src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1510:

> 1508:   compare_big_haystack_to_needle(sizeKnown, size, 
> NUMBER_OF_NEEDLE_BYTES_TO_COMPARE, loop_top, hsPtrRet, hsLength,
> 1509:  needleLen, isU, DO_EARLY_BAILOUT, 
> eq_mask, temp2, r10, _masm);
> 1510: 

At this point hsLength is not the remaining length from hsPtrRet, would that 
cause a problem? If not, all the special paths in 
compare_big_haystack_to_needle need not be generated on this call.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1602016421
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1601943761
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1602251994
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1602010926

Re: [External] : Re: New candidate JEP: 471: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal

[David Lloyd]

On Tue, May 14, 2024 at 10:01 AM David Lloyd  wrote:

> (Moving to core-libs-dev)
>
> On Tue, May 14, 2024 at 9:40 AM Alan Bateman 
> wrote:
>
>> On 14/05/2024 14:42, David Lloyd wrote:
>>
>> ReflectionFactory allows access to serialization facilities without any
>> access checking (other than the defunct SecurityManager checks). Perhaps
>> this class could gain some more methods, like this:
>>
>> * `newGetterForSerialization(Field field)` - includes ability to access
>> `objectStreamFields` and `serialVersionUID`, or these could be separate
>> methods
>> * `newSetterForSerialziation(Field field)`
>>
>> Does this seem workable?
>>
>> The intention with ReflectionFactory is that serialization libraries go
>> through the readObject/writeObject and other magic methods, to avoid field
>> access.
>>
>> Probably best to being this to core-libs-dev for further discussion.
>>
>
> This doesn't match my recollection. The `readObject` and `writeObject`
> methods are optional private methods which serializable classes *may*
> provide when they want a bespoke serialization strategy (and the other
> methods that are accessed via this special class are similar in this
> regard). They are not in any way magical in that they do not provide the
> means to perform this part of the serialization process, and thus they are
> not a substitute for field access in serialization. According to
> JDK-8164908, these methods were added because at the time, Unsafe was still
> state of the art for custom serialization and IIOP to access fields (with
> libraries using Field actively moving to Unsafe instead). However Unsafe
> did not and does not provide access to methods, so in order to avoid the
> aforementioned `add-opens` explosion, these methods were added as a
> compromise. Now that Unsafe is being deprecated, it is my opinion that this
> does need to be revisited because at the time, Unsafe was the recommended
> approach.
>
> [1] https://bugs.openjdk.org/browse/JDK-8164908
>
It seems to me that it might be a good idea (as part of 8305968 (Integrity
by default)) to put the `ReflectionFactory` API behind a restricted method
call somehow, even considered separately from the changes suggested above.
Maybe in conjunction with a non-standard switch that works similarly to
`--enable-native-access`, e.g.
`--enable-serialization=org.serial.framework`? That would also somewhat
mitigate the security risk of adding the aforementioned methods or
something like them to this class.

Should I open a bug for either (or both) of these suggestions? Or perhaps
there is a better way to proceed?

-- 
- DML • he/him

Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v19]

[Volodymyr Paprotski]

On Wed, 15 May 2024 19:21:37 GMT, Volodymyr Paprotski  wrote:

>> Scott Gibbons has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Rearrange; add lambdas for clarity
>
> test/jdk/java/lang/StringBuffer/IndexOf.java line 47:
> 
>> 45:   char[] haystack_16 = new char[128];
>> 46: 
>> 47:   for (int i = 0; i < 128; i++) {
> 
> you can use `char` instead of `int` as iterator

combine into single loop
haystack[i] = (char) i;
haystack_16[i] = (char) (i + 256);

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1602141543

Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v19]

[Volodymyr Paprotski]

On Sat, 4 May 2024 19:35:21 GMT, Scott Gibbons  wrote:

>> Re-write the IndexOf code without the use of the pcmpestri instruction, only 
>> using AVX2 instructions.  This change accelerates String.IndexOf on average 
>> 1.3x for AVX2.  The benchmark numbers:
>> 
>> 
>> Benchmark   Score
>> Latest  
>> StringIndexOf.advancedWithMediumSub   343.573317.934 
>> 0.925375393x
>> StringIndexOf.advancedWithShortSub11039.081  1053.96 
>> 1.014319384x
>> StringIndexOf.advancedWithShortSub255.828110.541 
>> 1.980027943x
>> StringIndexOf.constantPattern9.361   11.906  
>> 1.271872663x
>> StringIndexOf.searchCharLongSuccess  4.216   4.218   
>> 1.000474383x
>> StringIndexOf.searchCharMediumSuccess3.133   3.216   
>> 1.02649218x
>> StringIndexOf.searchCharShortSuccess 3.763.761   
>> 1.000265957x
>> StringIndexOf.success9.186   
>> 9.713   1.057369911x
>> StringIndexOf.successBig   14.34146.343  
>> 3.231504079x
>> StringIndexOfChar.latin1_AVX2_String   6220.918  12154.52
>> 1.953814533x
>> StringIndexOfChar.latin1_AVX2_char 5503.556  5540.044
>> 1.006629895x
>> StringIndexOfChar.latin1_SSE4_String   6978.854  6818.689
>> 0.977049957x
>> StringIndexOfChar.latin1_SSE4_char 5657.499  5474.624
>> 0.967675646x
>> StringIndexOfChar.latin1_Short_String  7132.541  
>> 6863.3590.962260014x
>> StringIndexOfChar.latin1_Short_char  16013.389 16162.437 
>> 1.009307711x
>> StringIndexOfChar.latin1_mixed_String  7386.12314771.622 
>> 1.15517x
>> StringIndexOfChar.latin1_mixed_char9901.671  9782.245
>> 0.987938803
>
> Scott Gibbons has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Rearrange; add lambdas for clarity

First pass at StringIndexOfHuge.java and IndexOf.java

test/jdk/java/lang/StringBuffer/IndexOf.java line 40:

> 38:   private static boolean failure = false;
> 39:   public static void main(String[] args) throws Exception {
> 40: String testName = "IndexOf";

intentation

test/jdk/java/lang/StringBuffer/IndexOf.java line 47:

> 45:   char[] haystack_16 = new char[128];
> 46: 
> 47:   for (int i = 0; i < 128; i++) {

you can use `char` instead of `int` as iterator

test/jdk/java/lang/StringBuffer/IndexOf.java line 54:

> 52:   // for (int i = 1; i < 128; i++) {
> 53:   //   haystack_16[i] = (char) (i);
> 54:   // }

dead code

test/jdk/java/lang/StringBuffer/IndexOf.java line 64:

> 62:   Charset hs_charset = StandardCharsets.UTF_16;
> 63:   Charset needleCharset = StandardCharsets.ISO_8859_1;
> 64:   // Charset needleCharset = StandardCharsets.UTF_16;

Move from main() into a function that takes `needleCharset` as a parameter, 
then call that function twice.

test/jdk/java/lang/StringBuffer/IndexOf.java line 81:

> 79: sourceBuffer = new StringBuffer(sourceString);
> 80: targetString = generateTestString(10, 11);
> 81: } while (sourceString.indexOf(targetString) != -1);

Should really keep the original test unmodified and add new tests as needed

test/jdk/java/lang/StringBuffer/IndexOf.java line 83:

> 81:   shs = "$&),,18+-!'8)+";
> 82:   endNeedle = "8)-";
> 83:   l_offset = 9;

dead code

test/jdk/java/lang/StringBuffer/IndexOf.java line 89:

> 87:   StringBuffer bshs = new StringBuffer(shs);
> 88: 
> 89:   // printStringBytes(shs.getBytes(hs_charset));

dead code (and next two comments)

test/jdk/java/lang/StringBuffer/IndexOf.java line 90:

> 88: 
> 89:   // printStringBytes(shs.getBytes(hs_charset));
> 90:   for (int i = 0; i < 20; i++) {

This wont be a deterministic way to reach the intrinsic. I would suggest 
copying the idea from 
test/jdk/com/sun/crypto/provider/Cipher/ChaCha20/unittest/Poly1305UnitTestDriver.java

i.e. Have two `@run main` invocations at the top of this file, one with default 
parameters, one with  `-Xcomp -XX:-TieredCompilation`. You dont need a 'driver' 
program, that was to handle something else.


/*
 * @test
 * @modules java.base/com.sun.crypto.provider
 * @run main java.base/com.sun.crypto.provider.Poly1305KAT
 * @summary Unit test for com.sun.crypto.provider.Poly1305.
 */

/*
 * @test
 * @modules java.base/com.sun.crypto.provider
 * @summary Unit test for IntrinsicCandidate in 
com.sun.crypto.provider.Poly1305.
 * @run main/othervm -Xcomp -XX:-TieredCompilation 
-XX:+UnlockDiagnosticVMOptions -XX:+ForceUnreachable 
java.base/com.sun.crypto.provider.Poly1305KAT
 */

test/jdk/java/lang/StringBuff

Integrated: 8330276: Console methods with explicit Locale

[Naoto Sato]

On Tue, 23 Apr 2024 20:35:43 GMT, Naoto Sato  wrote:

> Proposing new overloaded methods in `java.io.Console` class that explicitly 
> take a `Locale` argument. Existing methods rely on the default locale, so the 
> users won't be able to format their prompts/outputs in a certain locale 
> explicitly.

This pull request has now been integrated.

Changeset: 7cff04fc
Author:Naoto Sato 
URL:   
https://git.openjdk.org/jdk/commit/7cff04fc8a8114a297437aa526b18b6185831eac
Stats: 441 lines in 8 files changed: 337 ins; 24 del; 80 mod

8330276: Console methods with explicit Locale

Reviewed-by: joehw, rriggs, jlahoda

-

PR: https://git.openjdk.org/jdk/pull/18923

Re: RFR: 8330465: Stable Values and Collections (Internal) [v5]

[Chen Liang]

On Wed, 15 May 2024 18:49:49 GMT, ExE Boss  wrote:

>> src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java 
>> line 240:
>> 
>>> 238: }
>>> 239: } finally {
>>> 240: supplying = false;
>> 
>> Resetting a stable field is a bad idea. I recommend renaming this to 
>> `supplierCalled` or `supplied` so we never transition this false -> true
>
> Yes, according to the `@Stable` annotation’s JavaDoc, this is UB:
> https://github.com/openjdk/jdk/blob/8a4315f833f3700075d65fae6bc566011c837c07/src/java.base/share/classes/jdk/internal/vm/annotation/Stable.java#L74-L80

Fyi what usually happens is that if a stable field or similarly constant-folded 
field is promoted to constant, the constant promotion can happen to any of the 
previous valid values written.

MethodHandle optimisitically sets a trusted final field this way:
https://github.com/openjdk/jdk/blob/8a4315f833f3700075d65fae6bc566011c837c07/src/java.base/share/classes/java/lang/invoke/MethodHandle.java#L1868-L1870

Also a similar example in user code targeting older Java releases, before JDK 
16's strong encapsulation so that enums could have been added by reflection:
https://github.com/MinecraftForge/MinecraftForge/issues/3885#issuecomment-355602542

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1602125255

Re: RFR: 8330465: Stable Values and Collections (Internal) [v5]

[ExE Boss]

On Wed, 15 May 2024 16:10:06 GMT, Chen Liang  wrote:

>> Per Minborg has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Switch to monomorphic StableValue and use lazy arrays
>
> src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java 
> line 104:
> 
>> 102: // Optimistically try plain semantics first
>> 103: final V v = value;
>> 104: if (v != null) {
> 
> If `value == null && state == NULL`, can the path still be constant folded? I 
> doubt it because `value` in this case may not be promoted to constant.

Maybe the `state == NULL` check should be moved before `v != null`, as the 
**JIT** doesn’t constant‑fold `null` [`@Stable`] values:
https://github.com/openjdk/jdk/blob/8a4315f833f3700075d65fae6bc566011c837c07/src/java.base/share/classes/jdk/internal/vm/annotation/Stable.java#L41-L44
https://github.com/openjdk/jdk/blob/8a4315f833f3700075d65fae6bc566011c837c07/src/java.base/share/classes/jdk/internal/vm/annotation/Stable.java#L64-L71

[`@Stable`]: 
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/vm/annotation/Stable.java

> src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java 
> line 240:
> 
>> 238: }
>> 239: } finally {
>> 240: supplying = false;
> 
> Resetting a stable field is a bad idea. I recommend renaming this to 
> `supplierCalled` or `supplied` so we never transition this false -> true

Yes, according to the `@Stable` annotation’s JavaDoc, this is UB:
https://github.com/openjdk/jdk/blob/8a4315f833f3700075d65fae6bc566011c837c07/src/java.base/share/classes/jdk/internal/vm/annotation/Stable.java#L74-L80

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1602101301
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1602106099

Re: RFR: 8330465: Stable Values and Collections (Internal) [v5]

[ExE Boss]

On Wed, 15 May 2024 15:27:34 GMT, Per Minborg  wrote:

>> # Stable Values & Collections (Internal)
>> 
>> ## Summary
>> This PR proposes to introduce an internal _Stable Values & Collections_ API, 
>> which provides immutable value holders where elements are initialized _at 
>> most once_. Stable Values & Collections offer the performance and safety 
>> benefits of final fields while offering greater flexibility as to the timing 
>> of initialization.
>> 
>> ## Goals
>>  * Provide an easy and intuitive API to describe value holders that can 
>> change at most once.
>>  * Decouple declaration from initialization without significant footprint or 
>> performance penalties.
>>  * Reduce the amount of static initializer and/or field initialization code.
>>  * Uphold integrity and consistency, even in a multi-threaded environment.
>>  
>> For more details, see the draft JEP: https://openjdk.org/jeps/8312611
>> 
>> ## Performance 
>> Performance compared to instance variables using two `AtomicReference` and 
>> two protected by double-checked locking under concurrent access by all 
>> threads:
>> 
>> 
>> Benchmark  Mode  Cnt  Score  
>> Error   Units
>> StableBenchmark.atomicthrpt   10259.478 ?   
>> 36.809  ops/us
>> StableBenchmark.dcl   thrpt   10225.710 ?   
>> 26.638  ops/us
>> StableBenchmark.stablethrpt   10   4382.478 ? 
>> 1151.472  ops/us <- StableValue significantly faster
>> 
>> 
>> Performance compared to static variables protected by `AtomicReference`, 
>> class-holder idiom holder, and double-checked locking (all threads):
>> 
>> 
>> Benchmark  Mode  Cnt  Score  
>> Error   Units
>> StableStaticBenchmark.atomic  thrpt   10   6487.835 ?  
>> 385.639  ops/us
>> StableStaticBenchmark.dcl thrpt   10   6605.239 ?  
>> 210.610  ops/us
>> StableStaticBenchmark.stable  thrpt   10  14338.239 ? 
>> 1426.874  ops/us
>> StableStaticBenchmark.staticCHI   thrpt   10  13780.341 ? 
>> 1839.651  ops/us
>> 
>> 
>> Performance for stable lists (thread safe) in both instance and static 
>> contexts whereby we access a single value compared to `ArrayList` instances 
>> (which are not thread-safe) (all threads):
>> 
>> 
>> Benchmark  Mode  Cnt  Score  
>> Error   Units
>> StableListElementBenchmark.instanceArrayList  thrpt   10   5812.992 ? 
>> 1169.730  ops/us
>> StableListElementBenchmark.instanceList   thrpt   10   4818.643 ?  
>> 704.893  ops/us
>> StableListElementBenchmark...
>
> Per Minborg has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Switch to monomorphic StableValue and use lazy arrays

src/java.base/share/classes/jdk/internal/lang/stable/StableUtil.java line 152:

> 150: StableValueImpl witness = (StableValueImpl)
> 151: Holder.UNSAFE.compareAndExchangeReference(elements, 
> offset, null, stable);
> 152: return witness == null ? stable: witness;

Suggestion:

return witness == null ? stable : witness;

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1602094801

Integrated: 8331987: Enhance stacktrace clarity for CompletableFuture CancellationException

[Viktor Klang]

On Mon, 13 May 2024 16:41:37 GMT, Viktor Klang  wrote:

> This change adds wrapping of the CancellationException produced by 
> CompletableFuture::get() and CompletableFuture::join() to add more diagnostic 
> information and align better with FutureTask.
> 
> Running the sample code from the JBS issue in JShell will produce the 
> following:
> 
> 
> jshell> java.util.concurrent.CancellationException: get
>   at 
> java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:392)
>   at 
> java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2073)
>   at REPL.$JShell$18.m2($JShell$18.java:10)
>   at REPL.$JShell$17.m1($JShell$17.java:8)
>   at REPL.$JShell$16B.lambda$main$0($JShell$16B.java:8)
>   at java.base/java.lang.Thread.run(Thread.java:1575)
> Caused by: java.util.concurrent.CancellationException
>   at 
> java.base/java.util.concurrent.CompletableFuture.cancel(CompletableFuture.java:2510)
>   at REPL.$JShell$16B.lambda$main$1($JShell$16B.java:11)
>   ... 1 more

This pull request has now been integrated.

Changeset: 8a4315f8
Author:Viktor Klang 
URL:   
https://git.openjdk.org/jdk/commit/8a4315f833f3700075d65fae6bc566011c837c07
Stats: 35 lines in 3 files changed: 25 ins; 0 del; 10 mod

8331987: Enhance stacktrace clarity for CompletableFuture CancellationException

Reviewed-by: alanb, dfuchs

-

PR: https://git.openjdk.org/jdk/pull/19219

Re: RFR: 8330276: Console methods with explicit Locale [v7]

[Jan Lahoda]

On Wed, 15 May 2024 17:05:17 GMT, Naoto Sato  wrote:

>> Proposing new overloaded methods in `java.io.Console` class that explicitly 
>> take a `Locale` argument. Existing methods rely on the default locale, so 
>> the users won't be able to format their prompts/outputs in a certain locale 
>> explicitly.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Reverted test workaround. Fixed JLine (backing out a questionable change)

Looks good to me.

-

Marked as reviewed by jlahoda (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18923#pullrequestreview-2058627699

Re: RFR: 8330276: Console methods with explicit Locale [v7]

[Naoto Sato]

> Proposing new overloaded methods in `java.io.Console` class that explicitly 
> take a `Locale` argument. Existing methods rely on the default locale, so the 
> users won't be able to format their prompts/outputs in a certain locale 
> explicitly.

Naoto Sato has updated the pull request incrementally with one additional 
commit since the last revision:

  Reverted test workaround. Fixed JLine (backing out a questionable change)

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18923/files
  - new: https://git.openjdk.org/jdk/pull/18923/files/cace6171..7833da36

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18923&range=06
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18923&range=05-06

  Stats: 9 lines in 2 files changed: 2 ins; 5 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/18923.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18923/head:pull/18923

PR: https://git.openjdk.org/jdk/pull/18923

Re: RFR: 8330465: Stable Values and Collections (Internal) [v5]

[Chen Liang]

On Wed, 15 May 2024 15:27:34 GMT, Per Minborg  wrote:

>> # Stable Values & Collections (Internal)
>> 
>> ## Summary
>> This PR proposes to introduce an internal _Stable Values & Collections_ API, 
>> which provides immutable value holders where elements are initialized _at 
>> most once_. Stable Values & Collections offer the performance and safety 
>> benefits of final fields while offering greater flexibility as to the timing 
>> of initialization.
>> 
>> ## Goals
>>  * Provide an easy and intuitive API to describe value holders that can 
>> change at most once.
>>  * Decouple declaration from initialization without significant footprint or 
>> performance penalties.
>>  * Reduce the amount of static initializer and/or field initialization code.
>>  * Uphold integrity and consistency, even in a multi-threaded environment.
>>  
>> For more details, see the draft JEP: https://openjdk.org/jeps/8312611
>> 
>> ## Performance 
>> Performance compared to instance variables using two `AtomicReference` and 
>> two protected by double-checked locking under concurrent access by all 
>> threads:
>> 
>> 
>> Benchmark  Mode  Cnt  Score  
>> Error   Units
>> StableBenchmark.atomicthrpt   10259.478 ?   
>> 36.809  ops/us
>> StableBenchmark.dcl   thrpt   10225.710 ?   
>> 26.638  ops/us
>> StableBenchmark.stablethrpt   10   4382.478 ? 
>> 1151.472  ops/us <- StableValue significantly faster
>> 
>> 
>> Performance compared to static variables protected by `AtomicReference`, 
>> class-holder idiom holder, and double-checked locking (all threads):
>> 
>> 
>> Benchmark  Mode  Cnt  Score  
>> Error   Units
>> StableStaticBenchmark.atomic  thrpt   10   6487.835 ?  
>> 385.639  ops/us
>> StableStaticBenchmark.dcl thrpt   10   6605.239 ?  
>> 210.610  ops/us
>> StableStaticBenchmark.stable  thrpt   10  14338.239 ? 
>> 1426.874  ops/us
>> StableStaticBenchmark.staticCHI   thrpt   10  13780.341 ? 
>> 1839.651  ops/us
>> 
>> 
>> Performance for stable lists (thread safe) in both instance and static 
>> contexts whereby we access a single value compared to `ArrayList` instances 
>> (which are not thread-safe) (all threads):
>> 
>> 
>> Benchmark  Mode  Cnt  Score  
>> Error   Units
>> StableListElementBenchmark.instanceArrayList  thrpt   10   5812.992 ? 
>> 1169.730  ops/us
>> StableListElementBenchmark.instanceList   thrpt   10   4818.643 ?  
>> 704.893  ops/us
>> StableListElementBenchmark...
>
> Per Minborg has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Switch to monomorphic StableValue and use lazy arrays

src/java.base/share/classes/jdk/internal/lang/StableValue.java line 384:

> 382:  * @param   the memoized type
> 383:  */
> 384: static  Supplier ofSupplier(Supplier original) {

`ofSupplier` sounds like this method returns a `StableValue` from a `Supplier`. 
I recommend another name, such as `stableSupplier`, `wrapSupplier`, or 
`memoize`, to better associate with the method's types.

src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line 
75:

> 73:  */
> 74: @Stable
> 75: private int state;

Can we change this to be a byte, so state and supplying fields can be packed 
together in 4 bytes in some layouts?

src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line 
104:

> 102: // Optimistically try plain semantics first
> 103: final V v = value;
> 104: if (v != null) {

If `value == null && state == NULL`, can the path still be constant folded? I 
doubt it because `value` in this case may not be promoted to constant.

src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line 
139:

> 137: case NON_NULL: { return valueVolatile(); }
> 138: case ERROR:{ throw StableUtil.error(this); }
> 139: case DUMMY:{ throw shouldNotReachHere(); }

Redundant branch?

src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line 
236:

> 234: } catch (Throwable t) {
> 235: putState(ERROR);
> 236: putMutex(t.getClass());

Should we cache the exception instance so we can rethrow it in future ERROR 
state `orThrow` calls?

src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line 
240:

> 238: }
> 239: } finally {
> 240: supplying = false;

Resetting a stable field is a bad idea. I recommend renaming this to 
`supplierCalled` or `supplied` so we never transition this false -> true

src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line 
256:

> 254: 
> 255: @ForceInline
> 256: private  V 

Re: RFR: 8330465: Stable Values and Collections (Internal) [v5]

[Chen Liang]

On Mon, 6 May 2024 19:31:43 GMT, Per Minborg  wrote:

>> src/java.base/share/classes/jdk/internal/lang/StableArray.java line 25:
>> 
>>> 23:  * @since 23
>>> 24:  */
>>> 25: public sealed interface StableArray
>> 
>> Do we have a use case for StableArray beyond those of StableList?
>
> I am trying to model multi-dimensional arrays that also provide flattening. 
> Let's see if it becomes useful.

I think this StableArray can be used as an explicit field type to block 
reflection modifications and enforce constant-folding; the List interface 
cannot. Though in long term I still believe strict final fields are better.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1601931752

Integrated: 8331940: ClassFile API ArrayIndexOutOfBoundsException with certain class files

[Adam Sotona]

On Tue, 14 May 2024 13:18:51 GMT, Adam Sotona  wrote:

> Class file with `LineNumberTable` attribute element pointing behind the 
> bytecode array throws `ArrayIndexOutOfBoundsException`.
> This patch performs the check and throws  expected `IllegalArgumentException` 
> instead.
> Relevant test is added.
> 
> Please review.
> 
> Thanks,
> Adam

This pull request has now been integrated.

Changeset: 42ccb743
Author:Adam Sotona 
URL:   
https://git.openjdk.org/jdk/commit/42ccb74399113a3d59ce016483518f033dd6e010
Stats: 20 lines in 2 files changed: 18 ins; 0 del; 2 mod

8331940: ClassFile API ArrayIndexOutOfBoundsException with certain class files

Reviewed-by: liach, psandoz

-

PR: https://git.openjdk.org/jdk/pull/19230

Re: RFR: 8330276: Console methods with explicit Locale [v6]

[Naoto Sato]

> Proposing new overloaded methods in `java.io.Console` class that explicitly 
> take a `Locale` argument. Existing methods rely on the default locale, so the 
> users won't be able to format their prompts/outputs in a certain locale 
> explicitly.

Naoto Sato has updated the pull request incrementally with one additional 
commit since the last revision:

  Adding workaround to the test due to JLine's questionable behavior

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18923/files
  - new: https://git.openjdk.org/jdk/pull/18923/files/6fd17574..cace6171

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18923&range=05
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18923&range=04-05

  Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/18923.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18923/head:pull/18923

PR: https://git.openjdk.org/jdk/pull/18923

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v6]

[Maurizio Cimadamore]

> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting 
> the use of JNI in the following ways:
> 
> * `System::load` and `System::loadLibrary` are now restricted methods
> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
> * binding a JNI `native` method declaration to a native implementation is now 
> considered a restricted operation
> 
> This PR slightly changes the way in which the JDK deals with restricted 
> methods, even for FFM API calls. In Java 22, the single 
> `--enable-native-access` was used both to specify a set of modules for which 
> native access should be allowed *and* to specify whether illegal native 
> access (that is, native access occurring from a module not specified by 
> `--enable-native-access`) should be treated as an error or a warning. More 
> specifically, an error is only issued if the `--enable-native-access flag` is 
> used at least once.
> 
> Here, a new flag is introduced, namely 
> `illegal-native-access=allow/warn/deny`, which is used to specify what should 
> happen when access to a restricted method and/or functionality is found 
> outside the set of modules specified with `--enable-native-access`. The 
> default policy is `warn`, but users can select `allow` to suppress the 
> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This 
> aligns the treatment of restricted methods with other mechanisms, such as 
> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
> 
> Some changes were required in the package-info javadoc for 
> `java.lang.foreign`, to reflect the changes in the command line flags 
> described above.

Maurizio Cimadamore has updated the pull request incrementally with one 
additional commit since the last revision:

  Address review comment

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19213/files
  - new: https://git.openjdk.org/jdk/pull/19213/files/daf729f4..1c45e5d5

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=05
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=04-05

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/19213.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19213/head:pull/19213

PR: https://git.openjdk.org/jdk/pull/19213

Re: RFR: 8331940: ClassFile API ArrayIndexOutOfBoundsException with certain class files [v2]

[Paul Sandoz]

On Wed, 15 May 2024 07:51:33 GMT, Adam Sotona  wrote:

>> Class file with `LineNumberTable` attribute element pointing behind the 
>> bytecode array throws `ArrayIndexOutOfBoundsException`.
>> This patch performs the check and throws  expected 
>> `IllegalArgumentException` instead.
>> Relevant test is added.
>> 
>> Please review.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fixed exception message

Marked as reviewed by psandoz (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19230#pullrequestreview-2058434044

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v5]

[Alan Bateman]

On Wed, 15 May 2024 10:40:34 GMT, Maurizio Cimadamore  
wrote:

>> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting 
>> the use of JNI in the following ways:
>> 
>> * `System::load` and `System::loadLibrary` are now restricted methods
>> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
>> * binding a JNI `native` method declaration to a native implementation is 
>> now considered a restricted operation
>> 
>> This PR slightly changes the way in which the JDK deals with restricted 
>> methods, even for FFM API calls. In Java 22, the single 
>> `--enable-native-access` was used both to specify a set of modules for which 
>> native access should be allowed *and* to specify whether illegal native 
>> access (that is, native access occurring from a module not specified by 
>> `--enable-native-access`) should be treated as an error or a warning. More 
>> specifically, an error is only issued if the `--enable-native-access flag` 
>> is used at least once.
>> 
>> Here, a new flag is introduced, namely 
>> `illegal-native-access=allow/warn/deny`, which is used to specify what 
>> should happen when access to a restricted method and/or functionality is 
>> found outside the set of modules specified with `--enable-native-access`. 
>> The default policy is `warn`, but users can select `allow` to suppress the 
>> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This 
>> aligns the treatment of restricted methods with other mechanisms, such as 
>> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
>> 
>> Some changes were required in the package-info javadoc for 
>> `java.lang.foreign`, to reflect the changes in the command line flags 
>> described above.
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Refine warning text for JNI method binding

src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line 871:

> 869: return IllegalNativeAccess.WARN;
> 870: } else {
> 871: fail("Value specified to --illegal-access not recognized:"

Typo in the message, should be --illegal-native-access.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1601898238

Re: RFR: 8330465: Stable Values and Collections (Internal) [v4]

[Chen Liang]

On Wed, 15 May 2024 15:20:58 GMT, Per Minborg  wrote:

>> At some point in the future, 'jdk.unsupported' will be removed
>
> Maybe there is a better home for this?

I don't think we should publish this API; this will soon be phased out by 
strict final fields (written only before super constructor calls) introduced by 
Valhalla, as strict final fields are never modifiable and can be safely trusted.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1601887921

Re: RFR: 8330465: Stable Values and Collections (Internal) [v5]

[Chen Liang]

On Tue, 14 May 2024 11:12:39 GMT, Per Minborg  wrote:

>> src/hotspot/share/ci/ciField.cpp line 262:
>> 
>>> 260: const char* stable_array3d_klass_name = 
>>> "jdk/internal/lang/StableArray3D";
>>> 261: 
>>> 262: static bool trust_final_non_static_fields_of_type(Symbol* signature) {
>> 
>> Is there a better way of doing this?
>
> How do we check if the type implements `TrustedFieldType` in C?

Is it possible for us to just look at strict fields from valhalla, so we can 
reliably constant-fold those strict final fields? 
https://cr.openjdk.org/~jrose/jls/constructive-classes.html

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1601876058

Re: RFR: 8330465: Stable Values and Collections (Internal) [v5]

[Per Minborg]

On Wed, 15 May 2024 15:27:34 GMT, Per Minborg  wrote:

>> # Stable Values & Collections (Internal)
>> 
>> ## Summary
>> This PR proposes to introduce an internal _Stable Values & Collections_ API, 
>> which provides immutable value holders where elements are initialized _at 
>> most once_. Stable Values & Collections offer the performance and safety 
>> benefits of final fields while offering greater flexibility as to the timing 
>> of initialization.
>> 
>> ## Goals
>>  * Provide an easy and intuitive API to describe value holders that can 
>> change at most once.
>>  * Decouple declaration from initialization without significant footprint or 
>> performance penalties.
>>  * Reduce the amount of static initializer and/or field initialization code.
>>  * Uphold integrity and consistency, even in a multi-threaded environment.
>>  
>> For more details, see the draft JEP: https://openjdk.org/jeps/8312611
>> 
>> ## Performance 
>> Performance compared to instance variables using two `AtomicReference` and 
>> two protected by double-checked locking under concurrent access by all 
>> threads:
>> 
>> 
>> Benchmark  Mode  Cnt  Score  
>> Error   Units
>> StableBenchmark.atomicthrpt   10259.478 ?   
>> 36.809  ops/us
>> StableBenchmark.dcl   thrpt   10225.710 ?   
>> 26.638  ops/us
>> StableBenchmark.stablethrpt   10   4382.478 ? 
>> 1151.472  ops/us <- StableValue significantly faster
>> 
>> 
>> Performance compared to static variables protected by `AtomicReference`, 
>> class-holder idiom holder, and double-checked locking (all threads):
>> 
>> 
>> Benchmark  Mode  Cnt  Score  
>> Error   Units
>> StableStaticBenchmark.atomic  thrpt   10   6487.835 ?  
>> 385.639  ops/us
>> StableStaticBenchmark.dcl thrpt   10   6605.239 ?  
>> 210.610  ops/us
>> StableStaticBenchmark.stable  thrpt   10  14338.239 ? 
>> 1426.874  ops/us
>> StableStaticBenchmark.staticCHI   thrpt   10  13780.341 ? 
>> 1839.651  ops/us
>> 
>> 
>> Performance for stable lists (thread safe) in both instance and static 
>> contexts whereby we access a single value compared to `ArrayList` instances 
>> (which are not thread-safe) (all threads):
>> 
>> 
>> Benchmark  Mode  Cnt  Score  
>> Error   Units
>> StableListElementBenchmark.instanceArrayList  thrpt   10   5812.992 ? 
>> 1169.730  ops/us
>> StableListElementBenchmark.instanceList   thrpt   10   4818.643 ?  
>> 704.893  ops/us
>> StableListElementBenchmark...
>
> Per Minborg has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Switch to monomorphic StableValue and use lazy arrays

I have reworked the stable collections so that we create StableValues on demand 
and store them in a lazily populated backing array. This improved performance 
significantly as well as gave us improved startup times (only one array needs 
to be created upfront). Also, StableValue is now monomorphic. On the flip side 
is the fact that slightly more memory is needed.

-

PR Comment: https://git.openjdk.org/jdk/pull/18794#issuecomment-2112886060

Re: RFR: 8330465: Stable Values and Collections (Internal) [v4]

[Per Minborg]

On Wed, 15 May 2024 08:53:32 GMT, ExE Boss  wrote:

>> Per Minborg has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Revise docs for ofBackground()
>
> src/java.base/share/classes/jdk/internal/lang/StableValue.java line 1:
> 
>> 1: /*
> 
> Maybe also add `StableValue​::⁠ofLazy​(Supplier)` which 
> behaves more like the original **Computed Constants** JEP draft?

There is a method `StableValue::asSupplier` that is similar to the former 
ComputedConstant behavior.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1601840547

Re: RFR: 8330465: Stable Values and Collections (Internal) [v5]

[Per Minborg]

> # Stable Values & Collections (Internal)
> 
> ## Summary
> This PR proposes to introduce an internal _Stable Values & Collections_ API, 
> which provides immutable value holders where elements are initialized _at 
> most once_. Stable Values & Collections offer the performance and safety 
> benefits of final fields while offering greater flexibility as to the timing 
> of initialization.
> 
> ## Goals
>  * Provide an easy and intuitive API to describe value holders that can 
> change at most once.
>  * Decouple declaration from initialization without significant footprint or 
> performance penalties.
>  * Reduce the amount of static initializer and/or field initialization code.
>  * Uphold integrity and consistency, even in a multi-threaded environment.
>  
> For more details, see the draft JEP: https://openjdk.org/jeps/8312611
> 
> ## Performance 
> Performance compared to instance variables using an `AtomicReference` and one 
> protected by double-checked locking under concurrent access by 8 threads:
> 
> 
> Benchmark   Mode  Cnt  Score   Error  Units
> StableBenchmark.instanceAtomic  avgt   10  1.576 ? 0.052  ns/op
> StableBenchmark.instanceDCL avgt   10  1.608 ? 0.059  ns/op
> StableBenchmark.instanceStable  avgt   10  0.979 ? 0.023  ns/op <- 
> StableValue (~40% faster than DCL)
> 
> 
> Performance compared to static variables protected by `AtomicReference`, 
> class-holder idiom holder, and double-checked locking (8 threads):
> 
> 
> Benchmark   Mode  Cnt  Score   Error  Units
> StableBenchmark.staticAtomicavgt   10  1.335 ? 0.056  ns/op
> StableBenchmark.staticCHI   avgt   10  0.623 ? 0.086  ns/op
> StableBenchmark.staticDCL   avgt   10  1.418 ? 0.171  ns/op
> StableBenchmark.staticList  avgt   10  0.617 ? 0.024  ns/op
> StableBenchmark.staticStableavgt   10  0.604 ? 0.022  ns/op <- 
> StableValue ( > 2x faster than `AtomicInteger` and DCL)
> 
> 
> Performance for stable lists in both instance and static contexts whereby the 
> sum of random contents is calculated for stable lists (which are thread-safe) 
> compared to `ArrayList` instances (which are not thread-safe) (under single 
> thread access):
> 
> 
> Benchmark Mode  Cnt  Score   Error  Units
> StableListSumBenchmark.instanceArrayList  avgt   10  0.356 ? 0.005  ns/op
> StableListSumBenchmark.instanceList   avgt   10  0.373 ? 0.017  ns/op 
> <- Stable list
> StableListSumBenchmark.staticArrayListavgt   10  0.352 ? 0.002  ns/op
> StableListSumBenchmark.staticList avgt   10  0.356 ? 0.00...

Per Minborg has updated the pull request incrementally with one additional 
commit since the last revision:

  Switch to monomorphic StableValue and use lazy arrays

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18794/files
  - new: https://git.openjdk.org/jdk/pull/18794/files/c92b16c4..2b840e06

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18794&range=04
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18794&range=03-04

  Stats: 471 lines in 9 files changed: 126 ins; 306 del; 39 mod
  Patch: https://git.openjdk.org/jdk/pull/18794.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18794/head:pull/18794

PR: https://git.openjdk.org/jdk/pull/18794

Re: RFR: 8330465: Stable Values and Collections (Internal) [v4]

[Per Minborg]

On Wed, 15 May 2024 12:26:34 GMT, Rémi Forax  wrote:

>> Given that `TrustedFieldType` is more generic than stable values, it could 
>> be moved to `jdk.internal.misc` or `jdk.internal.reflect`, then 
>> `jdk.unsupported` could use it without exporting new packages to 
>> `jdk.unsupported`.
>
> At some point in the future, 'jdk.unsupported' will be removed

Maybe there is a better home for this?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1601843499

Integrated: 8332003: Clarify javadoc for MemoryLayout::offsetHandle

[Maurizio Cimadamore]

On Thu, 9 May 2024 15:00:35 GMT, Maurizio Cimadamore  
wrote:

> Consider this layout:
> 
> 
> MemoryLayout SEQ = MemoryLayout.sequenceLayout(5,
>  MemoryLayout.sequenceLayout(10, JAVA_INT));
> 
> 
> And the corresponding offset handle:
> 
> 
> MethodHandle offsetHandle = SEQ.offsetHandle(PathElement.sequenceLayout(), 
> PathElement.sequenceLayout());
> 
> 
> The resulting method handle takes two additional `long` indices. The 
> implementation checks that the dynamically provided indices conform to the 
> bound available at construction: that is, the first index must be < 5, while 
> the second must be < 10. If this is not true, an `IndexOutOfBoundException` 
> is thrown.
> 
> However, the javadoc for `MemoryLayout::byteOffsetHandle` doesn't claim 
> anything in this direction. There are only some vague claims in the javadoc 
> for `PathElement::sequenceElement()` and `PathElement::sequenceElement(long, 
> long, long)` which make some claims on which indices are actually allowed, 
> but the text seems more in the tone of a discussion, rather than actual 
> normative text.
> 
> I've tweaked the javadoc for `MemoryLayout::byteOffsetHandle` to actually 
> state that the indices will be checked against the "size" of the 
> corresponding open path element (this is a new concept that I also have 
> defined in the javadoc).
> 
> I also added a test for `byteOffsetHandle` as I don't think we had a test for 
> that specifically (although this method is tested indirectly, via 
> `MemoryLayout::varHandle`).

This pull request has now been integrated.

Changeset: 30bb066b
Author:Maurizio Cimadamore 
URL:   
https://git.openjdk.org/jdk/commit/30bb066b1982c5318d54bfe74115306c602e2974
Stats: 67 lines in 2 files changed: 61 ins; 0 del; 6 mod

8332003: Clarify javadoc for MemoryLayout::offsetHandle

Reviewed-by: psandoz

-

PR: https://git.openjdk.org/jdk/pull/19158

Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v13]

[Claes Redestad]

On Wed, 15 May 2024 11:17:42 GMT, Claes Redestad  wrote:

>> This PR suggests refactoring the implementation classes of 
>> java.lang.constant into a new package jdk.internal.constant to enable 
>> sharing some trusted static factory methods with users elsewhere in 
>> java.base, such as java.lang.invoke and java.lang.classfile. The refactoring 
>> also adds some "trusted" methods for use when input is pre-validated, and 
>> makes use of them where applicable in the current implementation. There are 
>> more changes in the pipeline which will be folded into #17108 or PR'ed once 
>> that is integrated. 
>> 
>> There are two optimizations mixed up here. One in 
>> `MethodTypeDesc.ofDescriptor`:
>> 
>> Name 
>> (descString) Cnt Base Error  Test   Error  Unit  Change
>> MethodTypeDescFactories.ofDescriptor  
>> (Ljava/lang/Object;Ljava/lang/String;)I   6  138,371 ±   0,767   136,939 ± 
>> 1,126 ns/op   1,01x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor  
>> ()V   6   10,528 ±   2,495 4,110 ± 0,047 ns/op   2,56x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor 
>> ([IJLjava/lang/String;Z)Ljava/util/List;   6  209,390 ±   4,583   196,175 ± 
>> 3,211 ns/op   1,07x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ()[Ljava/lang/String;   6   36,039 ±   8,68420,794 ± 1,110 ns/op   1,73x 
>> (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor 
>> (..IIJ)V   6  279,139 ±   6,248   187,934 ± 0,857 ns/op   1,49x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor 
>> (.).   6 2174,387 ± 132,879  1150,652 ± 3,158 ns/op   
>> 1,89x (p = 0,000*)
>>   * = significant
>> 
>> 
>> The other in `ClassDesc::nested`, where to get rid of a simple static method 
>> in `ConstantUtils` I ended up speeding up and simplifying the public factory 
>> method:
>> 
>> Name  CntBase ErrorTest   
>> Error  Unit  Change
>> ClassDescFactories.ReferenceOnly.ofNested   6 209,853 ± 132,525  22,017 ± 
>> 0,573 ns/op   9,53x (p = 0,000*)
>>   * = significant
>> 
>> 
>> The optimizations could be split out and PRd independently, but I think they 
>> are simple enough to include in this refactoring.
>
> Claes Redestad has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Update src/java.base/share/classes/java/lang/constant/ClassDesc.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>  - Update src/java.base/share/classes/java/lang/constant/ClassDesc.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>

FTR I had to back out a change that would delegate to the now simplified and 
optimized `arrayType()` from `arrayType(int rank)` when `rank` equals `1` since 
the methods actually throw different exceptions, `IllegalStateException` vs 
`IllegalArgumentException`. This was caught by a later test. 

Unfortunate that `java.lang.constant` has chosen to use these two in particular 
since it's easy to miss due to the similarity of the exception names. There are 
also some unspecified exceptional behavior. For example on `arrayType()` only 
`IllegalStateException` is specified to be thrown, but calling 
`ClassDesc.ofDescriptor("V").arrayType()` throws an `IllegalArgumentException`. 
This behavior is not changed by this PR.

-

PR Comment: https://git.openjdk.org/jdk/pull/19105#issuecomment-2112663111

Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis [v2]

[Magnus Ihse Bursie]

On Thu, 9 May 2024 07:50:00 GMT, Julian Waters  wrote:

>> WIP
>> 
>> This changeset contains hsdis for Windows/gcc Port. It supports both the 
>> binutils and capstone backends, though the LLVM backend is left out due to 
>> compatibility issues encountered during the build. Currently, which gcc 
>> distributions are supported is still to be clarified, as several, ranging 
>> from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, 
>> the build system hack in place at the moment to compile the binutils backend 
>> on Windows is still left in place, for now.
>
> Julian Waters has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains two commits:
> 
>  - Add check for compiler in configure
>  - 8330988: Implementation of 8288293: Windows/gcc Port for hsdis

Hi Julian, sorry for not getting back to you.

The problem from my PoV is that this is a very large and intrusive patch in the 
build of the actual product, for a claimed problem in the tangential hsdis 
library. I have not understood a pressing business need to get a Windows/gcc 
port for hsdis, which means I can't really prioritize trying to understand this 
patch.

As you know, the build system does not currently really separate between "the 
OS is Windows" and "the toolchain is Microsoft". I understand that you want to 
fix that, and in theory I support it. However, you must also realize that 
making a complete fix of this requires a lot of work, for something that there 
is no clearly defined need. (After all, cl.exe works fine to create the build, 
has no apparent issues, and is as far as an "official" compiler for Windows as 
you can get.) That makes it fall squarely in the WIBNIs bucked ("wouldn't it be 
nice if...").

If you can fix just the hsdis build without changing anything outside the hsdis 
Makefiles, that would be a different story. Such a change would be limited in 
scope, easy to say it will not affect the product proper, and be easier to 
review.

-

PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2112546029

Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v28]

[Magnus Ihse Bursie]

On Wed, 15 May 2024 11:55:39 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 with a new target base due to a 
> merge or a rebase. The pull request now contains 108 commits:
> 
>  - Merge branch 'master' into jdk-8311302-jmodless-link
>  - Simplify JLINK_JDK_EXTRA_OPTS var handling
>  - Only add runtime track files for linkable runtimes
>  - Generate the runtime image link diff at jlink time
>
>But only do that once the plugin-pipeline has run. The generation is
>enabled with the hidden option '--generate-linkable-runtime' and
>the resource pools available at jlink time are being used to generate
>the diff.
>  - Merge branch 'master' into jdk-8311302-jmodless-link
>  - Use shorter name for the build tool
>  - Review feedback from Erik J.
>  - Remove dependency on CDS which isn't needed anymore
>  - Fix coment
>  - Fix comment
>  - ... and 98 more: https://git.openjdk.org/jdk/compare/957eb611...8cca277e

Thanks! Now I am 100% happy with the build changes. :)

-

Marked as reviewed by ihse (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14787#pullrequestreview-2057984229

Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis [v2]

[Julian Waters]

On Thu, 9 May 2024 07:50:00 GMT, Julian Waters  wrote:

>> WIP
>> 
>> This changeset contains hsdis for Windows/gcc Port. It supports both the 
>> binutils and capstone backends, though the LLVM backend is left out due to 
>> compatibility issues encountered during the build. Currently, which gcc 
>> distributions are supported is still to be clarified, as several, ranging 
>> from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, 
>> the build system hack in place at the moment to compile the binutils backend 
>> on Windows is still left in place, for now.
>
> Julian Waters has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains two commits:
> 
>  - Add check for compiler in configure
>  - 8330988: Implementation of 8288293: Windows/gcc Port for hsdis

Magnus? Erik? You guys there? :(

-

PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2112465392

Re: RFR: 8331670: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal

[Alan Bateman]

On Wed, 15 May 2024 10:18:11 GMT, Maurizio Cimadamore  
wrote:

> The FFM code throws if an unknown value is passed. Here we log. Should we try 
> to be more consistent?

I don't have a strong opinion on this. The value of --illegal-native-access is 
examined during startup so startup can fail if a bad value is specified. Here 
it is mid-flight when Unsafe initialises. The coin toss landed on printing a 
warning but it could equally fail. I jus didn't want code executing during 
startup to check configuration for something faraway in the jdk.unsupported 
module.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19174#discussion_r1601582672

Re: RFR: 8330465: Stable Values and Collections (Internal) [v4]

[Rémi Forax]

On Wed, 15 May 2024 11:27:04 GMT, ExE Boss  wrote:

>>> Maybe export this interface to `jdk.unsupported`?
>> 
>> I don't we should do that. In general, we need jdk.unsupported to go away in 
>> the long term. Also integrity of the platform depends on java.base being 
>> very stingy and not exporting internal packages to other modules.
>
> Given that `TrustedFieldType` is more generic than stable values, it could be 
> moved to `jdk.internal.misc` or `jdk.internal.reflect`, then 
> `jdk.unsupported` could use it without exporting new packages to 
> `jdk.unsupported`.

At some point in the future, 'jdk.unsupported' will be removed

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1601548492

Re: RFR: 8329653: JLILaunchTest fails on AIX after JDK-8329131 [v5]

[Joachim Kern]

> Since ~ end of March, after 
> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), 
> tools/launcher/JliLaunchTest.java fails on AIX. Failure is :
> 
>  stdout: [];
>  stderr: [Error: could not find libjava.so
> Error: Could not find Java SE Runtime Environment.
> ]
>  exitValue = 2
> 
> java.lang.RuntimeException: Expected to get exit value of [0], exit value is: 
> [2]
> at 
> jdk.test.lib.process.OutputAnalyzer.shouldHaveExitValue(OutputAnalyzer.java:521)
> at JliLaunchTest.main(JliLaunchTest.java:58)
> at 
> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
> at java.base/java.lang.reflect.Method.invoke(Method.java:580)
> at 
> com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333)
> at java.base/java.lang.Thread.run(Thread.java:1575)
> 
> Maybe we need to do further adjustments to 
> BUILD_JDK_JTREG_EXECUTABLES_LIBS_exeJliLaunchTest / 
> BUILD_JDK_JTREG_EXECUTABLES_LDFLAGS_exeJliLaunchTest on AIX ?
> Or somehow adjust the coding that attempts to find parts of "JRE" 
> (libjava.so) ? The libjli.so is gone on AIX after 
> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), and with this 
> also the image discovery based on GetApplicationHomeFromDll which uses 
> libjli.so .
> 
> Without libjli.so we have to analyze the LD-LIBRARY_PATH / LIBPATH envvar. 
> There is no other methos available on AIX, because it lacks the $ORIGIN 
> feature of the rpath.

Joachim Kern has updated the pull request incrementally with one additional 
commit since the last revision:

  cosmetic changes

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19000/files
  - new: https://git.openjdk.org/jdk/pull/19000/files/ba3abf02..0c8c22ce

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=19000&range=04
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19000&range=03-04

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/19000.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19000/head:pull/19000

PR: https://git.openjdk.org/jdk/pull/19000

Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v28]

[Severin Gehwolf]

> 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 b
 eing 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.internal.vm.compiler.manage...

Severin Gehwolf has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains 108 commits:

 - Merge branch 'master' into jdk-8311302-jmodless-link
 - Simplify JLINK_JDK_EXTRA_OPTS var handling
 - Only add runtime track files for linkable runtimes
 - Generate the runtime image link diff at jlink time
   
   But only do that once the plugin-pipeline has run. The generation is
   enabled with the hidden option '--generate-linkable-runtime' and
   the resource pools available at jlink time are being used to generate
   the diff.
 - Merge branch 'master' into jdk-8311302-jmodless-link
 - Use shorter name for the build tool
 - Review feedback from Erik J.
 - Remove dependency on CDS which isn't needed anymore
 - Fix coment
 - Fix comment
 - ... and 98 more: https://git.openjdk.org/jdk/compare/957eb611...8cca277e

-

Changes: https://git.openjdk.org/jdk/pull/14787/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14787&range=27
  Stats: 3424 lines in 36 files changed: 3272 ins; 110 del; 42 mod
  Patch: https://git.openjdk.org/jdk/pull/14787.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14787/head:pull/14787

PR: https://git.openjdk.org/jdk/pull/14787

Re: RFR: 8329653: JLILaunchTest fails on AIX after JDK-8329131 [v4]

[Christoph Langer]

On Wed, 15 May 2024 11:40:38 GMT, Joachim Kern  wrote:

>> Since ~ end of March, after 
>> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), 
>> tools/launcher/JliLaunchTest.java fails on AIX. Failure is :
>> 
>>  stdout: [];
>>  stderr: [Error: could not find libjava.so
>> Error: Could not find Java SE Runtime Environment.
>> ]
>>  exitValue = 2
>> 
>> java.lang.RuntimeException: Expected to get exit value of [0], exit value 
>> is: [2]
>> at 
>> jdk.test.lib.process.OutputAnalyzer.shouldHaveExitValue(OutputAnalyzer.java:521)
>> at JliLaunchTest.main(JliLaunchTest.java:58)
>> at 
>> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
>> at java.base/java.lang.reflect.Method.invoke(Method.java:580)
>> at 
>> com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333)
>> at java.base/java.lang.Thread.run(Thread.java:1575)
>> 
>> Maybe we need to do further adjustments to 
>> BUILD_JDK_JTREG_EXECUTABLES_LIBS_exeJliLaunchTest / 
>> BUILD_JDK_JTREG_EXECUTABLES_LDFLAGS_exeJliLaunchTest on AIX ?
>> Or somehow adjust the coding that attempts to find parts of "JRE" 
>> (libjava.so) ? The libjli.so is gone on AIX after 
>> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), and with this 
>> also the image discovery based on GetApplicationHomeFromDll which uses 
>> libjli.so .
>> 
>> Without libjli.so we have to analyze the LD-LIBRARY_PATH / LIBPATH envvar. 
>> There is no other methos available on AIX, because it lacks the $ORIGIN 
>> feature of the rpath.
>
> Joachim Kern has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   cosmetic changes

src/java.base/unix/native/libjli/java_md_common.c line 128:

> 126: /*
> 127:  * Retrieves the path to the JRE home by locating libjava.so in
> 128:  * one of the LIBPATH and then truncating the path to it.

Suggestion:

 * LIBPATH and then truncating the path to it.


Another small comment suggestion

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19000#discussion_r1601491722

Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v9]

[Chen Liang]

On Wed, 15 May 2024 07:16:37 GMT, Claes Redestad  wrote:

>> src/java.base/share/classes/java/lang/constant/ClassDesc.java line 226:
>> 
>>> 224: for (int i = 0; i < rank; i++) {
>>> 225: sb.append('[');
>>> 226: }
>> 
>> Would `sb.repeat("[", rank);` be better here?
>> 
>> Also for parity with `arrayType()` I recommend moving this building code to 
>> after the void check.
>
> Oh, that's new (in 21). I was comparing against `append("[".repeat(rank))` 
> which had a cost here. `sb.repeat('[', rank)` is in the same ballpark and 
> simpler. Fixing.
> 
> 
> Benchmark   Mode  Cnt Score Error 
>   Units
> ClassDescMethods.arrayType2 avgt   1531,568 ±   2,108 
>   ns/op
> ClassDescMethods.arrayType2:gc.alloc.rate.norm  avgt   15   256,000 ±   0,001 
>B/op
> 
> 
> I'll align the code to construct `newDesc` before the void check in both 
> methods.

Didn't realize the char version actually takes an int codepoint 😄 so I 
suggested the String one.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1601477998

Re: RFR: 8329653: JLILaunchTest fails on AIX after JDK-8329131 [v4]

[Joachim Kern]

> Since ~ end of March, after 
> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), 
> tools/launcher/JliLaunchTest.java fails on AIX. Failure is :
> 
>  stdout: [];
>  stderr: [Error: could not find libjava.so
> Error: Could not find Java SE Runtime Environment.
> ]
>  exitValue = 2
> 
> java.lang.RuntimeException: Expected to get exit value of [0], exit value is: 
> [2]
> at 
> jdk.test.lib.process.OutputAnalyzer.shouldHaveExitValue(OutputAnalyzer.java:521)
> at JliLaunchTest.main(JliLaunchTest.java:58)
> at 
> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
> at java.base/java.lang.reflect.Method.invoke(Method.java:580)
> at 
> com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333)
> at java.base/java.lang.Thread.run(Thread.java:1575)
> 
> Maybe we need to do further adjustments to 
> BUILD_JDK_JTREG_EXECUTABLES_LIBS_exeJliLaunchTest / 
> BUILD_JDK_JTREG_EXECUTABLES_LDFLAGS_exeJliLaunchTest on AIX ?
> Or somehow adjust the coding that attempts to find parts of "JRE" 
> (libjava.so) ? The libjli.so is gone on AIX after 
> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), and with this 
> also the image discovery based on GetApplicationHomeFromDll which uses 
> libjli.so .
> 
> Without libjli.so we have to analyze the LD-LIBRARY_PATH / LIBPATH envvar. 
> There is no other methos available on AIX, because it lacks the $ORIGIN 
> feature of the rpath.

Joachim Kern has updated the pull request incrementally with one additional 
commit since the last revision:

  cosmetic changes

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19000/files
  - new: https://git.openjdk.org/jdk/pull/19000/files/5890bca3..ba3abf02

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=19000&range=03
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19000&range=02-03

  Stats: 7 lines in 1 file changed: 0 ins; 5 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/19000.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19000/head:pull/19000

PR: https://git.openjdk.org/jdk/pull/19000

Re: RFR: 8320396: Class-File API ClassModel::verify should include checks from hotspot/share/classfile/classFileParser.cpp [v7]

[Chen Liang]

On Wed, 15 May 2024 10:47:23 GMT, Adam Sotona  wrote:

>> ClassFile API `jdk.internal.classfile.verifier.VerifierImpl` performed only 
>> bytecode-level class verification.
>> This patch adds `jdk.internal.classfile.verifier.ParserVerifier` with 
>> additional class checks inspired by 
>> `hotspot/share/classfile/classFileParser.cpp`.
>> 
>> Also new `VerifierSelfTest::testParserVerifier` has been added.
>> 
>> Please review.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   applied the suggested changes

src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
 line 308:

> 306: 0;
> 307: default ->
> 308: -1;

I recommend we explicitly return -1 to skip verification only for 
UnknownAttribute and CustomAttribute; then our tests can catch missing 
verification for new attribute additions.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1601464598

Re: RFR: 8330465: Stable Values and Collections (Internal) [v4]

[ExE Boss]

On Wed, 15 May 2024 10:43:52 GMT, Alan Bateman  wrote:

>> src/java.base/share/classes/jdk/internal/lang/stable/TrustedFieldType.java 
>> line 14:
>> 
>>> 12:  * operations.
>>> 13:  */
>>> 14: public sealed interface TrustedFieldType
>> 
>> Maybe export this interface to `jdk.unsupported`?
>
>> Maybe export this interface to `jdk.unsupported`?
> 
> I don't we should do that. In general, we need jdk.unsupported to go away in 
> the long term. Also integrity of the platform depends on java.base being very 
> stingy and not exporting internal packages to other modules.

Given that `TrustedFieldType` is more generic than stable values, it could be 
moved to `jdk.internal.misc` or `jdk.internal.reflect`, then `jdk.unsupported` 
could use it without exporting new packages to `jdk.unsupported`.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1601455889

Re: RFR: 8331940: ClassFile API ArrayIndexOutOfBoundsException with certain class files [v2]

[Chen Liang]

On Wed, 15 May 2024 07:51:33 GMT, Adam Sotona  wrote:

>> Class file with `LineNumberTable` attribute element pointing behind the 
>> bytecode array throws `ArrayIndexOutOfBoundsException`.
>> This patch performs the check and throws  expected 
>> `IllegalArgumentException` instead.
>> Relevant test is added.
>> 
>> Please review.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fixed exception message

Marked as reviewed by liach (Author).

-

PR Review: https://git.openjdk.org/jdk/pull/19230#pullrequestreview-2057703566

Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v13]

[Claes Redestad]

> This PR suggests refactoring the implementation classes of java.lang.constant 
> into a new package jdk.internal.constant to enable sharing some trusted 
> static factory methods with users elsewhere in java.base, such as 
> java.lang.invoke and java.lang.classfile. The refactoring also adds some 
> "trusted" methods for use when input is pre-validated, and makes use of them 
> where applicable in the current implementation. There are more changes in the 
> pipeline which will be folded into #17108 or PR'ed once that is integrated. 
> 
> There are two optimizations mixed up here. One in 
> `MethodTypeDesc.ofDescriptor`:
> 
> Name (descString) 
> Cnt Base Error  Test   Error  Unit  Change
> MethodTypeDescFactories.ofDescriptor  (Ljava/lang/Object;Ljava/lang/String;)I 
>   6  138,371 ±   0,767   136,939 ± 1,126 ns/op   1,01x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor  ()V 
>   6   10,528 ±   2,495 4,110 ± 0,047 ns/op   2,56x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List; 
>   6  209,390 ±   4,583   196,175 ± 3,211 ns/op   1,07x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor()[Ljava/lang/String; 
>   6   36,039 ±   8,68420,794 ± 1,110 ns/op   1,73x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor (..IIJ)V 
>   6  279,139 ±   6,248   187,934 ± 0,857 ns/op   1,49x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor (.). 
>   6 2174,387 ± 132,879  1150,652 ± 3,158 ns/op   1,89x (p = 0,000*)
>   * = significant
> 
> 
> The other in `ClassDesc::nested`, where to get rid of a simple static method 
> in `ConstantUtils` I ended up speeding up and simplifying the public factory 
> method:
> 
> Name  CntBase ErrorTest   
> Error  Unit  Change
> ClassDescFactories.ReferenceOnly.ofNested   6 209,853 ± 132,525  22,017 ± 
> 0,573 ns/op   9,53x (p = 0,000*)
>   * = significant
> 
> 
> The optimizations could be split out and PRd independently, but I think they 
> are simple enough to include in this refactoring.

Claes Redestad has updated the pull request incrementally with two additional 
commits since the last revision:

 - Update src/java.base/share/classes/java/lang/constant/ClassDesc.java
   
   Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
 - Update src/java.base/share/classes/java/lang/constant/ClassDesc.java
   
   Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19105/files
  - new: https://git.openjdk.org/jdk/pull/19105/files/2a1b9ac9..273bcecc

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=19105&range=12
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19105&range=11-12

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/19105.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19105/head:pull/19105

PR: https://git.openjdk.org/jdk/pull/19105

Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v12]

[ExE Boss]

On Wed, 15 May 2024 10:55:31 GMT, Claes Redestad  wrote:

>> This PR suggests refactoring the implementation classes of 
>> java.lang.constant into a new package jdk.internal.constant to enable 
>> sharing some trusted static factory methods with users elsewhere in 
>> java.base, such as java.lang.invoke and java.lang.classfile. The refactoring 
>> also adds some "trusted" methods for use when input is pre-validated, and 
>> makes use of them where applicable in the current implementation. There are 
>> more changes in the pipeline which will be folded into #17108 or PR'ed once 
>> that is integrated. 
>> 
>> There are two optimizations mixed up here. One in 
>> `MethodTypeDesc.ofDescriptor`:
>> 
>> Name 
>> (descString) Cnt Base Error  Test   Error  Unit  Change
>> MethodTypeDescFactories.ofDescriptor  
>> (Ljava/lang/Object;Ljava/lang/String;)I   6  138,371 ±   0,767   136,939 ± 
>> 1,126 ns/op   1,01x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor  
>> ()V   6   10,528 ±   2,495 4,110 ± 0,047 ns/op   2,56x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor 
>> ([IJLjava/lang/String;Z)Ljava/util/List;   6  209,390 ±   4,583   196,175 ± 
>> 3,211 ns/op   1,07x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ()[Ljava/lang/String;   6   36,039 ±   8,68420,794 ± 1,110 ns/op   1,73x 
>> (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor 
>> (..IIJ)V   6  279,139 ±   6,248   187,934 ± 0,857 ns/op   1,49x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor 
>> (.).   6 2174,387 ± 132,879  1150,652 ± 3,158 ns/op   
>> 1,89x (p = 0,000*)
>>   * = significant
>> 
>> 
>> The other in `ClassDesc::nested`, where to get rid of a simple static method 
>> in `ConstantUtils` I ended up speeding up and simplifying the public factory 
>> method:
>> 
>> Name  CntBase ErrorTest   
>> Error  Unit  Change
>> ClassDescFactories.ReferenceOnly.ofNested   6 209,853 ± 132,525  22,017 ± 
>> 0,573 ns/op   9,53x (p = 0,000*)
>>   * = significant
>> 
>> 
>> The optimizations could be split out and PRd independently, but I think they 
>> are simple enough to include in this refactoring.
>
> Claes Redestad has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Can't call arrayType() from arrayType(int) due different exception types

src/java.base/share/classes/java/lang/constant/ClassDesc.java line 209:

> 207:  */
> 208: default ClassDesc arrayType(int rank) {
> 209: if (rank <= 1) {

Suggestion:

if (rank <= 0) {

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1601429229

Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v10]

[ExE Boss]

On Wed, 15 May 2024 07:20:37 GMT, Claes Redestad  wrote:

>> This PR suggests refactoring the implementation classes of 
>> java.lang.constant into a new package jdk.internal.constant to enable 
>> sharing some trusted static factory methods with users elsewhere in 
>> java.base, such as java.lang.invoke and java.lang.classfile. The refactoring 
>> also adds some "trusted" methods for use when input is pre-validated, and 
>> makes use of them where applicable in the current implementation. There are 
>> more changes in the pipeline which will be folded into #17108 or PR'ed once 
>> that is integrated. 
>> 
>> There are two optimizations mixed up here. One in 
>> `MethodTypeDesc.ofDescriptor`:
>> 
>> Name 
>> (descString) Cnt Base Error  Test   Error  Unit  Change
>> MethodTypeDescFactories.ofDescriptor  
>> (Ljava/lang/Object;Ljava/lang/String;)I   6  138,371 ±   0,767   136,939 ± 
>> 1,126 ns/op   1,01x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor  
>> ()V   6   10,528 ±   2,495 4,110 ± 0,047 ns/op   2,56x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor 
>> ([IJLjava/lang/String;Z)Ljava/util/List;   6  209,390 ±   4,583   196,175 ± 
>> 3,211 ns/op   1,07x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ()[Ljava/lang/String;   6   36,039 ±   8,68420,794 ± 1,110 ns/op   1,73x 
>> (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor 
>> (..IIJ)V   6  279,139 ±   6,248   187,934 ± 0,857 ns/op   1,49x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor 
>> (.).   6 2174,387 ± 132,879  1150,652 ± 3,158 ns/op   
>> 1,89x (p = 0,000*)
>>   * = significant
>> 
>> 
>> The other in `ClassDesc::nested`, where to get rid of a simple static method 
>> in `ConstantUtils` I ended up speeding up and simplifying the public factory 
>> method:
>> 
>> Name  CntBase ErrorTest   
>> Error  Unit  Change
>> ClassDescFactories.ReferenceOnly.ofNested   6 209,853 ± 132,525  22,017 ± 
>> 0,573 ns/op   9,53x (p = 0,000*)
>>   * = significant
>> 
>> 
>> The optimizations could be split out and PRd independently, but I think they 
>> are simple enough to include in this refactoring.
>
> Claes Redestad has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Use sb.repeat, consolidate

src/java.base/share/classes/java/lang/constant/ClassDesc.java line 190:

> 188: MAX_ARRAY_TYPE_DESC_DIMENSIONS + " dimensions");
> 189: }
> 190: String newDesc = new StringBuilder(desc.length() + 
> 1).append('[').append(desc).toString();

This could use [`String​::concat​(String)`], which uses the same underlying 
code path as the `invokedynamic`‑based `String` concatenation:
Suggestion:

String newDesc = "[".concat(desc);


[`String​::concat​(String)`]: 
https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/String.html#concat(java.lang.String)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1601423761

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]

[Alan Bateman]

On Wed, 15 May 2024 10:34:01 GMT, Maurizio Cimadamore  
wrote:

> I don't fully agree that this option is not module related (which is why I 
> gave it that name). The very definition of illegal native access is related 
> to native access occurring from a module that is outside a specific set. So I 
> think it's 50/50 as to whether this option is module-related or not. Of 
> course I can fix the code if there's something clearly better.

It maps here to a jdk.module.* property so I suppose it's okay. The functions 
were introduced to create jdk.module.* properties where the values were a set 
module names or a module path. Maybe these functions should be renamed at some 
point (not here) as they are more widely useful now.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1601421535

Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v12]

[Claes Redestad]

> This PR suggests refactoring the implementation classes of java.lang.constant 
> into a new package jdk.internal.constant to enable sharing some trusted 
> static factory methods with users elsewhere in java.base, such as 
> java.lang.invoke and java.lang.classfile. The refactoring also adds some 
> "trusted" methods for use when input is pre-validated, and makes use of them 
> where applicable in the current implementation. There are more changes in the 
> pipeline which will be folded into #17108 or PR'ed once that is integrated. 
> 
> There are two optimizations mixed up here. One in 
> `MethodTypeDesc.ofDescriptor`:
> 
> Name (descString) 
> Cnt Base Error  Test   Error  Unit  Change
> MethodTypeDescFactories.ofDescriptor  (Ljava/lang/Object;Ljava/lang/String;)I 
>   6  138,371 ±   0,767   136,939 ± 1,126 ns/op   1,01x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor  ()V 
>   6   10,528 ±   2,495 4,110 ± 0,047 ns/op   2,56x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List; 
>   6  209,390 ±   4,583   196,175 ± 3,211 ns/op   1,07x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor()[Ljava/lang/String; 
>   6   36,039 ±   8,68420,794 ± 1,110 ns/op   1,73x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor (..IIJ)V 
>   6  279,139 ±   6,248   187,934 ± 0,857 ns/op   1,49x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor (.). 
>   6 2174,387 ± 132,879  1150,652 ± 3,158 ns/op   1,89x (p = 0,000*)
>   * = significant
> 
> 
> The other in `ClassDesc::nested`, where to get rid of a simple static method 
> in `ConstantUtils` I ended up speeding up and simplifying the public factory 
> method:
> 
> Name  CntBase ErrorTest   
> Error  Unit  Change
> ClassDescFactories.ReferenceOnly.ofNested   6 209,853 ± 132,525  22,017 ± 
> 0,573 ns/op   9,53x (p = 0,000*)
>   * = significant
> 
> 
> The optimizations could be split out and PRd independently, but I think they 
> are simple enough to include in this refactoring.

Claes Redestad has updated the pull request incrementally with one additional 
commit since the last revision:

  Can't call arrayType() from arrayType(int) due different exception types

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19105/files
  - new: https://git.openjdk.org/jdk/pull/19105/files/4e78b1b1..2a1b9ac9

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=19105&range=11
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19105&range=10-11

  Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/19105.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19105/head:pull/19105

PR: https://git.openjdk.org/jdk/pull/19105

Re: RFR: 8320396: Class-File API ClassModel::verify should include checks from hotspot/share/classfile/classFileParser.cpp [v6]

[Adam Sotona]

On Wed, 15 May 2024 10:06:42 GMT, Adam Sotona  wrote:

>> src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
>>  line 205:
>> 
>>> 203: private void verifyAttribute(AttributedElement ae, Attribute a, 
>>> List errors) {
>>> 204: int size = -1;
>>> 205: switch (a) {
>> 
>> Maybe use a switch expression to set `size` where `default` yields -1?
>
> For better readability I've recently turned it from switch expression 
> (yielding values pre-calculated in temporary locals) into explicit 
> assignments to the `size` variable.

I've turned it back to switch expression and compacted it a bit more.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1601399982

Re: RFR: 8320396: Class-File API ClassModel::verify should include checks from hotspot/share/classfile/classFileParser.cpp [v6]

[Adam Sotona]

On Wed, 15 May 2024 09:45:24 GMT, Maurizio Cimadamore  
wrote:

>> Adam Sotona has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   applied the suggested changes
>
> src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
>  line 306:
> 
>> 304: size = 1;
>> 305: for (var ans : aa.parameterAnnotations()) {
>> 306: size += annotationsSize(ans);
> 
> Shouldn't we call the new method here (passing `parameterAnnotations`) ?

Yes, I've compacted it a bit more, thanks.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1601399335

Re: RFR: 8320396: Class-File API ClassModel::verify should include checks from hotspot/share/classfile/classFileParser.cpp [v7]

[Adam Sotona]

> ClassFile API `jdk.internal.classfile.verifier.VerifierImpl` performed only 
> bytecode-level class verification.
> This patch adds `jdk.internal.classfile.verifier.ParserVerifier` with 
> additional class checks inspired by 
> `hotspot/share/classfile/classFileParser.cpp`.
> 
> Also new `VerifierSelfTest::testParserVerifier` has been added.
> 
> Please review.
> 
> Thanks,
> Adam

Adam Sotona has updated the pull request incrementally with one additional 
commit since the last revision:

  applied the suggested changes

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16809/files
  - new: https://git.openjdk.org/jdk/pull/16809/files/9257b7ea..6f12b3bd

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16809&range=06
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16809&range=05-06

  Stats: 92 lines in 1 file changed: 29 ins; 29 del; 34 mod
  Patch: https://git.openjdk.org/jdk/pull/16809.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16809/head:pull/16809

PR: https://git.openjdk.org/jdk/pull/16809

Re: RFR: 8330465: Stable Values and Collections (Internal) [v4]

[Alan Bateman]

On Wed, 15 May 2024 08:49:46 GMT, ExE Boss  wrote:

> Maybe export this interface to `jdk.unsupported`?

I don't we should do that. In general, we need jdk.unsupported to go away in 
the long term. Also integrity of the platform depends on java.base being very 
stingy and not exporting internal packages to other modules.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1601399452

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v5]

[Maurizio Cimadamore]

> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting 
> the use of JNI in the following ways:
> 
> * `System::load` and `System::loadLibrary` are now restricted methods
> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
> * binding a JNI `native` method declaration to a native implementation is now 
> considered a restricted operation
> 
> This PR slightly changes the way in which the JDK deals with restricted 
> methods, even for FFM API calls. In Java 22, the single 
> `--enable-native-access` was used both to specify a set of modules for which 
> native access should be allowed *and* to specify whether illegal native 
> access (that is, native access occurring from a module not specified by 
> `--enable-native-access`) should be treated as an error or a warning. More 
> specifically, an error is only issued if the `--enable-native-access flag` is 
> used at least once.
> 
> Here, a new flag is introduced, namely 
> `illegal-native-access=allow/warn/deny`, which is used to specify what should 
> happen when access to a restricted method and/or functionality is found 
> outside the set of modules specified with `--enable-native-access`. The 
> default policy is `warn`, but users can select `allow` to suppress the 
> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This 
> aligns the treatment of restricted methods with other mechanisms, such as 
> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
> 
> Some changes were required in the package-info javadoc for 
> `java.lang.foreign`, to reflect the changes in the command line flags 
> described above.

Maurizio Cimadamore has updated the pull request incrementally with one 
additional commit since the last revision:

  Refine warning text for JNI method binding

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19213/files
  - new: https://git.openjdk.org/jdk/pull/19213/files/0d21bf99..daf729f4

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=04
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=03-04

  Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod
  Patch: https://git.openjdk.org/jdk/pull/19213.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19213/head:pull/19213

PR: https://git.openjdk.org/jdk/pull/19213

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]

[Maurizio Cimadamore]

On Wed, 15 May 2024 06:15:35 GMT, Alan Bateman  wrote:

>> So my recollection/understanding is that we use this mechanism to convert 
>> module-related `--` flags passed to the VM into system properties that the 
>> Java code can then read, but we set them up such that you are not allowed to 
>> specify them directly via `-D`. Is the question here whether this new 
>> property should be in the `jdk.module` namespace?
>
> That's my recollection too. The usage here isn' related to modules which 
> makes me wonder if this function should be renamed (not by this PR of course) 
> of if we should be using PropertyList_unique_add (with AddProperty, 
> WriteableProperty, InternalProperty) instead. There will be further GNU style 
> options coming that will likely need to map to an internal system property in 
> the same way.

I don't fully agree that this option is not module related (which is why I gave 
it that name). The very definition of illegal native access is related to 
native access occurring from a module that is outside a specific set. So I 
think it's 50/50 as to whether this option is module-related or not. Of course 
I can fix the code if there's something clearly better.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1601386336

Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v10]

[Claes Redestad]

On Wed, 15 May 2024 09:51:13 GMT, Adam Sotona  wrote:

>> Claes Redestad has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Use sb.repeat, consolidate
>
> src/java.base/share/classes/java/lang/constant/ClassDesc.java line 341:
> 
>> 339: String desc = descriptorString();
>> 340: int index = desc.lastIndexOf('/');
>> 341: return (index == -1) ? "" : internalToBinary(desc.substring(1, 
>> index - 1));
> 
> Here it cuts the package name, for example 
> `ConstantDescs.CD_Integer.packageName()` returns `java.lan`
> 
> Suggestion:
> 
> return (index == -1) ? "" : internalToBinary(desc.substring(1, 
> index));

Thanks! Verified there were tests covering this.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1601383209

Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v11]

[Claes Redestad]

> This PR suggests refactoring the implementation classes of java.lang.constant 
> into a new package jdk.internal.constant to enable sharing some trusted 
> static factory methods with users elsewhere in java.base, such as 
> java.lang.invoke and java.lang.classfile. The refactoring also adds some 
> "trusted" methods for use when input is pre-validated, and makes use of them 
> where applicable in the current implementation. There are more changes in the 
> pipeline which will be folded into #17108 or PR'ed once that is integrated. 
> 
> There are two optimizations mixed up here. One in 
> `MethodTypeDesc.ofDescriptor`:
> 
> Name (descString) 
> Cnt Base Error  Test   Error  Unit  Change
> MethodTypeDescFactories.ofDescriptor  (Ljava/lang/Object;Ljava/lang/String;)I 
>   6  138,371 ±   0,767   136,939 ± 1,126 ns/op   1,01x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor  ()V 
>   6   10,528 ±   2,495 4,110 ± 0,047 ns/op   2,56x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List; 
>   6  209,390 ±   4,583   196,175 ± 3,211 ns/op   1,07x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor()[Ljava/lang/String; 
>   6   36,039 ±   8,68420,794 ± 1,110 ns/op   1,73x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor (..IIJ)V 
>   6  279,139 ±   6,248   187,934 ± 0,857 ns/op   1,49x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor (.). 
>   6 2174,387 ± 132,879  1150,652 ± 3,158 ns/op   1,89x (p = 0,000*)
>   * = significant
> 
> 
> The other in `ClassDesc::nested`, where to get rid of a simple static method 
> in `ConstantUtils` I ended up speeding up and simplifying the public factory 
> method:
> 
> Name  CntBase ErrorTest   
> Error  Unit  Change
> ClassDescFactories.ReferenceOnly.ofNested   6 209,853 ± 132,525  22,017 ± 
> 0,573 ns/op   9,53x (p = 0,000*)
>   * = significant
> 
> 
> The optimizations could be split out and PRd independently, but I think they 
> are simple enough to include in this refactoring.

Claes Redestad has updated the pull request incrementally with one additional 
commit since the last revision:

  Update src/java.base/share/classes/java/lang/constant/ClassDesc.java
  
  Co-authored-by: Adam Sotona <10807609+asot...@users.noreply.github.com>

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19105/files
  - new: https://git.openjdk.org/jdk/pull/19105/files/b0aca160..4e78b1b1

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=19105&range=10
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19105&range=09-10

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/19105.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19105/head:pull/19105

PR: https://git.openjdk.org/jdk/pull/19105

Re: RFR: 8329653: JLILaunchTest fails on AIX after JDK-8329131 [v3]

[Christoph Langer]

On Tue, 7 May 2024 08:08:05 GMT, Joachim Kern  wrote:

>> Since ~ end of March, after 
>> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), 
>> tools/launcher/JliLaunchTest.java fails on AIX. Failure is :
>> 
>>  stdout: [];
>>  stderr: [Error: could not find libjava.so
>> Error: Could not find Java SE Runtime Environment.
>> ]
>>  exitValue = 2
>> 
>> java.lang.RuntimeException: Expected to get exit value of [0], exit value 
>> is: [2]
>> at 
>> jdk.test.lib.process.OutputAnalyzer.shouldHaveExitValue(OutputAnalyzer.java:521)
>> at JliLaunchTest.main(JliLaunchTest.java:58)
>> at 
>> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
>> at java.base/java.lang.reflect.Method.invoke(Method.java:580)
>> at 
>> com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333)
>> at java.base/java.lang.Thread.run(Thread.java:1575)
>> 
>> Maybe we need to do further adjustments to 
>> BUILD_JDK_JTREG_EXECUTABLES_LIBS_exeJliLaunchTest / 
>> BUILD_JDK_JTREG_EXECUTABLES_LDFLAGS_exeJliLaunchTest on AIX ?
>> Or somehow adjust the coding that attempts to find parts of "JRE" 
>> (libjava.so) ? The libjli.so is gone on AIX after 
>> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), and with this 
>> also the image discovery based on GetApplicationHomeFromDll which uses 
>> libjli.so .
>> 
>> Without libjli.so we have to analyze the LD-LIBRARY_PATH / LIBPATH envvar. 
>> There is no other methos available on AIX, because it lacks the $ORIGIN 
>> feature of the rpath.
>
> Joachim Kern has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   cosmetic changes

Marked as reviewed by clanger (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19000#pullrequestreview-2057562812

Re: RFR: 8331670: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal

[Maurizio Cimadamore]

On Fri, 10 May 2024 10:06:55 GMT, Alan Bateman  wrote:

> This is the implementation changes for JEP 471.
> 
> The methods in sun.misc.Unsafe for on-heap and off-heap access are deprecated 
> for removal. This means a removal warning at compile time. No methods have 
> been removed. A deprecated message is added to each of the methods but 
> unlikely to be seen as the JDK does not generate or publish the API docs for 
> this class.
> 
> A new command line option --sun-misc-unsafe-memory-access=$value is 
> introduced to allow or deny access to these methods. The default proposed for 
> JDK 23 is "allow" so no change in behavior compared to JDK 22 or previous 
> releases.
> 
> A new test is added to test the command line option settings. The existing 
> micros for FFM that use Unsafe are updated to suppress the removal warning at 
> compile time. A new micro is introduced with a small sample of methods to 
> ensure the changes doesn't cause any perf regressions.
> 
> For now, the changes include the update to the man page for the "java" 
> command. It might be that this has to be separated out so that it goes with 
> other updates in the release.

Looks good. I've left some optional comments.

test/micro/org/openjdk/bench/java/lang/foreign/AllocFromTest.java line 167:

> 165: 
> 166: @Override
> 167: @SuppressWarnings("removal")

I believe we already disable a bunch of warnings from the command line when 
compiling these benchmarks. Perhaps we can just tweak the build script in a 
similar way and avoid the changes to the sources? E.g.


DISABLED_WARNINGS := restricted this-escape processing rawtypes cast \


Should we add `removal` there?

-

Marked as reviewed by mcimadamore (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19174#pullrequestreview-2057562462
PR Review Comment: https://git.openjdk.org/jdk/pull/19174#discussion_r1601368794

Re: RFR: 8331670: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal

[Maurizio Cimadamore]

On Fri, 10 May 2024 10:06:55 GMT, Alan Bateman  wrote:

> This is the implementation changes for JEP 471.
> 
> The methods in sun.misc.Unsafe for on-heap and off-heap access are deprecated 
> for removal. This means a removal warning at compile time. No methods have 
> been removed. A deprecated message is added to each of the methods but 
> unlikely to be seen as the JDK does not generate or publish the API docs for 
> this class.
> 
> A new command line option --sun-misc-unsafe-memory-access=$value is 
> introduced to allow or deny access to these methods. The default proposed for 
> JDK 23 is "allow" so no change in behavior compared to JDK 22 or previous 
> releases.
> 
> A new test is added to test the command line option settings. The existing 
> micros for FFM that use Unsafe are updated to suppress the removal warning at 
> compile time. A new micro is introduced with a small sample of methods to 
> ensure the changes doesn't cause any perf regressions.
> 
> For now, the changes include the update to the man page for the "java" 
> command. It might be that this has to be separated out so that it goes with 
> other updates in the release.

src/jdk.unsupported/share/classes/sun/misc/Unsafe.java line 1764:

> 1762: }
> 1763: // set to true by first usage of memory-access method
> 1764: private static @Stable boolean memoryAccessWarned;

Very nice code. The fact that you only need to report first use (and not first 
use per module, like in FFM) simplify things a bit (and, most importantly, 
avoids the need to make all unsafe methods caller sensitive!).

src/jdk.unsupported/share/classes/sun/misc/Unsafe.java line 1869:

> 1867: default -> {
> 1868: // emit warning
> 1869: log("--sun-misc-unsafe-memory-access ignored, 
> value '" + value +

The FFM code throws if an unknown value is passed. Here we log. Should we try 
to be more consistent?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19174#discussion_r1601362348
PR Review Comment: https://git.openjdk.org/jdk/pull/19174#discussion_r1601364811

Re: RFR: 8320396: Class-File API ClassModel::verify should include checks from hotspot/share/classfile/classFileParser.cpp [v6]

[Adam Sotona]

On Wed, 15 May 2024 09:43:04 GMT, Maurizio Cimadamore  
wrote:

>> Adam Sotona has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   applied the suggested changes
>
> src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
>  line 205:
> 
>> 203: private void verifyAttribute(AttributedElement ae, Attribute a, 
>> List errors) {
>> 204: int size = -1;
>> 205: switch (a) {
> 
> Maybe use a switch expression to set `size` where `default` yields -1?

For better readability I've recently turned it from switch expression (yielding 
values pre-calculated in temporary locals) into explicit assignments to the 
`size` variable.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1601349377

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v4]

[Maurizio Cimadamore]

On Wed, 15 May 2024 07:55:27 GMT, ExE Boss  wrote:

> Note that this line is still not entirely correct, as for code like:

You are correct - the message is however consistent with what written in JEP 
472. I'll discuss with @pron

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1601335120

Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v10]

[Adam Sotona]

On Wed, 15 May 2024 07:20:37 GMT, Claes Redestad  wrote:

>> This PR suggests refactoring the implementation classes of 
>> java.lang.constant into a new package jdk.internal.constant to enable 
>> sharing some trusted static factory methods with users elsewhere in 
>> java.base, such as java.lang.invoke and java.lang.classfile. The refactoring 
>> also adds some "trusted" methods for use when input is pre-validated, and 
>> makes use of them where applicable in the current implementation. There are 
>> more changes in the pipeline which will be folded into #17108 or PR'ed once 
>> that is integrated. 
>> 
>> There are two optimizations mixed up here. One in 
>> `MethodTypeDesc.ofDescriptor`:
>> 
>> Name 
>> (descString) Cnt Base Error  Test   Error  Unit  Change
>> MethodTypeDescFactories.ofDescriptor  
>> (Ljava/lang/Object;Ljava/lang/String;)I   6  138,371 ±   0,767   136,939 ± 
>> 1,126 ns/op   1,01x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor  
>> ()V   6   10,528 ±   2,495 4,110 ± 0,047 ns/op   2,56x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor 
>> ([IJLjava/lang/String;Z)Ljava/util/List;   6  209,390 ±   4,583   196,175 ± 
>> 3,211 ns/op   1,07x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ()[Ljava/lang/String;   6   36,039 ±   8,68420,794 ± 1,110 ns/op   1,73x 
>> (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor 
>> (..IIJ)V   6  279,139 ±   6,248   187,934 ± 0,857 ns/op   1,49x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor 
>> (.).   6 2174,387 ± 132,879  1150,652 ± 3,158 ns/op   
>> 1,89x (p = 0,000*)
>>   * = significant
>> 
>> 
>> The other in `ClassDesc::nested`, where to get rid of a simple static method 
>> in `ConstantUtils` I ended up speeding up and simplifying the public factory 
>> method:
>> 
>> Name  CntBase ErrorTest   
>> Error  Unit  Change
>> ClassDescFactories.ReferenceOnly.ofNested   6 209,853 ± 132,525  22,017 ± 
>> 0,573 ns/op   9,53x (p = 0,000*)
>>   * = significant
>> 
>> 
>> The optimizations could be split out and PRd independently, but I think they 
>> are simple enough to include in this refactoring.
>
> Claes Redestad has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Use sb.repeat, consolidate

src/java.base/share/classes/java/lang/constant/ClassDesc.java line 341:

> 339: String desc = descriptorString();
> 340: int index = desc.lastIndexOf('/');
> 341: return (index == -1) ? "" : internalToBinary(desc.substring(1, 
> index - 1));

Here it cuts the package name, for example 
`ConstantDescs.CD_Integer.packageName()` returns `java.lan`

Suggestion:

return (index == -1) ? "" : internalToBinary(desc.substring(1, index));

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1601328026

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory [v5]

[Raffaello Giulietti]

> All random number generator algorithms are implemented in module `java.base`. 
> The usage of `ServiceLoader` in `j.u.r.RandomGeneratorFactory` is no longer 
> needed.

Raffaello Giulietti has updated the pull request incrementally with one 
additional commit since the last revision:

  Small documentation changes.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19212/files
  - new: https://git.openjdk.org/jdk/pull/19212/files/4bc5bf95..920655a5

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=19212&range=04
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19212&range=03-04

  Stats: 23 lines in 2 files changed: 6 ins; 1 del; 16 mod
  Patch: https://git.openjdk.org/jdk/pull/19212.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19212/head:pull/19212

PR: https://git.openjdk.org/jdk/pull/19212

Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v27]

[Severin Gehwolf]

> 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 b
 eing 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.internal.vm.compiler.manage...

Severin Gehwolf has updated the pull request incrementally with two additional 
commits since the last revision:

 - Simplify JLINK_JDK_EXTRA_OPTS var handling
 - Only add runtime track files for linkable runtimes

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/14787/files
  - new: https://git.openjdk.org/jdk/pull/14787/files/67aea4ca..be30a182

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=14787&range=26
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14787&range=25-26

  Stats: 11 lines in 2 files changed: 5 ins; 5 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/14787.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14787/head:pull/14787

PR: https://git.openjdk.org/jdk/pull/14787

Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v26]

[Severin Gehwolf]

On Wed, 8 May 2024 22:36:51 GMT, Magnus Ihse Bursie  wrote:

>> Severin Gehwolf has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 105 commits:
>> 
>>  - Generate the runtime image link diff at jlink time
>>
>>But only do that once the plugin-pipeline has run. The generation is
>>enabled with the hidden option '--generate-linkable-runtime' and
>>the resource pools available at jlink time are being used to generate
>>the diff.
>>  - Merge branch 'master' into jdk-8311302-jmodless-link
>>  - Use shorter name for the build tool
>>  - Review feedback from Erik J.
>>  - Remove dependency on CDS which isn't needed anymore
>>  - Fix coment
>>  - Fix comment
>>  - Fix typo
>>  - Revert some now unneded build changes
>>  - Follow build tools naming convention
>>  - ... and 95 more: https://git.openjdk.org/jdk/compare/23a72a1f...67aea4ca
>
> make/Images.gmk line 100:
> 
>> 98: 
>> 99: ifeq ($(JLINK_PRODUCE_RUNTIME_LINK_JDK), true)
>> 100:   JLINK_JDK_EXTRA_OPTS := $(JLINK_JDK_EXTRA_OPTS) 
>> --generate-linkable-runtime
> 
> I just noticed this slight improvement:
> 
> Suggestion:
> 
>   JLINK_JDK_EXTRA_OPTS += --generate-linkable-runtime

Thanks, fixed.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1601322116

Re: RFR: 8331987: Enhance stacktrace clarity for CompletableFuture CancellationException [v2]

[Daniel Fuchs]

On Mon, 13 May 2024 23:59:17 GMT, Viktor Klang  wrote:

>> This change adds wrapping of the CancellationException produced by 
>> CompletableFuture::get() and CompletableFuture::join() to add more 
>> diagnostic information and align better with FutureTask.
>> 
>> Running the sample code from the JBS issue in JShell will produce the 
>> following:
>> 
>> 
>> jshell> java.util.concurrent.CancellationException: get
>>  at 
>> java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:392)
>>  at 
>> java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2073)
>>  at REPL.$JShell$18.m2($JShell$18.java:10)
>>  at REPL.$JShell$17.m1($JShell$17.java:8)
>>  at REPL.$JShell$16B.lambda$main$0($JShell$16B.java:8)
>>  at java.base/java.lang.Thread.run(Thread.java:1575)
>> Caused by: java.util.concurrent.CancellationException
>>  at 
>> java.base/java.util.concurrent.CompletableFuture.cancel(CompletableFuture.java:2510)
>>  at REPL.$JShell$16B.lambda$main$1($JShell$16B.java:11)
>>  ... 1 more
>
> Viktor Klang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Adding CancellationException detail message to CompletableFuture

LGTM

-

Marked as reviewed by dfuchs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19219#pullrequestreview-2057484574

Re: RFR: 8320396: Class-File API ClassModel::verify should include checks from hotspot/share/classfile/classFileParser.cpp [v6]

[Maurizio Cimadamore]

On Tue, 14 May 2024 11:42:19 GMT, Adam Sotona  wrote:

>> ClassFile API `jdk.internal.classfile.verifier.VerifierImpl` performed only 
>> bytecode-level class verification.
>> This patch adds `jdk.internal.classfile.verifier.ParserVerifier` with 
>> additional class checks inspired by 
>> `hotspot/share/classfile/classFileParser.cpp`.
>> 
>> Also new `VerifierSelfTest::testParserVerifier` has been added.
>> 
>> Please review.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   applied the suggested changes

src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
 line 205:

> 203: private void verifyAttribute(AttributedElement ae, Attribute a, 
> List errors) {
> 204: int size = -1;
> 205: switch (a) {

Maybe use a switch expression to set `size` where `default` yields -1?

src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
 line 306:

> 304: size = 1;
> 305: for (var ans : aa.parameterAnnotations()) {
> 306: size += annotationsSize(ans);

Shouldn't we call the new method here (passing `parameterAnnotations`) ?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1601317434
PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1601320492

RFR: 8331851: Add specific regression leap year tests for Calendar.roll()

[serhiysachkov]

Add specific regression tests for Calendar.roll() method to explicitly various 
leap year test scenarios. This is inspired by the ambiguity which occurred in 
leap year unaware test creation as in case with Calendar.add() in swing 
component test case as detailed in 
(https://bugs.openjdk.org/browse/JDK-8327088).

-

Commit messages:
 - JDK-8331851 Add specific regression leap year tests for Calendar.roll()

Changes: https://git.openjdk.org/jdk/pull/19247/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19247&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8331851
  Stats: 190 lines in 1 file changed: 190 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/19247.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19247/head:pull/19247

PR: https://git.openjdk.org/jdk/pull/19247

Re: RFR: 8329653: JLILaunchTest fails on AIX after JDK-8329131 [v3]

[Martin Doerr]

On Tue, 7 May 2024 08:08:05 GMT, Joachim Kern  wrote:

>> Since ~ end of March, after 
>> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), 
>> tools/launcher/JliLaunchTest.java fails on AIX. Failure is :
>> 
>>  stdout: [];
>>  stderr: [Error: could not find libjava.so
>> Error: Could not find Java SE Runtime Environment.
>> ]
>>  exitValue = 2
>> 
>> java.lang.RuntimeException: Expected to get exit value of [0], exit value 
>> is: [2]
>> at 
>> jdk.test.lib.process.OutputAnalyzer.shouldHaveExitValue(OutputAnalyzer.java:521)
>> at JliLaunchTest.main(JliLaunchTest.java:58)
>> at 
>> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
>> at java.base/java.lang.reflect.Method.invoke(Method.java:580)
>> at 
>> com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333)
>> at java.base/java.lang.Thread.run(Thread.java:1575)
>> 
>> Maybe we need to do further adjustments to 
>> BUILD_JDK_JTREG_EXECUTABLES_LIBS_exeJliLaunchTest / 
>> BUILD_JDK_JTREG_EXECUTABLES_LDFLAGS_exeJliLaunchTest on AIX ?
>> Or somehow adjust the coding that attempts to find parts of "JRE" 
>> (libjava.so) ? The libjli.so is gone on AIX after 
>> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), and with this 
>> also the image discovery based on GetApplicationHomeFromDll which uses 
>> libjli.so .
>> 
>> Without libjli.so we have to analyze the LD-LIBRARY_PATH / LIBPATH envvar. 
>> There is no other methos available on AIX, because it lacks the $ORIGIN 
>> feature of the rpath.
>
> Joachim Kern has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   cosmetic changes

LGTM.

-

Marked as reviewed by mdoerr (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19000#pullrequestreview-2057450094

Re: RFR: 8330465: Stable Values and Collections (Internal) [v4]

[ExE Boss]

On Wed, 15 May 2024 07:48:42 GMT, Per Minborg  wrote:

>> # Stable Values & Collections (Internal)
>> 
>> ## Summary
>> This PR proposes to introduce an internal _Stable Values & Collections_ API, 
>> which provides immutable value holders where elements are initialized _at 
>> most once_. Stable Values & Collections offer the performance and safety 
>> benefits of final fields while offering greater flexibility as to the timing 
>> of initialization.
>> 
>> ## Goals
>>  * Provide an easy and intuitive API to describe value holders that can 
>> change at most once.
>>  * Decouple declaration from initialization without significant footprint or 
>> performance penalties.
>>  * Reduce the amount of static initializer and/or field initialization code.
>>  * Uphold integrity and consistency, even in a multi-threaded environment.
>>  
>> For more details, see the draft JEP: https://openjdk.org/jeps/8312611
>> 
>> ## Performance 
>> Performance compared to instance variables using an `AtomicReference` and 
>> one protected by double-checked locking under concurrent access by 8 threads:
>> 
>> 
>> Benchmark   Mode  Cnt  Score   Error  Units
>> StableBenchmark.instanceAtomic  avgt   10  1.576 ? 0.052  ns/op
>> StableBenchmark.instanceDCL avgt   10  1.608 ? 0.059  ns/op
>> StableBenchmark.instanceStable  avgt   10  0.979 ? 0.023  ns/op <- 
>> StableValue (~40% faster than DCL)
>> 
>> 
>> Performance compared to static variables protected by `AtomicReference`, 
>> class-holder idiom holder, and double-checked locking (8 threads):
>> 
>> 
>> Benchmark   Mode  Cnt  Score   Error  Units
>> StableBenchmark.staticAtomicavgt   10  1.335 ? 0.056  ns/op
>> StableBenchmark.staticCHI   avgt   10  0.623 ? 0.086  ns/op
>> StableBenchmark.staticDCL   avgt   10  1.418 ? 0.171  ns/op
>> StableBenchmark.staticList  avgt   10  0.617 ? 0.024  ns/op
>> StableBenchmark.staticStableavgt   10  0.604 ? 0.022  ns/op <- 
>> StableValue ( > 2x faster than `AtomicInteger` and DCL)
>> 
>> 
>> Performance for stable lists in both instance and static contexts whereby 
>> the sum of random contents is calculated for stable lists (which are 
>> thread-safe) compared to `ArrayList` instances (which are not thread-safe) 
>> (under single thread access):
>> 
>> 
>> Benchmark Mode  Cnt  Score   Error  Units
>> StableListSumBenchmark.instanceArrayList  avgt   10  0.356 ? 0.005  ns/op
>> StableListSumBenchmark.instanceList   avgt   10  0.373 ? 0.017  
>> ns/op <- Stable list
>> StableListSumBenchmark.staticArrayListavgt   10  0.352 ? ...
>
> Per Minborg has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Revise docs for ofBackground()

src/java.base/share/classes/jdk/internal/lang/StableValue.java line 1:

> 1: /*

Maybe also add `StableValue​::⁠ofLazy​(Supplier)` which behaves 
more like the original **Computed Constants** JEP draft?

src/java.base/share/classes/jdk/internal/lang/stable/TrustedFieldType.java line 
14:

> 12:  * operations.
> 13:  */
> 14: public sealed interface TrustedFieldType

Maybe export this interface to `jdk.unsupported`?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1601229245
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1601223866

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v4]

[ExE Boss]

On Tue, 14 May 2024 18:10:28 GMT, Maurizio Cimadamore  
wrote:

>> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting 
>> the use of JNI in the following ways:
>> 
>> * `System::load` and `System::loadLibrary` are now restricted methods
>> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
>> * binding a JNI `native` method declaration to a native implementation is 
>> now considered a restricted operation
>> 
>> This PR slightly changes the way in which the JDK deals with restricted 
>> methods, even for FFM API calls. In Java 22, the single 
>> `--enable-native-access` was used both to specify a set of modules for which 
>> native access should be allowed *and* to specify whether illegal native 
>> access (that is, native access occurring from a module not specified by 
>> `--enable-native-access`) should be treated as an error or a warning. More 
>> specifically, an error is only issued if the `--enable-native-access flag` 
>> is used at least once.
>> 
>> Here, a new flag is introduced, namely 
>> `illegal-native-access=allow/warn/deny`, which is used to specify what 
>> should happen when access to a restricted method and/or functionality is 
>> found outside the set of modules specified with `--enable-native-access`. 
>> The default policy is `warn`, but users can select `allow` to suppress the 
>> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This 
>> aligns the treatment of restricted methods with other mechanisms, such as 
>> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
>> 
>> Some changes were required in the package-info javadoc for 
>> `java.lang.foreign`, to reflect the changes in the command line flags 
>> described above.
>
> Maurizio Cimadamore has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - Address review comments
>Improve warning for JNI methods, similar to what's described in JEP 472
>Beef up tests
>  - Address review comments

src/java.base/share/classes/java/lang/Module.java line 334:

> 332: System.err.printf("""
> 333: WARNING: A native method in %s has been bound
> 334: WARNING: %s has been called by %s in %s

Note that this line is still not entirely correct, as for code like:

// in module a:
package a;

import b.Foo;

public class Foo {
public static void main(String... args) {
System.load("JNI library implementing Java_b_Bar_nativeMethod");

Bar.nativeMethod();
}
}


// in module b:
package b;

public class Bar {
public static native void nativeMethod();
}


It’ll show `Bar` as the caller of `Bar::nativeMethod()`, even though the caller 
is `Foo` in this case, which is why I initially suggested just omitting the 
caller from **JNI** linkage warnings.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1601140578

Re: RFR: 8331940: ClassFile API ArrayIndexOutOfBoundsException with certain class files [v2]

[Adam Sotona]

On Tue, 14 May 2024 15:59:33 GMT, Paul Sandoz  wrote:

>> Adam Sotona has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   fixed exception message
>
> src/java.base/share/classes/jdk/internal/classfile/impl/CodeImpl.java line 
> 241:
> 
>> 239: int startPc = classReader.readU2(p);
>> 240: if (startPc > codeLength) {
>> 241: throw new 
>> IllegalArgumentException(String.format("Line number out of range; 
>> start_pc=%d, codeLength=%d",
> 
> It's the byte code index that is out of range, not the line number associated 
> with it.

Right, fixed the message, thanks!

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19230#discussion_r1601131607

Re: RFR: 8331940: ClassFile API ArrayIndexOutOfBoundsException with certain class files [v2]

[Adam Sotona]

> Class file with `LineNumberTable` attribute element pointing behind the 
> bytecode array throws `ArrayIndexOutOfBoundsException`.
> This patch performs the check and throws  expected `IllegalArgumentException` 
> instead.
> Relevant test is added.
> 
> Please review.
> 
> Thanks,
> Adam

Adam Sotona has updated the pull request incrementally with one additional 
commit since the last revision:

  fixed exception message

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19230/files
  - new: https://git.openjdk.org/jdk/pull/19230/files/f2a8d981..df6ecdc2

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=19230&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19230&range=00-01

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/19230.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19230/head:pull/19230

PR: https://git.openjdk.org/jdk/pull/19230

Re: RFR: 8330465: Stable Values and Collections (Internal) [v4]

[Per Minborg]

> # Stable Values & Collections (Internal)
> 
> ## Summary
> This PR proposes to introduce an internal _Stable Values & Collections_ API, 
> which provides immutable value holders where elements are initialized _at 
> most once_. Stable Values & Collections offer the performance and safety 
> benefits of final fields while offering greater flexibility as to the timing 
> of initialization.
> 
> ## Goals
>  * Provide an easy and intuitive API to describe value holders that can 
> change at most once.
>  * Decouple declaration from initialization without significant footprint or 
> performance penalties.
>  * Reduce the amount of static initializer and/or field initialization code.
>  * Uphold integrity and consistency, even in a multi-threaded environment.
>  
> For more details, see the draft JEP: https://openjdk.org/jeps/8312611
> 
> ## Performance 
> Performance compared to instance variables using an `AtomicReference` and one 
> protected by double-checked locking under concurrent access by 8 threads:
> 
> 
> Benchmark   Mode  Cnt  Score   Error  Units
> StableBenchmark.instanceAtomic  avgt   10  1.576 ? 0.052  ns/op
> StableBenchmark.instanceDCL avgt   10  1.608 ? 0.059  ns/op
> StableBenchmark.instanceStable  avgt   10  0.979 ? 0.023  ns/op <- 
> StableValue (~40% faster than DCL)
> 
> 
> Performance compared to static variables protected by `AtomicReference`, 
> class-holder idiom holder, and double-checked locking (8 threads):
> 
> 
> Benchmark   Mode  Cnt  Score   Error  Units
> StableBenchmark.staticAtomicavgt   10  1.335 ? 0.056  ns/op
> StableBenchmark.staticCHI   avgt   10  0.623 ? 0.086  ns/op
> StableBenchmark.staticDCL   avgt   10  1.418 ? 0.171  ns/op
> StableBenchmark.staticList  avgt   10  0.617 ? 0.024  ns/op
> StableBenchmark.staticStableavgt   10  0.604 ? 0.022  ns/op <- 
> StableValue ( > 2x faster than `AtomicInteger` and DCL)
> 
> 
> Performance for stable lists in both instance and static contexts whereby the 
> sum of random contents is calculated for stable lists (which are thread-safe) 
> compared to `ArrayList` instances (which are not thread-safe) (under single 
> thread access):
> 
> 
> Benchmark Mode  Cnt  Score   Error  Units
> StableListSumBenchmark.instanceArrayList  avgt   10  0.356 ? 0.005  ns/op
> StableListSumBenchmark.instanceList   avgt   10  0.373 ? 0.017  ns/op 
> <- Stable list
> StableListSumBenchmark.staticArrayListavgt   10  0.352 ? 0.002  ns/op
> StableListSumBenchmark.staticList avgt   10  0.356 ? 0.00...

Per Minborg has updated the pull request incrementally with one additional 
commit since the last revision:

  Revise docs for ofBackground()

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18794/files
  - new: https://git.openjdk.org/jdk/pull/18794/files/7db1101c..c92b16c4

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18794&range=03
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18794&range=02-03

  Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.org/jdk/pull/18794.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18794/head:pull/18794

PR: https://git.openjdk.org/jdk/pull/18794

Re: RFR: 8330465: Stable Values and Collections (Internal) [v3]

[Per Minborg]

> # Stable Values & Collections (Internal)
> 
> ## Summary
> This PR proposes to introduce an internal _Stable Values & Collections_ API, 
> which provides immutable value holders where elements are initialized _at 
> most once_. Stable Values & Collections offer the performance and safety 
> benefits of final fields while offering greater flexibility as to the timing 
> of initialization.
> 
> ## Goals
>  * Provide an easy and intuitive API to describe value holders that can 
> change at most once.
>  * Decouple declaration from initialization without significant footprint or 
> performance penalties.
>  * Reduce the amount of static initializer and/or field initialization code.
>  * Uphold integrity and consistency, even in a multi-threaded environment.
>  
> For more details, see the draft JEP: https://openjdk.org/jeps/8312611
> 
> ## Performance 
> Performance compared to instance variables using an `AtomicReference` and one 
> protected by double-checked locking under concurrent access by 8 threads:
> 
> 
> Benchmark   Mode  Cnt  Score   Error  Units
> StableBenchmark.instanceAtomic  avgt   10  1.576 ? 0.052  ns/op
> StableBenchmark.instanceDCL avgt   10  1.608 ? 0.059  ns/op
> StableBenchmark.instanceStable  avgt   10  0.979 ? 0.023  ns/op <- 
> StableValue (~40% faster than DCL)
> 
> 
> Performance compared to static variables protected by `AtomicReference`, 
> class-holder idiom holder, and double-checked locking (8 threads):
> 
> 
> Benchmark   Mode  Cnt  Score   Error  Units
> StableBenchmark.staticAtomicavgt   10  1.335 ? 0.056  ns/op
> StableBenchmark.staticCHI   avgt   10  0.623 ? 0.086  ns/op
> StableBenchmark.staticDCL   avgt   10  1.418 ? 0.171  ns/op
> StableBenchmark.staticList  avgt   10  0.617 ? 0.024  ns/op
> StableBenchmark.staticStableavgt   10  0.604 ? 0.022  ns/op <- 
> StableValue ( > 2x faster than `AtomicInteger` and DCL)
> 
> 
> Performance for stable lists in both instance and static contexts whereby the 
> sum of random contents is calculated for stable lists (which are thread-safe) 
> compared to `ArrayList` instances (which are not thread-safe) (under single 
> thread access):
> 
> 
> Benchmark Mode  Cnt  Score   Error  Units
> StableListSumBenchmark.instanceArrayList  avgt   10  0.356 ? 0.005  ns/op
> StableListSumBenchmark.instanceList   avgt   10  0.373 ? 0.017  ns/op 
> <- Stable list
> StableListSumBenchmark.staticArrayListavgt   10  0.352 ? 0.002  ns/op
> StableListSumBenchmark.staticList avgt   10  0.356 ? 0.00...

Per Minborg has updated the pull request incrementally with one additional 
commit since the last revision:

  Add delegation to the thread's exception handler

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18794/files
  - new: https://git.openjdk.org/jdk/pull/18794/files/d7c31585..7db1101c

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18794&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18794&range=01-02

  Stats: 70 lines in 3 files changed: 62 ins; 0 del; 8 mod
  Patch: https://git.openjdk.org/jdk/pull/18794.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18794/head:pull/18794

PR: https://git.openjdk.org/jdk/pull/18794

Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v10]

[Claes Redestad]

> This PR suggests refactoring the implementation classes of java.lang.constant 
> into a new package jdk.internal.constant to enable sharing some trusted 
> static factory methods with users elsewhere in java.base, such as 
> java.lang.invoke and java.lang.classfile. The refactoring also adds some 
> "trusted" methods for use when input is pre-validated, and makes use of them 
> where applicable in the current implementation. There are more changes in the 
> pipeline which will be folded into #17108 or PR'ed once that is integrated. 
> 
> There are two optimizations mixed up here. One in 
> `MethodTypeDesc.ofDescriptor`:
> 
> Name (descString) 
> Cnt Base Error  Test   Error  Unit  Change
> MethodTypeDescFactories.ofDescriptor  (Ljava/lang/Object;Ljava/lang/String;)I 
>   6  138,371 ±   0,767   136,939 ± 1,126 ns/op   1,01x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor  ()V 
>   6   10,528 ±   2,495 4,110 ± 0,047 ns/op   2,56x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List; 
>   6  209,390 ±   4,583   196,175 ± 3,211 ns/op   1,07x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor()[Ljava/lang/String; 
>   6   36,039 ±   8,68420,794 ± 1,110 ns/op   1,73x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor (..IIJ)V 
>   6  279,139 ±   6,248   187,934 ± 0,857 ns/op   1,49x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor (.). 
>   6 2174,387 ± 132,879  1150,652 ± 3,158 ns/op   1,89x (p = 0,000*)
>   * = significant
> 
> 
> The other in `ClassDesc::nested`, where to get rid of a simple static method 
> in `ConstantUtils` I ended up speeding up and simplifying the public factory 
> method:
> 
> Name  CntBase ErrorTest   
> Error  Unit  Change
> ClassDescFactories.ReferenceOnly.ofNested   6 209,853 ± 132,525  22,017 ± 
> 0,573 ns/op   9,53x (p = 0,000*)
>   * = significant
> 
> 
> The optimizations could be split out and PRd independently, but I think they 
> are simple enough to include in this refactoring.

Claes Redestad has updated the pull request incrementally with one additional 
commit since the last revision:

  Use sb.repeat, consolidate

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19105/files
  - new: https://git.openjdk.org/jdk/pull/19105/files/6d5df18e..b0aca160

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=19105&range=09
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19105&range=08-09

  Stats: 8 lines in 1 file changed: 1 ins; 5 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/19105.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19105/head:pull/19105

PR: https://git.openjdk.org/jdk/pull/19105

Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v9]

[Claes Redestad]

On Tue, 14 May 2024 20:20:59 GMT, Chen Liang  wrote:

>> Claes Redestad has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Add microbenchmark for ClassDesc methods + a few optimizations
>
> src/java.base/share/classes/java/lang/constant/ClassDesc.java line 226:
> 
>> 224: for (int i = 0; i < rank; i++) {
>> 225: sb.append('[');
>> 226: }
> 
> Would `sb.repeat("[", rank);` be better here?
> 
> Also for parity with `arrayType()` I recommend moving this building code to 
> after the void check.

Oh, that's new (in 21). I was comparing against `append("[".repeat(rank))` 
which had a cost here. `sb.repeat('[', rank)` is in the same ballpark and 
simpler. Fixing.


Benchmark   Mode  Cnt Score Error   
Units
ClassDescMethods.arrayType2 avgt   1531,568 ±   2,108   
ns/op
ClassDescMethods.arrayType2:gc.alloc.rate   avgt   15  7758,056 ± 474,378  
MB/sec
ClassDescMethods.arrayType2:gc.alloc.rate.norm  avgt   15   256,000 ±   0,001   
 B/op


I'll align the code to construct `newDesc` before the void check in both 
methods.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1601088246