Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a secure by default configuration [v8]

[Alan Bateman]

On Thu, 16 May 2024 22:20:39 GMT, Joe Wang  wrote:

>> Add two sample configuration files:
>> 
>>   jaxp-strict.properties: used to set strict configuration, stricter than 
>> jaxp.properties in previous versions such as JDK 22
>> 
>>>   jaxp-compat.properties: used to regain compatibility from any more 
>>> restricted configuration than previous versions such as JDK 22
>> 
>> Updated 5/16/2024
>> 
>> Design change:
>> The design is changed to include in the JDK two configuration files that are 
>> the default jaxp.properties and jaxp-strict.properties, instead of three, 
>> dropping jaxp-compat.properties.
>
> Joe Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   remove jaxp-compat.properties from the list

make/modules/java.xml/Copy.gmk line 37:

> 35: JAXPPROPFILE_TARGET_FILES := $(subst 
> $(JAXPPROPFILE_SRC_DIR),$(CONF_DST_DIR),$(JAXPPROPFILE_SRCS))
> 36: 
> 37: $(CONF_DST_DIR)/%: $(JAXPPROPFILE_SRC_DIR)/%

The make file changes to copy the properties files look okay but I'm curious 
about why the naming changes from "XML" to "JAXPPROFILE".

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1604383246

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

[Roger Riggs]

Hi Aman,

You may also run into hidden classes (JEP 371: Hidden Classes) that 
allow classes to be defined, at runtime, without names.
It has been proposed to use them for generated proxies but that hasn't 
been implemented yet.
There are benefits to having nameless classes, because they can't be 
referenced by name, only as a capability, they can be better encapsulated.


fyi, Roger Riggs


On 5/16/24 8:11 AM, Aman Sharma wrote:


Hi,


Thanks for your response, Liang!


> I think you meant CVE-2021-42392 instead of 2022.


Sorry of the error. I indeed meant CVE-2021-42392 
.



> Leyden mainly avoids this unstable generation by performing a 
training run to collect classes loaded



Would love to know the details of Project Leyden and how they worked 
so far to focus on this goal. In our case, the training run is the 
test suite.



> GeneratedConstructorAccessor is already retired by JEP 416 [2] in 
Java 18



I did see them not appearing in my allowlist when I ran my study 
subject (Apache PDFBox) with Java 21. Thanks for letting me know about 
this JEP. I see they are re-implemented with method handles.



> How are you checking the classes?


To detect runtime generated code, we have javaagent that is hooked 
statically to the test suite execution. It gives us all classes that 
that is loaded post the JVM and the javaagent are loaded. So we only 
check the classes loaded for the purpose of running the application. 
This is also why we did not choose -agentlib as it would give classes 
for the setting up JVM and javaagent and we the user of our tool must 
the classes they load.



Next, we have a `ClassFileTransformer` hook in the agent where we 
produce the checksum using the bytecode. And we compare the checksum 
with the one existing in the allowlist. The checksum computation 
algorithm is same for both steps. Let me describe how I compute the 
checksum.



 1. I get the CONSTANT_Class_info

entry corresponding to `this_class` and rewrite the
CONSTANT_Utf8_info

corresponding to a fix String constant, say "foo".
 2. Since, the name of the class is used to refer to its types members
(fields/method), I get all CONSTANT_Fieldref_info

and if its `class_index` corresponds to the old `this_class`, we
rewrite the UTF8 value of class_index to the same constant "foo".
 3. Next, since the naming of the fields, in Proxy classes, are also
suffixed by numbers, for example, `private static Method m4`, we
rewrite the UTF8 value of name in the CONSTANT_NameAndType_info

.
 4. These fields can also have a random order so we simply sort the
entire byte code using `Arrays.sort(byte[])` to eliminate any
differences due to ordering of fields/methods.
 5. Simply sorting the byte array still had minute differences. I
could not understand why they existed even though values in
constant pool of the bytecode in allowlist and at runtime were
exactly the same after rewriting. The differences existed in the
bytes of the Code attribute of methods. I concluded that the bytes
stored some position information. To avoid this, I created a
subarray where I considered the bytes corresponding to
`CONSTANT_Utf8_info.bytes` only. Computing a checksum for it
resulted in the same checksums for both classfiles.


Let's understand the whole approach with an example of Proxy class.

`
public  final  class  $Proxy42  extends  Proxy  implements  
org.apache.logging.log4j.core.config.plugins.Plugin  {
`

The will go in the allowlist as "Proxy_Plugin: ".

When the same class is intercepted at runtime, say "$Proxy10", we look 
for "Proxy_Plugin" in the allowlist and since the checksum algorithm 
is same in both cases, we get a match and let the class load.


This approach has seemed to work well for Proxy classes, Generated 
Constructor Accessor (which is removed as you said). I also looked at 
the species generated by method handles. I did not notice any 
modification in them. Their name generation seemed okay to me. If some 
new Species are generated, it is of course detected since it is not in 
the allowlist.


I have not looked into LambdaMetafactory because I did not encounter 
it as a problem so far, but I am aware its name generation is also 
unstable. I have run my approach only a few projects only. And for 
hidden classes, I assume the the agent won't be able to intercept them 
so detecting them would be really hard.



Regards,
Aman Sharma

PhD Student
KTH Royal Institute of Technology
School of Electrical Engineering and Computer Science (EECS)
Department of Theoretical Computer Science (TCS)

Re: RFR: 8331224: ClassCastException in ObjectInputStream hides ClassNotFoundException

[Stuart Marks]

On Wed, 1 May 2024 18:43:21 GMT, Roger Riggs  wrote:

> The issue reported a ClassCastException "cannot assign instance of 
> java.util.CollSer to field of type java.util.Map"
> while deserializing an object referring to an immutable Map that contained a 
> reference to a class that was not available.
> Immutable Collections such as Map utilize a serialization proxy in their 
> serialized form.
> During deserialization the serialization proxy (a private implementation 
> class) was attempted to be set in a field resulting in the 
> ClassCastException. The ClassCastException and bug hid the ClassCastException 
> that should have been thrown.
> 
> When reading record fields or fields of a class, the results of 
> deserialization of individual fields are recorded as dependencies of the 
> object being constructed.
> The apparent bug is that the summary of those dependencies is not checked 
> between reading the fields and invoking the constructor to create the record 
> or assigning the fields to an object being constructed.

OK, I finally went through the changes. First, it's correct to assign the field 
values only when passHandle is not marked with an exception. It's fairly subtle 
but if passHandle is marked with an exception, the exception will be thrown by 
one of the methods farther up the call stack. Second, the test looks pretty 
comprehensive.

The main difficulty I had is not with the changes here but that the code in 
this area is handling rather too many cases, including: record/ordinary-class, 
has vs no special read method, read data into an object vs skipping data, and 
probably a few other cases. I'm not sure what, if anything, should be done 
about this. But in any case this should be a subject of a separate conversation.

-

Marked as reviewed by smarks (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19043#pullrequestreview-2062163253

Re: RFR: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic [v9]

[Sandhya Viswanathan]

On Fri, 10 May 2024 00:19:32 GMT, Volodymyr Paprotski  wrote:

>> Performance. Before:
>> 
>> Benchmark(algorithm)  (dataSize)  (keyLength)  
>> (provider)   Mode  Cnt ScoreError  Units
>> SignatureBench.ECDSA.signSHA256withECDSA1024  256
>>   thrpt3  6443.934 ±  6.491  ops/s
>> SignatureBench.ECDSA.signSHA256withECDSA   16384  256
>>   thrpt3  6152.979 ±  4.954  ops/s
>> SignatureBench.ECDSA.verify  SHA256withECDSA1024  256
>>   thrpt3  1895.410 ± 36.979  ops/s
>> SignatureBench.ECDSA.verify  SHA256withECDSA   16384  256
>>   thrpt3  1878.955 ± 45.487  ops/s
>> Benchmark(algorithm)  
>> (keyLength)  (kpgAlgorithm)  (provider)   Mode  Cnt ScoreError  Units
>> o.o.b.j.c.full.KeyAgreementBench.EC.generateSecret  ECDH  
>> 256  EC  thrpt3  1357.810 ± 26.584  ops/s
>> o.o.b.j.c.small.KeyAgreementBench.EC.generateSecret ECDH  
>> 256  EC  thrpt3  1352.119 ± 23.547  ops/s
>> Benchmark  (isMontBench)   Mode  Cnt Score
>> Error  Units
>> PolynomialP256Bench.benchMultiply  false  thrpt3  1746.126 ± 
>> 10.970  ops/s
>> 
>> Performance, no intrinsic:
>> 
>> Benchmark(algorithm)  (dataSize)  (keyLength)  
>> (provider)   Mode  Cnt Score Error  Units
>> SignatureBench.ECDSA.signSHA256withECDSA1024  256
>>   thrpt3  6529.839 ±  42.420  ops/s
>> SignatureBench.ECDSA.signSHA256withECDSA   16384  256
>>   thrpt3  6199.747 ± 133.566  ops/s
>> SignatureBench.ECDSA.verify  SHA256withECDSA1024  256
>>   thrpt3  1973.676 ±  54.071  ops/s
>> SignatureBench.ECDSA.verify  SHA256withECDSA   16384  256
>>   thrpt3  1932.127 ±  35.920  ops/s
>> Benchmark(algorithm)  
>> (keyLength)  (kpgAlgorithm)  (provider)   Mode  Cnt ScoreError  Units
>> o.o.b.j.c.full.KeyAgreementBench.EC.generateSecret  ECDH  
>> 256  EC  thrpt3  1355.788 ± 29.858  ops/s
>> o.o.b.j.c.small.KeyAgreementBench.EC.generateSecret ECDH  
>> 256  EC  thrpt3  1346.523 ± 28.722  ops/s
>> Benchmark  (isMontBench)   Mode  Cnt Score
>> Error  Units
>> PolynomialP256Bench.benchMultiply   true  thrpt3  1919.57...
>
> Volodymyr Paprotski has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   whitespace

src/hotspot/cpu/x86/stubGenerator_x86_64_poly_mont.cpp line 168:

> 166:   XMMRegister broadcast5 = xmm24;
> 167:   KRegister limb0 = k1;
> 168:   KRegister limb5 = k2;

limb5 and select are not being used anymore.

src/hotspot/cpu/x86/stubGenerator_x86_64_poly_mont.cpp line 185:

> 183:   __ evmovdquq(modulus, allLimbs, ExternalAddress(modulus_p256()), 
> false, Assembler::AVX_512bit, rscratch);
> 184: 
> 185:   // A = load(*aLimbs)

A little bit more description in comments on what the load step involves would 
be helpful. e.g. Load upper 4 limbs, shift left by 1 limb using perm, or in the 
lowest limb.

src/hotspot/cpu/x86/stubGenerator_x86_64_poly_mont.cpp line 270:

> 268:   __ push(r14);
> 269:   __ push(r15);
> 270: 

No need to save/restore  rbx, r12, r14, r15.  Only r13 is used as temp in 
montgomeryMultiply(aLimbs, bLimbs, rLimbs). That too could be easily changed to 
r8.

src/hotspot/cpu/x86/stubGenerator_x86_64_poly_mont.cpp line 286:

> 284:   __ mov(aLimbs, c_rarg0);
> 285:   __ mov(bLimbs, c_rarg1);
> 286:   __ mov(rLimbs, c_rarg2);

We could directly call montgomeryMultiply(c_rarg0, c_rarg1, c_rarg2) then these 
moves are not necessary.

src/hotspot/cpu/x86/vm_version_x86.cpp line 1370:

> 1368: 
> 1369: #ifdef _LP64
> 1370:   if (supports_avx512ifma() && supports_avx512vlbw() && MaxVectorSize 
> >= 64) {

No need to tie the intrinsic to MaxVectorSize setting.

src/hotspot/share/opto/library_call.cpp line 7564:

> 7562: 
> 7563:   if (!stubAddr) return false;
> 7564:   if (stopped())  return true;

Line 7564 seems redundant here as there is no range check or anything like that 
 before this.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1604169603
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1604141586
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1604174141
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1604175443
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1603792252
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1603865712

Re: RFR: 8332154: Memory leak in SynchronousQueue

[Konstantin Nisht]

On Thu, 16 May 2024 15:06:45 GMT, Viktor Klang  wrote:

>> Local testing seems to indicate that this fix (which mirrors what's done in 
>> the FIFO mode) addresses the problem.
>> 
>> But with that said, I haven't come up with a decent way of adding some form 
>> of regression test. Suggestions are most welcome. /cc @DougLea
>
> @AlanBateman @DougLea Reviews are most welcome :)

@viktorklang-ora We managed to reproduce it quite reliably by running the test 
from the bug report multiple times (in our case, it was 100). I understand this 
does not make a 100% stable regression reproducer, but in case of multiple runs 
the probability of false positive in quite low.

-

PR Comment: https://git.openjdk.org/jdk/pull/19271#issuecomment-2116230751

Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a secure by default configuration [v8]

[Joe Wang]

> Add two sample configuration files:
> 
>   jaxp-strict.properties: used to set strict configuration, stricter than 
> jaxp.properties in previous versions such as JDK 22
> 
>>   jaxp-compat.properties: used to regain compatibility from any more 
>> restricted configuration than previous versions such as JDK 22
> 
> Updated 5/16/2024
> 
> Design change:
> The design is changed to include in the JDK two configuration files that are 
> the default jaxp.properties and jaxp-strict.properties, instead of three, 
> dropping jaxp-compat.properties.

Joe Wang has updated the pull request incrementally with one additional commit 
since the last revision:

  remove jaxp-compat.properties from the list

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18831/files
  - new: https://git.openjdk.org/jdk/pull/18831/files/f3af4ae9..cf4df792

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18831=07
 - incr: https://webrevs.openjdk.org/?repo=jdk=18831=06-07

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

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

Re: RFR: 8331224: ClassCastException in ObjectInputStream hides ClassNotFoundException

[Stuart Marks]

On Wed, 1 May 2024 18:43:21 GMT, Roger Riggs  wrote:

> The issue reported a ClassCastException "cannot assign instance of 
> java.util.CollSer to field of type java.util.Map"
> while deserializing an object referring to an immutable Map that contained a 
> reference to a class that was not available.
> Immutable Collections such as Map utilize a serialization proxy in their 
> serialized form.
> During deserialization the serialization proxy (a private implementation 
> class) was attempted to be set in a field resulting in the 
> ClassCastException. The ClassCastException and bug hid the ClassCastException 
> that should have been thrown.
> 
> When reading record fields or fields of a class, the results of 
> deserialization of individual fields are recorded as dependencies of the 
> object being constructed.
> The apparent bug is that the summary of those dependencies is not checked 
> between reading the fields and invoking the constructor to create the record 
> or assigning the fields to an object being constructed.

src/java.base/share/classes/java/io/ObjectInputStream.java line 2376:

> 2374: if (handles.lookupException(passHandle) != null) {
> 2375: return null; // slot marked with exception, don't 
> create record
> 2376: }

It's proper to avoid creating a record instance in this case. However, 
returning null is new behavior; this initially seemed a bit odd. The calling 
method `readOrdinaryObject()` does allow a null return if the class couldn't be 
resolved, and the body of `readOrdinaryObject()` does allow `obj` to be null 
throughout. So, returning `null` from here is correct, though it took me a 
while to puzzle out. :-)

I'd suggest adding some docs to `readRecord()` to indicate that it returns the 
record instance if it could be created successfully, null if the record class 
couldn't be resolved, or throws an error or other exception if one occurred 
when instantiation was attempted.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19043#discussion_r1604052829

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

[Scott Gibbons]

On Tue, 16 Jan 2024 12:09:11 GMT, Jatin Bhateja  wrote:

>> Scott Gibbons has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 22 commits:
>> 
>>  - Merge branch 'openjdk:master' into indexof
>>  - Merge branch 'openjdk:master' into indexof
>>  - Addressing review comments.
>>  - Fix for JDK-8321599
>>  - Support UU IndexOf
>>  - Only use optimization when EnableX86ECoreOpts is true
>>  - Fix whitespace
>>  - Merge branch 'openjdk:master' into indexof
>>  - Comments; added exhaustive-ish test
>>  - Subtracting 0x10 twice.
>>  - ... and 12 more: https://git.openjdk.org/jdk/compare/8e12053e...3e58d0c2
>
> src/hotspot/share/opto/library_call.cpp line 1229:
> 
>> 1227:   } else {
>> 1228: result = make_indexOf_node(src_start, src_count, tgt_start, 
>> tgt_count,
>> 1229:result_rgn, result_phi, ae);
> 
> Existing routines emits IR to handle following special cases.
> 
> tgt_cnt > src_cnt return -1
> tgt_cnt == 0 return 0.
> 
> Should we not be preserving those check before calling stub ?
> 
> As of now these checks are part of stub and doing them in JIT code will save 
> call overhead.

Working on this.  Trying to develop my IR chops.  However, this is optimizing 
for a very small percentage of calls, so there will be unnoticable effect on 
overall performance.  There will only be savings for calls that have needle 
length == 0 (probably zero calls do this) or haystack length < needle length 
(maybe, but highly unlikely).

-

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

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

[Scott Gibbons]

On Mon, 6 May 2024 20:56:36 GMT, Sandhya Viswanathan  
wrote:

>> 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/macroAssembler_x86.cpp line 1174:
> 
>> 1172: // Alignment specifying the maximum number of allowed bytes to pad.
>> 1173: // If padding > max, no padding is inserted.
>> 1174: void MacroAssembler::p2align(int modulus, int maxbytes) {
> 
> We could pass offset() as an argument to p2align. Basically have three 
> arguments to p2align(modulus, target, maxbytes). Also maybe rename p2align as 
> align then?

Removed p2align().  Was never used and was a remnant of prior implementation 
attempt.

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 208:
> 
>> 206:   
>> 
>> 207:   
>> 
>> 208:   if (VM_Version::supports_avx2()) {  // AVX2 version
> 
> Instead of the if check here, it would be better to do an assert here:
> assert (VM_Version::supports_avx2(), "Needs AVX2 support");

Done

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 238:
> 
>> 236: const Register needle   = rdx;
>> 237: const Register needle_len   = rcx;
>> 238: 
> 
> This is the calling convention on Linux. How is windows platform handled?

The entry code switches Windows calling convention into Linux calling 
convention by moving/saving registers, which are properly restored on function 
exit.  This makes register tracking easier.

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 260:
> 
>> 258: // const XMMRegister save_rcx  = xmm11;
>> 259: // const XMMRegister save_r8   = xmm12;
>> 260: 
> 
> This could be removed?

Done

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 279:
> 
>> 277: fnptrs[isLL   ? StrIntrinsicNode::LL
>> 278:: isUU ? StrIntrinsicNode::UU
>> 279:   : StrIntrinsicNode::UL] = __ pc();
> 
> Could this not be simplified as:
>  fnptrs[ae] = __ pc();

Done

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 314:
> 
>> 312: 
>> 313: // needle_len is in elements, not bytes, for UTF-16
>> 314: __ cmpq(needle_len, isUU ? OPT_NEEDLE_SIZE_MAX / 2 : 
>> OPT_NEEDLE_SIZE_MAX);
> 
> OPT_NEEDLE_SIZE_MAX is an odd number (set to 5), should that have been an 
> even number?

Removed OPT_NEEDLE_SIZE_MAX and replaced with constant == 6.

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 383:
> 
>> 381: {
>> 382:   Label L_short;
>> 383: 
> 
> A comment here:
> // Broadcast the beginning of needle into a vector register.

Done

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 390:
> 
>> 388: __ vpbroadcastb(byte_0, Address(needle, 0), 
>> Assembler::AVX_256bit);
>> 389:   }
>> 390: 
> 
> A comment here:
> // Broadcast the end of needle into a vector register. This step is not 
> needed for single element needle.

Done

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 418:
> 
>> 416:   __ cmpq(haystack_len, 0x10);
>> 417:   __ ja_b(L_moreThan16);
>> 418: 
> 
> An assert here to check for header size >= 16 would be good. 
> Also a comment here would he good, something like: 
> // Copy 16 or 32 bytes prior to haystack end onto stack 
> // This will possibly including some object header bytes when haystack length 
> is less than 16 or 32 bytes // Set the new haystack address to beginning of 
> copied haystack on stack adjusting for extra bytes copied

I don't know how to assert header size >= 16 bytes, so I'll add a comment 
stating such.  If you can tell me how to assert, I'll add that code in place of 
the comment.

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 498:
> 
>> 496: 
>> 497:   // big_case_loop_helper will fall through to this point if one or 
>> more potential matches are found
>> 498:   // The mask will have a bitmask indicating the position of the 
>> potential matches within the haystack
> 
> If no potential match, which label does the big_case_loop_helper jump to?

Added comment

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 517:
> 
>> 515:   __C2 arrays_equals(false, haystackStart, firstNeedleCompare, 
>> compLen, retval, rScratch, xmm_tmp3, xmm_tmp4,
>> 516:  false /* char */, knoreg);
>> 517:   __ testl(retval, retval);
> 
> Since this is byte compare even for isU, the retval here could be a 64-bit 
> quantity so the testl should be a testq.

`arrays_equals` returns a boolean value of `0` for not found and `1` for found 
using `movl(result, 0/1)` so testl is appropriate here.

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 553:
> 
>> 551: //  Haystack always copied to stack, so 32-byte reads OK
>> 552: //  Haystack length < 32
>> 553: //  10 < needle length < 

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

[Scott Gibbons]

On Tue, 7 May 2024 17:25:04 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
>
> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4492:
> 
>> 4490: 
>> 4491: // Compare char[] or byte[] arrays aligned to 4 bytes or substrings.
>> 4492: void C2_MacroAssembler::arrays_equals(bool is_array_equ, Register ary1,
> 
> I liked the old style better, fewer longer lines.. same for rest of the 
> changes in this file.

Done.

> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4594:
> 
>> 4592: #endif //_LP64
>> 4593: bind(COMPARE_WIDE_VECTORS);
>> 4594: vmovdqu(vec1, Address(ary1, limit,
> 
> create a local scale variable instead of ternary operators. Used several 
> times.

Done

> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 4250:
> 
>> 4248:   generate_chacha_stubs();
>> 4249: 
>> 4250:   if ((UseAVX == 2) && EnableX86ECoreOpts && 
>> VM_Version::supports_avx2()) {
> 
> Just `if (EnableX86ECoreOpts)`?

I think all 3 should be specified, even if `EnableX86ECoreOpts` checks.  This 
is for clarity of intent.

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 391:
> 
>> 389:   }
>> 390: 
>> 391:   __ cmpq(needle_len, isU ? 2 : 1);
> 
> Can we remove this comparison? i.e.
> - broadcast first and last character unconditionally (same character). Or
> - move broadcasts 'down' into individual cases..
> There is already specialized code to handle needle of size 1.. This adds 
> extra pathlength. (Will we actually call this intrinsic for needle_size==1? 
> Assume length>=2?)

At this point in the code it is entirely possible for needle size to be == 1, 
but only in the case where haystack size is > 32 bytes.  Moving the broadcasts 
'down' into individual cases increases code size by 14 broadcast instructions.

Seems like the best option is to just remove the compare and branch, 
broadcasting the first needle element twice.

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1365:
> 
>> 1363:   // Compare first byte of needle to haystack
>> 1364:  vpcmpeq(cmp_0, byte_0, Address(haystack, 0), 
>> Assembler::AVX_256bit);
>> 1365:   if (size != (isU ? 2 : 1)) {
> 
> `if (size != scale)`
> 
> Though in this case, `elem_size` might hold more meaning.

Done

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1372:
> 
>> 1370: 
>> 1371: if (bytesToCompare > 2) {
>> 1372:   if (size > (isU ? 4 : 2)) {
> 
> `if (size > 2*scale)`?

Done

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1373:
> 
>> 1371: if (bytesToCompare > 2) {
>> 1372:   if (size > (isU ? 4 : 2)) {
>> 1373: if (doEarlyBailout) {
> 
> Is there a big perf difference when `doEarlyBailout` is enabled? And/or just 
> for this function?
> 
> (i.e. removing `doEarlyBailout` in this function will mean less pathlength. 
> Feels like a few extra vpands should be cheap enough.)

I removed the macro DO_EARLY_BAILOUT and assumed it to be true.  There's not 
much difference (if any) in performance, so we maybe ought to consider not 
bailing out early.  I'll consider not bailing out and let you know how 
performance is impacted.

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1469:
> 
>> 1467: 
>> 1468:   if (isU && (size & 1)) {
>> 1469: __ emit_int8(0xcc);
> 
> This should also be an `assert()` to catch this at compile-time.

Although assert is technically runtime (;-)) I'll change these.  They were put 
in to double-check while debugging.

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1633:
> 
>> 1631:   if (isU) {
>> 1632: if ((size & 1) != 0) {
>> 1633:   __ emit_int8(0xcc);
> 
> Compile-time assert to ensure this code is never called instead?

Done

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1889:
> 
>> 1887: //  r13 = (needle length - 1)
>> 1888: //  r14 = 
>> 1889: //  r15 = unused
> 
> There is quite a bit of redundancy in register usage. Its not incorrect, but 
> looks odd. Not clear if this duplication can easily be removed (or if/why 
> needed). 
> 
> //  rbx = 
> //  rdi = 
> //  rdx = 
> //  r14 = 
> //  rcx = haystack length
> //  rsi = haystack length
> //  r12 = needle length
> //  r13 = (needle length - 1)
> //  r10 = hs_len - needle len
> //  rbp = -1
> 
> //  rax = unused
> //  r11 = unused
> //  r8  = unused
> //  r9  = unused
> //  r15 = unused
> 
> 
> (Could this comment be out-of-sync with the code? Looks like only rbx, r14 
> and temps out of unused registers are used few lines down)

This comment provides the full register state upon entry to each of the cases 
of the switch.  The duplication is an artifact of the decisions made in setup 
code (like checking ranges, etc.).  Each case can depend on the values of the 
registers to be as documented on entry.  It can use either rcx or rsi to get 
the 

Re: In support of Instant.minus(Instant)

[Stephen Colebourne]

I remain happy to see until() go into the JDK. Potentially on more
classes than the recent PR.

*If* Java gets operator overloading, and *if* it is based on a mapping
to method names, then *perhaps* there is a case to consider adding
minus(). But I don't feel now is the time.

Stephen


On Thu, 16 May 2024 at 17:50, Kurt Alfred Kluever  wrote:
>
> Hi all,
>
>
> Sorry for the delay here. I took a quick look at other popular date/time 
> libraries and there seems to be a fairly strong preference for minus (JS/TS's 
> Temporal library is the only other date/time library that I found which uses 
> until for this concept):
>
>
> Abseil's Time (C++) overloads the - (minus) operator to allow for end - start
>
> NodaTime (.NET) defines end.minus(start) (which also allows end - start)
>
> Kotlinx.datetime defines end.minus(start) (which also allows end - start)
>
> Python's date/time API allows end - start
>
> Temporal (JS/TS) defines start.until(end)
>
>
> The first 4 APIs support both instant - instant = duration and instant - 
> duration = instant — I find this quite natural.
>
>
> Additionally, "until" seems to imply it will return a positive duration. This 
> is fine if you're certain that "end" is after "start". However, it becomes 
> very confusing otherwise. E.g., if you ask: "how long from when you were 
> married until now?". If you were married 4 years ago, that makes sense. But 
> if your wedding is in 3 months, then the result is a negative duration (which 
> often cause confusion). If you ask the question the other way around ("how 
> long from now until your wedding day?"), that works if your wedding is in the 
> future, but not so much if you were married 4 years ago (again, you'll get a 
> negative duration).
>
>
> Additionally, the word "until" seems to imply a range with defined start/end 
> points; e.g., "I'm out from Tuesday until next Thursday" --- that's more of a 
> Range (or an Interval which java.time doesn't have – but of course 
> JodaTime did), not just a span of time (a Duration).
>
>
> Does that sway anybody's opinion?
>
>
> Thanks,
>
> -kak
>
>
>
> On Mon, May 13, 2024 at 1:38 PM Naoto Sato  wrote:
>>
>> Hi,
>>
>> With Stephen/Roger's comments, as well as Kevin's observation that
>> until(end) has a good argument ordering that is easy to understand, I'd
>> still propose `until()`. Please post if you have further comments.
>>
>> Naoto
>>
>> On 5/3/24 6:39 AM, Roger Riggs wrote:
>> > Hi,
>> >
>> > I would also reinforce Stephen's early observation that the pattern for
>> > "until" methods in java.time includes those of the XXXDate classes, with
>> > a single Temporal parameter.  Period and Duration are similar values
>> > holding relative TemporalAmounts.
>> >
>> >  public Period until(ChronoLocalDate endDateExclusive)
>> >
>> > In addition to Instant, the LocalTime class might also benefit from adding:
>> >
>> >  public Duration until(LocalTime endExclusive)`
>> >
>> > The API design of java.time included an emphasis on consistent naming
>> > across the packages.
>> >
>> > Regards, Roger
>> >
>> >
>> > On 5/2/24 4:01 PM, Naoto Sato wrote:
>> >> `Temporal` interface is clear that its `minus` methods return objects
>> >> of the same `Temporal` type, and `until` calculates the amount of time
>> >> until another `Temporal` type. Introducing `Instant.minus` that
>> >> returns `Duration` would be confusing to me.
>> >>
>> >> Naoto
>> >>
>> >> On 5/2/24 10:41 AM, Éamonn McManus wrote:
>> >>> I'd say too that this makes intuitive sense based on algebra. If we
>> >>> have:
>> >>> /instant1/ + /duration/ = /instant2/
>> >>> then we can subtract /duration/ from both sides:
>> >>> /instant1 = instant2 - duration/
>> >>> or we can subtract /instant1/ from both sides:
>> >>> /duration = instant2 - instant1/
>> >>>
>> >>> There's no manipulation we can do that would cause us to try to add
>> >>> instants together, and it's a bit surprising for the API to allow the
>> >>> first subtraction but not the second.
>> >>> I also think that if I see instant2.minus(instant1) it's immediately
>> >>> obvious to me what that means, while instant1.until(instant2) seems
>> >>> both less discoverable and less obvious.
>> >>>
>> >>> On Thu, 2 May 2024 at 10:29, Louis Wasserman > >>> > wrote:
>> >>>
>> >>> That doesn't follow for me at all.
>> >>>
>> >>> The structure formed by Instants and Durations is an affine space
>> >>> , with
>> >>> instants the points and durations the vectors.  (An affine space is
>> >>> a vector space without a distinguished origin, which of course
>> >>> Instants don't have.)  It is 100% standard to use the minus sign for
>> >>> the operation "point - point = vector," even when "point + point" is
>> >>> not defined, and to use all the other standard idioms for
>> >>> subtraction; the Wikipedia article uses "subtraction" and
>> >>> "difference" ubiquitously.
>> 

Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a secure by default configuration [v7]

[Lance Andersen]

On Thu, 16 May 2024 18:53:40 GMT, Joe Wang  wrote:

>> Add two sample configuration files:
>> 
>>   jaxp-strict.properties: used to set strict configuration, stricter than 
>> jaxp.properties in previous versions such as JDK 22
>> 
>>>   jaxp-compat.properties: used to regain compatibility from any more 
>>> restricted configuration than previous versions such as JDK 22
>> 
>> Updated 5/16/2024
>> 
>> Design change:
>> The design is changed to include in the JDK two configuration files that are 
>> the default jaxp.properties and jaxp-strict.properties, instead of three, 
>> dropping jaxp-compat.properties.
>
> Joe Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   The JDK provides two configuration files instead of three. Updated 
> jaxp-strict.properties to reflect the change. Removed jaxp-compat. Updated 
> jaxp.properties with properties the same as in jaxp-strict, setting to 
> default values.

src/java.xml/share/classes/module-info.java line 409:

> 407:  *  {@code jaxp.properties}
> 408:  *  {@code 
> jaxp-strict.properties}
> 409:  *  {@code 
> jaxp-compat.properties}

This line should be removed

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1603907393

Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a secure by default configuration [v7]

[Joe Wang]

> Add two sample configuration files:
> 
>   jaxp-strict.properties: used to set strict configuration, stricter than 
> jaxp.properties in previous versions such as JDK 22
> 
>   jaxp-compat.properties: used to regain compatibility from any more 
> restricted configuration than previous versions such as JDK 22

Joe Wang has updated the pull request incrementally with one additional commit 
since the last revision:

  The JDK provides two configuration files instead of three. Updated 
jaxp-strict.properties to reflect the change. Removed jaxp-compat. Updated 
jaxp.properties with properties the same as in jaxp-strict, setting to default 
values.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18831/files
  - new: https://git.openjdk.org/jdk/pull/18831/files/af351a4d..f3af4ae9

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18831=06
 - incr: https://webrevs.openjdk.org/?repo=jdk=18831=05-06

  Stats: 191 lines in 6 files changed: 8 ins; 157 del; 26 mod
  Patch: https://git.openjdk.org/jdk/pull/18831.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18831/head:pull/18831

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

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

[Alan Bateman]

On Thu, 16 May 2024 12:23:44 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:
> 
>   Add note on --illegal-native-access default value in the launcher help

src/java.base/share/classes/java/lang/System.java line 2023:

> 2021:  * @throws NullPointerException if {@code filename} is {@code 
> null}
> 2022:  * @throws IllegalCallerException If the caller is in a module 
> that
> 2023:  * does not have native access enabled.

The exception description is fine, just noticed the other exception 
descriptions start with a lowercase "if", this one is different.

src/java.base/share/man/java.1 line 587:

> 585: \f[V]deny\f[R]: This mode disables all illegal native access except for
> 586: those modules enabled by the \f[V]--enable-native-access\f[R]
> 587: command-line option.

"This mode disable all illegal native access except for those modules enabled 
the --enable-native-access command-line option". 

This can be read to mean that modules granted native access with the command 
line option is also illegal native access An alternative is to make the second 
part of the sentence a new sentence, something like "Only modules enabled by 
the --enable-native-access command line option may perform native access.

-

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

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

[Ioi Lam]

On Thu, 16 May 2024 03:31:05 GMT, Chen Liang  wrote:

>> Ioi Lam has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   (1) comments from @liach; (2) added javadoc; (3) Use 
>> createTestJavaProcessBuilder() instead of 
>> createLimitedTestJavaProcessBuilder()
>
> 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;

Thanks for the suggestion. I used your `arraycopy` version. I also added some 
javadocs about the intended use of these `concat()` methods.

-

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

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

[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.

Ioi Lam has updated the pull request incrementally with one additional commit 
since the last revision:

  (1) comments from @liach; (2) added javadoc; (3) Use 
createTestJavaProcessBuilder() instead of createLimitedTestJavaProcessBuilder()

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19256/files
  - new: https://git.openjdk.org/jdk/pull/19256/files/10cf69d1..354665a3

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19256=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=19256=00-01

  Stats: 38 lines in 2 files changed: 17 ins; 8 del; 13 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: In support of Instant.minus(Instant)

[Kurt Alfred Kluever]

Hi all,

Sorry for the delay here. I took a quick look at other popular date/time
libraries and there seems to be a fairly strong preference for minus
(JS/TS's Temporal library is the only other date/time library that I found
which uses until for this concept):


   -

   Abseil's Time (C++) overloads the - (minus) operator
   
   to allow for end - start
   -

   NodaTime (.NET) defines
   

   end.minus(start) (which also allows end - start)
   -

   Kotlinx.datetime defines
   

   end.minus(start) (which also allows end - start)
   -

   Python's date/time API allows
    end
   - start
   -

   Temporal (JS/TS) defines
   
   start.until(end)


The first 4 APIs support both instant - instant = duration and instant -
duration = instant — I find this quite natural.

Additionally, "until" seems to imply it will return a positive duration.
This is fine if you're certain that "end" is after "start". However, it
becomes very confusing otherwise. E.g., if you ask: "how long from when you
were married until now?". If you were married 4 years ago, that makes
sense. But if your wedding is in 3 months, then the result is a negative
duration (which often cause confusion). If you ask the question the other
way around ("how long from now until your wedding day?"), that works if
your wedding is in the future, but not so much if you were married 4 years
ago (again, you'll get a negative duration).

Additionally, the word "until" seems to imply a range with defined
start/end points; e.g., "I'm out from Tuesday until next Thursday" ---
that's more of a Range (or an Interval which java.time doesn't
have – but of course JodaTime did), not just a span of time (a Duration).

Does that sway anybody's opinion?

Thanks,

-kak


On Mon, May 13, 2024 at 1:38 PM Naoto Sato  wrote:

> Hi,
>
> With Stephen/Roger's comments, as well as Kevin's observation that
> until(end) has a good argument ordering that is easy to understand, I'd
> still propose `until()`. Please post if you have further comments.
>
> Naoto
>
> On 5/3/24 6:39 AM, Roger Riggs wrote:
> > Hi,
> >
> > I would also reinforce Stephen's early observation that the pattern for
> > "until" methods in java.time includes those of the XXXDate classes, with
> > a single Temporal parameter.  Period and Duration are similar values
> > holding relative TemporalAmounts.
> >
> >  public Period until(ChronoLocalDate endDateExclusive)
> >
> > In addition to Instant, the LocalTime class might also benefit from
> adding:
> >
> >  public Duration until(LocalTime endExclusive)`
> >
> > The API design of java.time included an emphasis on consistent naming
> > across the packages.
> >
> > Regards, Roger
> >
> >
> > On 5/2/24 4:01 PM, Naoto Sato wrote:
> >> `Temporal` interface is clear that its `minus` methods return objects
> >> of the same `Temporal` type, and `until` calculates the amount of time
> >> until another `Temporal` type. Introducing `Instant.minus` that
> >> returns `Duration` would be confusing to me.
> >>
> >> Naoto
> >>
> >> On 5/2/24 10:41 AM, Éamonn McManus wrote:
> >>> I'd say too that this makes intuitive sense based on algebra. If we
> >>> have:
> >>> /instant1/ + /duration/ = /instant2/
> >>> then we can subtract /duration/ from both sides:
> >>> /instant1 = instant2 - duration/
> >>> or we can subtract /instant1/ from both sides:
> >>> /duration = instant2 - instant1/
> >>>
> >>> There's no manipulation we can do that would cause us to try to add
> >>> instants together, and it's a bit surprising for the API to allow the
> >>> first subtraction but not the second.
> >>> I also think that if I see instant2.minus(instant1) it's immediately
> >>> obvious to me what that means, while instant1.until(instant2) seems
> >>> both less discoverable and less obvious.
> >>>
> >>> On Thu, 2 May 2024 at 10:29, Louis Wasserman  >>> > wrote:
> >>>
> >>> That doesn't follow for me at all.
> >>>
> >>> The structure formed by Instants and Durations is an affine space
> >>> , with
> >>> instants the points and durations the vectors.  (An affine space is
> >>> a vector space without a distinguished origin, which of course
> >>> Instants don't have.)  It is 100% standard to use the minus sign
> for
> >>> the operation "point - point = vector," even when "point + point"
> is
> >>> not defined, and to use all the other standard idioms for
> >>> subtraction; the Wikipedia article uses "subtraction" and
> >>> "difference" ubiquitously.
> >>>
> >>> Personally, I'd be willing to live 

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

[Naoto Sato]

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)

The changeset included a workaround patch to the upstream JLine. An issue for 
it was created by Jan: https://github.com/jline/jline3/issues/982

-

PR Comment: https://git.openjdk.org/jdk/pull/18923#issuecomment-2115684085

Integrated: 8331202: Support for Duration until another Instant

[Naoto Sato]

On Mon, 29 Apr 2024 19:40:31 GMT, Naoto Sato  wrote:

> A new method on Instant to return the duration `until` another Instant is 
> suggested per the following discussion thread:
> 
> https://mail.openjdk.org/pipermail/core-libs-dev/2024-April/122131.html
> 
> A CSR has also been drafted.

This pull request has now been integrated.

Changeset: 25991516
Author:Naoto Sato 
URL:   
https://git.openjdk.org/jdk/commit/259915168d6656b1b8ddad03c377082d6a5224e5
Stats: 58 lines in 2 files changed: 55 ins; 0 del; 3 mod

8331202: Support for Duration until another Instant

Reviewed-by: joehw, scolebourne, rriggs

-

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

Re: RFR: 8331202: Support for Duration until another Instant [v3]

[Naoto Sato]

On Wed, 1 May 2024 17:21:20 GMT, Naoto Sato  wrote:

>> A new method on Instant to return the duration `until` another Instant is 
>> suggested per the following discussion thread:
>> 
>> https://mail.openjdk.org/pipermail/core-libs-dev/2024-April/122131.html
>> 
>> A CSR has also been drafted.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addressing CSR reviews

Hearing no further comments, integrating this PR.

-

PR Comment: https://git.openjdk.org/jdk/pull/19007#issuecomment-2115651450

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

[Olexandr Rotan]

Is it possible to make stable values and collections Serializable? I see
various applications for this feature in entity classes as a way to
preserve immutability of entity fields and at the same time not break
current JPA specifications. It is a *very* common task in commercial
development. Current workarounds lack either thread safety or performance,
and this looks like a really good solution for both of those problems.
However, unless StableValue is serializable, it is really unlikely that JPA
will adopt them (we have precedent with Optional).

On Thu, May 16, 2024 at 5:07 PM Per Minborg  wrote:

> On Thu, 16 May 2024 12:48:24 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:
> >
> >   Fix copyringht issues
>
> Here are some updated benchmark graphs where we sum two instance variables
> of different sorts (higher is better):
>
> ![image](
> https://github.com/openjdk/jdk/assets/7457876/d82561d6-e803-4345-b6d2-6b0402e60211
> )
>
> -
>
> PR Comment: https://git.openjdk.org/jdk/pull/18794#issuecomment-2115343828
>

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

[Aman Sharma]

Hi,


> have not looked into LambdaMetafactory because I did not encounter it as a 
> problem so far


It is possible that java agents are unable to intercept it. `-verbose:class` 
logs classes such as 
"org.apache.pdfbox.cos.COSDocument$$Lambda/0x7a80631a0d08".


Regards,
Aman Sharma

PhD Student
KTH Royal Institute of Technology
School of Electrical Engineering and Computer Science (EECS)
Department of Theoretical Computer Science (TCS)

https://algomaster99.github.io/

From: Aman Sharma
Sent: Thursday, May 16, 2024 2:11:59 PM
To: liangchenb...@gmail.com; core-libs-dev
Cc: Martin Monperrus
Subject: Re: Re: Deterministic naming of subclasses of `java/lang/reflect/Proxy`


Hi,


Thanks for your response, Liang!


> I think you meant CVE-2021-42392 instead of 2022.


Sorry of the error. I indeed meant 
CVE-2021-42392.


> Leyden mainly avoids this unstable generation by performing a training run to 
> collect classes loaded


Would love to know the details of Project Leyden and how they worked so far to 
focus on this goal. In our case, the training run is the test suite.


> GeneratedConstructorAccessor is already retired by JEP 416 [2] in Java 18


I did see them not appearing in my allowlist when I ran my study subject 
(Apache PDFBox) with Java 21. Thanks for letting me know about this JEP. I see 
they are re-implemented with method handles.


> How are you checking the classes?


To detect runtime generated code, we have javaagent that is hooked statically 
to the test suite execution. It gives us all classes that that is loaded post 
the JVM and the javaagent are loaded. So we only check the classes loaded for 
the purpose of running the application. This is also why we did not choose 
-agentlib as it would give classes for the setting up JVM and javaagent and we 
the user of our tool must the classes they load.


Next, we have a `ClassFileTransformer` hook in the agent where we produce the 
checksum using the bytecode. And we compare the checksum with the one existing 
in the allowlist. The checksum computation algorithm is same for both steps. 
Let me describe how I compute the checksum.


  1.  I get the 
CONSTANT_Class_info
 entry corresponding to `this_class` and rewrite the 
CONSTANT_Utf8_info
 corresponding to a fix String constant, say "foo".
  2.  Since, the name of the class is used to refer to its types members 
(fields/method), I get all 
CONSTANT_Fieldref_info
 and if its `class_index` corresponds to the old `this_class`, we rewrite the 
UTF8 value of class_index to the same constant "foo".
  3.  Next, since the naming of the fields, in Proxy classes, are also suffixed 
by numbers, for example, `private static Method m4`, we rewrite the UTF8 value 
of name in the 
CONSTANT_NameAndType_info.
  4.  These fields can also have a random order so we simply sort the entire 
byte code using `Arrays.sort(byte[])` to eliminate any differences due to 
ordering of fields/methods.
  5.  Simply sorting the byte array still had minute differences. I could not 
understand why they existed even though values in constant pool of the bytecode 
in allowlist and at runtime were exactly the same after rewriting. The 
differences existed in the bytes of the Code attribute of methods. I concluded 
that the bytes stored some position information. To avoid this, I created a 
subarray where I considered the bytes corresponding to 
`CONSTANT_Utf8_info.bytes` only. Computing a checksum for it resulted in the 
same checksums for both classfiles.

Let's understand the whole approach with an example of Proxy class.

`

public final class $Proxy42 extends Proxy implements 
org.apache.logging.log4j.core.config.plugins.Plugin {

`

The will go in the allowlist as "Proxy_Plugin: ".

When the same class is intercepted at runtime, say "$Proxy10", we look for 
"Proxy_Plugin" in the allowlist and since the checksum algorithm is same in 
both cases, we get a match and let the class load.

This approach has seemed to work well for Proxy classes, Generated Constructor 
Accessor (which is removed as you said). I also looked at the species generated 
by method handles. I did not notice any modification in them. Their name 
generation seemed okay to me. If some new Species are generated, it is of 
course detected since it is not in the allowlist.

I have not looked into LambdaMetafactory because I did not encounter it as a 
problem so far, but I am aware its name generation is also unstable. I have run 
my approach only a few 

Re: RFR: 8332154: Memory leak in SynchronousQueue

[Viktor Klang]

On Thu, 16 May 2024 14:54:52 GMT, Viktor Klang  wrote:

> Local testing seems to indicate that this fix (which mirrors what's done in 
> the FIFO mode) addresses the problem.
> 
> But with that said, I haven't come up with a decent way of adding some form 
> of regression test. Suggestions are most welcome. /cc @DougLea

@AlanBateman @DougLea Reviews are most welcome :)

-

PR Comment: https://git.openjdk.org/jdk/pull/19271#issuecomment-2115505240

RFR: 8332154: Memory leak in SynchronousQueue

[Viktor Klang]

Local testing seems to indicate that this fix (which mirrors what's done in the 
FIFO mode) addresses the problem.

But with that said, I haven't come up with a decent way of adding some form of 
regression test. Suggestions are most welcome. /cc @DougLea

-

Commit messages:
 - Memory leak in unfair mode of SynchronousQueue addressed

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

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

Re: RFR: 8331865: Consolidate size and alignment checks in LayoutPath [v2]

[Maurizio Cimadamore]

> When creating a nested memory access var handle, we ensure that the segment 
> is accessed at the correct alignment for the root layout being accessed. But 
> we do not ensure that the segment has at least the size of the accessed root 
> layout. Example:
> 
> 
> MemoryLayout LAYOUT = sequenceLayout(2, structLayout(JAVA_INT.withName("x"), 
> JAVA_INT.withName("y")));
> VarHandle X_HANDLE = LAYOUT.varHandle(PathElement.sequenceElement(), 
> PathElement.groupElement("x"));
> 
> 
> If I have a memory segment whose size is 12, I can successfully call X_HANDLE 
> on it with index 1, like so:
> 
> 
> MemorySegment segment = Arena.ofAuto().allocate(12);
> int x = (int)X_HANDLE.get(segment, 0, 1);
> 
> 
> This seems incorrect: after all, the provided segment doesn't have enough 
> space to fit _two_ elements of the nested structs. 
> 
> This PR adds checks to detect this kind of scenario earlier, thus improving 
> usability. To achieve this we could, in principle, just tweak 
> `LayoutPath::checkEnclosingLayout` and add the required size check.
> 
> But doing so will add yet another redundant check on top of the other already 
> added by [pull/19124](https://git.openjdk.org/jdk/pull/19124). Instead, this 
> PR rethinks how memory segment var handles are created, in order to eliminate 
> redundant checks.
> 
> The main observation is that size and alignment checks depend on an 
> _enclosing_ layout. This layout *might* be the very same value layout being 
> accessed (this is the case when e.g. using `JAVA_INT.varHandle()`). But, in 
> the general case, the accessed value layout and the enclosing layout might 
> differ (e.g. think about accessing a struct field).
> 
> Furthermore, the enclosing layout check depends on the _base_ offset at which 
> the segment is accessed, _prior_ to any index computation that occurs if the 
> accessed layout path has any open elements. It is important to notice that 
> this base offset is only available when looking at the var handle that is 
> returned to the user. For instance, an indexed var handle with coordinates:
> 
> 
> (MemorySegment, long /* base */, long /* index 1 */, long /* index 2 */, long 
> /* index 3 */)
> 
> 
> Is obtained through adaptation, by taking a more basic var handle of the form:
> 
> 
> (MemorySegment, long /* offset */)
> 
> 
> And then injecting the result of the index multiplication into `offset`. As 
> such, we can't add an enclosing layout check inside the var handle returned 
> by `VarHandles::memorySegmentViewHandle`, as doing so will end up seeing the 
> *wrong* offset (e.g. an offset in which the index part has already been mixed 
> in)...

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

  Fix copyrights

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19251/files
  - new: https://git.openjdk.org/jdk/pull/19251/files/236007c3..a7b09d9d

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19251=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=19251=00-01

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

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

Re: RFR: 8331865: Consolidate size and alignment checks in LayoutPath [v2]

[Maurizio Cimadamore]

On Thu, 16 May 2024 14:34:41 GMT, Maurizio Cimadamore  
wrote:

>> When creating a nested memory access var handle, we ensure that the segment 
>> is accessed at the correct alignment for the root layout being accessed. But 
>> we do not ensure that the segment has at least the size of the accessed root 
>> layout. Example:
>> 
>> 
>> MemoryLayout LAYOUT = sequenceLayout(2, structLayout(JAVA_INT.withName("x"), 
>> JAVA_INT.withName("y")));
>> VarHandle X_HANDLE = LAYOUT.varHandle(PathElement.sequenceElement(), 
>> PathElement.groupElement("x"));
>> 
>> 
>> If I have a memory segment whose size is 12, I can successfully call 
>> X_HANDLE on it with index 1, like so:
>> 
>> 
>> MemorySegment segment = Arena.ofAuto().allocate(12);
>> int x = (int)X_HANDLE.get(segment, 0, 1);
>> 
>> 
>> This seems incorrect: after all, the provided segment doesn't have enough 
>> space to fit _two_ elements of the nested structs. 
>> 
>> This PR adds checks to detect this kind of scenario earlier, thus improving 
>> usability. To achieve this we could, in principle, just tweak 
>> `LayoutPath::checkEnclosingLayout` and add the required size check.
>> 
>> But doing so will add yet another redundant check on top of the other 
>> already added by [pull/19124](https://git.openjdk.org/jdk/pull/19124). 
>> Instead, this PR rethinks how memory segment var handles are created, in 
>> order to eliminate redundant checks.
>> 
>> The main observation is that size and alignment checks depend on an 
>> _enclosing_ layout. This layout *might* be the very same value layout being 
>> accessed (this is the case when e.g. using `JAVA_INT.varHandle()`). But, in 
>> the general case, the accessed value layout and the enclosing layout might 
>> differ (e.g. think about accessing a struct field).
>> 
>> Furthermore, the enclosing layout check depends on the _base_ offset at 
>> which the segment is accessed, _prior_ to any index computation that occurs 
>> if the accessed layout path has any open elements. It is important to notice 
>> that this base offset is only available when looking at the var handle that 
>> is returned to the user. For instance, an indexed var handle with 
>> coordinates:
>> 
>> 
>> (MemorySegment, long /* base */, long /* index 1 */, long /* index 2 */, 
>> long /* index 3 */)
>> 
>> 
>> Is obtained through adaptation, by taking a more basic var handle of the 
>> form:
>> 
>> 
>> (MemorySegment, long /* offset */)
>> 
>> 
>> And then injecting the result of the index multiplication into `offset`. As 
>> such, we can't add an enclosing layout check inside the var handle returned 
>> by `VarHandles::memorySegmentViewHandle`, as doing so will end up seeing the 
>> *wrong* off...
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix copyrights

test/jdk/java/foreign/TestHeapAlignment.java line 48:

> 46: assertAligned(align, layout, () -> 
> layout.varHandle().get(segment, 0L));
> 47: assertAligned(align, layout, () -> 
> layout.varHandle().set(segment, 0L, val));
> 48: MemoryLayout seq = MemoryLayout.sequenceLayout(1, layout);

This change was an actual issue in the test, which was made manifest by the new 
checks

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19251#discussion_r1603496914

Re: RFR: 8331865: Consolidate size and alignment checks in LayoutPath

[Maurizio Cimadamore]

On Thu, 16 May 2024 13:55:15 GMT, Maurizio Cimadamore  
wrote:

> > Do we have any performance tests available to see if there are any 
> > potential impacts?
> 
> I've run all micro benchmarks whose name match `LoopOver*`. No regression was 
> found. Few benchmarks seems a tad faster, but the percentages are so tiny 
> that I'm sure it has nothing to do with the patch. I compared results 
> before/after using [JMH visualizer](https://jmh.morethan.io/).

Results here: https://cr.openjdk.org/~mcimadamore/jdk/8331865/
Unfortunately I can't seem to be able to upload the results in the visualizer 
(which would be handy, so I could share a link to the results). Not sure if 
it's an issue in the visualizer, or in the cr server itself. But, it is 
possible to at least download the above results locally, and then upload them 
to the visualizer tool.

-

PR Comment: https://git.openjdk.org/jdk/pull/19251#issuecomment-2115361171

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

[Per Minborg]

On Thu, 16 May 2024 12:48:24 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:
> 
>   Fix copyringht issues

Here are some updated benchmark graphs where we sum two instance variables of 
different sorts (higher is better):

![image](https://github.com/openjdk/jdk/assets/7457876/d82561d6-e803-4345-b6d2-6b0402e60211)

-

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

Re: RFR: 8331865: Consolidate size and alignment checks in LayoutPath

[Maurizio Cimadamore]

On Thu, 16 May 2024 11:18:18 GMT, Per Minborg  wrote:

> Do we have any performance tests available to see if there are any potential 
> impacts?

I've run all micro benchmarks whose name match `LoopOver*`. No regression was 
found. Few benchmarks seems a tad faster, but the percentages are so tiny that 
I'm sure it has nothing to do with the patch. I compared results before/after 
using [JMH visualizer](https://jmh.morethan.io/).

-

PR Comment: https://git.openjdk.org/jdk/pull/19251#issuecomment-2115321357

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

[Alan Bateman]

On Tue, 16 Apr 2024 13:54:53 GMT, Alan Bateman  wrote:

>>> > @mlchung @AlanBateman Any thoughts on this latest version? Is this going 
>>> > into the direction you had envisioned? Any more blockers? The CSR should 
>>> > be up-to-date and is open for review as well. If no more blockers I'll go 
>>> > over this patch once more and clean it up to prepare for integration. 
>>> > Thanks!
>>> 
>>> Thanks for all the efforts on this.
>> 
>> Thanks for looking at this, Alan!
>> 
>>> I've looked through the latest version. I'm a little bit comfortable with 
>>> LinkDeltaProducer. One reason it's build tool that is tied tied to code in 
>>> the jdk.jlink module, and secondly is that it copies 
>>> runtime-image-link.delta into the run-time image. Our long standing 
>>> position is that the run-time image is created by jlink rather than a 
>>> combination of jlink and extra stuff copied in by the build.
>>> 
>>> The part that I like with the current proposal is 
>>> lib/runtime-image-link.delta as it's less complicated that previous 
>>> iterations that added a resource into the jimage container.
>>> 
>>> What would you (and @mlchung) think of having jlink generate 
>>> lib/runtime-image-link.delta as a step between the pipeline and image 
>>> generation?
>> 
>> If I understand you correctly, this would be no longer a build-time only 
>> approach to produce the "linkable runtime"? It would be some-kind of 
>> jlink-option driven approach (as it would run some code that should only run 
>> when producing a linkable runtime is requested)? Other than that, it should 
>> be fine as the previous iteration basically did that but at a different 
>> phase. Also note that the previous iteration used a build-only jlink plugin 
>> so as to satisfy the build-time only approach, yet it ran as part of the 
>> plugin-pipeline which wasn't desired either. But it was something similar to 
>> what you seem to be describing now. Did I get something wrong?
>
>> If I understand you correctly, this would be no longer a build-time only 
>> approach to produce the "linkable runtime"? It would be some-kind of 
>> jlink-option driven approach (as it would run some code that should only run 
>> when producing a linkable runtime is requested)? Other than that, it should 
>> be fine as the previous iteration basically did that but at a different 
>> phase. Also note that the previous iteration used a build-only jlink plugin 
>> so as to satisfy the build-time only approach, yet it ran as part of the 
>> plugin-pipeline which wasn't desired either. But it was something similar to 
>> what you seem to be describing now. Did I get something wrong?
> 
> I think it continues to build time, it's just using some hidden jlink option. 
> So yes, it similar to a previous iteration except that it doesn't run as a 
> plugin the pipeline and the delta goes to the lib directory.
> 
> Let's see what @mlchung says. You've put a lot of work into this so let's see 
> if we can converge to avoid too many more rounds.

> @AlanBateman @mlchung The latest update now uses the `jlink` build time 
> option `--generate-linkable-runtime` to add needed resources to the `jimage` 
> when a runtime linkable JDK image is being asked for using the configure 
> option. This now runs outside the plugin-pipeline. I think this is what you 
> meant. Sorry it took longer to get back to this...

I think you've got this to a good place and I think the overall solution is 
good. It may be that JDK should move to this by default in the future, and at 
the same time re-visit the restriction on generating an image containing 
jdk.jlink, but let's see if any issues come up.

I've added myself as Reviewer to the CSR and I'm working through the code 
changes.

-

PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2115298661

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

[Raffaello Giulietti]

On Thu, 16 May 2024 12:35:22 GMT, Raffaello Giulietti  
wrote:

>> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java 
>> line 204:
>> 
>>> 202: new RandomGeneratorProperties(rgClass, name, group,
>>> 203: i, j, k, equidistribution,
>>> 204: flags | 
>>> (rgClass.isAnnotationPresent(Deprecated.class) ? DEPRECATED : 0)));
>> 
>> Hello Raffaello, this is the final remaining reflection usage and even this 
>> I think isn't required now that all the random generator implementations 
>> reside within java.base as an implementation detail.
>> 
>> I think we should just skip this annotation check here and set `DEPRECATED` 
>> bit on the `flags` to `0` for all implementations. When/if we do deprecate 
>> any of the random generators, we can just come here and switch that bit to 
>> on for the specific random generator when instantiating this 
>> `RandomGeneratorProperties` record. I had a brief look at the code and the 
>> documentation in `package-info.java` of `java/util/random` and we don't 
>> mention that we rely on the `@Deprecated`  annotation to determine whether 
>> an algorithm is deprecated. I think that's a good thing.
>
> Yes, I thought about this the other day but decided for a bit more 
> conservative approach, relying on the annotation.
> 
> But I agree that, since the meta-information now resides in 
> `RandomGeneratorProperties`, we might "migrate" the deprecation status here 
> as well.

Since the legacy generators are public classes, they can be publicly 
deprecated, so in the last commit the `DEPRECATED` bit for them still relies on 
the annotation, which IMO is the authoritative "source of truth".

For the 10 other algorithms, which are accessible only via 
`RandomFactoryGenerator`, we can rely on the info in `RandomProperties`.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1603336696

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

[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:

  Rely on Deprecated annotation only for legacy generators.

-

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

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19212=05
 - incr: https://webrevs.openjdk.org/?repo=jdk=19212=04-05

  Stats: 49 lines in 2 files changed: 18 ins; 1 del; 30 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: 8331670: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal [v2]

[Per Minborg]

On Thu, 16 May 2024 07:14:53 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.
>
> Alan Bateman has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains ten additional 
> commits since the last revision:
> 
>  - Add removal to DISABLED_WARNINGS
>Fail at startup if bad value specified to option
>  - Merge
>  - Update man page
>  - Update man page
>  - Fix comment
>  - Merge
>  - Merge
>  - Whitespace
>  - Initial commit

Looks good. It appears the JVM will be able to constant-fold away the 
`beforeMemoryAcess()` checks but, it would be nice to see the output of the new 
benchmark for; before PR, `MemoryAccessOption.ALLOW` and 
`MemoryAccessOption.WARN`.

-

Marked as reviewed by pminborg (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19174#pullrequestreview-2060709939

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

[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 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.staticArrayListthrpt   10   7614.741 ?  
> 564.777  ops/us
> StableListElementBe...

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

  Fix copyringht issues

-

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

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18794=15
 - incr: https://webrevs.openjdk.org/?repo=jdk=18794=14-15

  Stats: 206 lines in 13 files changed: 200 ins; 1 del; 5 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: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory [v5]

[Raffaello Giulietti]

On Thu, 16 May 2024 10:51:14 GMT, Jaikiran Pai  wrote:

>> Raffaello Giulietti has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Small documentation changes.
>
> test/jdk/java/util/Random/RandomTestCoverage.java line 179:
> 
>> 177: try {
>> 178: rng = factory.create(12345L);
>> 179: } catch (UnsupportedOperationException ignore) {
> 
> I think now that we know for sure which algorithm instances don't support 
> which type of seed value and since throwing `UnsupportedOperationException` 
> is now a specification of the `create(...)` methods, we should probably do 
> something like this:
> 
> 
> diff --git a/test/jdk/java/util/Random/RandomTestCoverage.java 
> b/test/jdk/java/util/Random/RandomTestCoverage.java
> index be12d188198..6e5c36e13c3 100644
> --- a/test/jdk/java/util/Random/RandomTestCoverage.java
> +++ b/test/jdk/java/util/Random/RandomTestCoverage.java
> @@ -171,8 +171,37 @@ static void coverFactory(RandomGeneratorFactory factory) 
> {
>  boolean isSplittable = factory.isSplittable();
>  
>  coverRandomGenerator(factory.create());
> -coverRandomGenerator(factory.create(12345L));
> -coverRandomGenerator(factory.create(new byte[] {1, 2, 3, 4, 5, 6, 7, 
> 8}));
> +
> +String algo = factory.name();
> +// test create(long)
> +switch (algo) {
> +// SecureRandom doesn't have long constructors so we expect
> +// UnsupportedOperationException
> +case "SecureRandom" -> {
> +try {
> +factory.create(12345L);
> +throw new 
> AssertionError("RandomGeneratorFactory.create(long) was expected" +
> +"to throw UnsupportedOperationException for " + 
> algo + " but didn't");
> +} catch (UnsupportedOperationException uoe) {
> +// expected
> +}
> +}
> +default -> coverRandomGenerator(factory.create(12345L));
> +}
> +// test create(byte[])
> +switch (algo) {
> +// these don't have byte[] constructors so we expect 
> UnsupportedOperationException
> +case "Random", "SplittableRandom" -> {
> +try {
> +factory.create(new byte[] {1, 2, 3, 4, 5, 6, 7, 8});
> +throw new 
> AssertionError("RandomGeneratorFactory.create(byte[]) was expected" +
> +"to throw UnsupportedOperationException for " + 
> algo + " but didn't");
> +} catch (UnsupportedOperationException uoe) {
> +// expected
> +}
> +}
> +default -> coverRandomGenerator(factory.create(new byte[] {1, 2, 
> 3, 4, 5, 6, 7, 8}));
> + ...

So you want to be very specific. OK.

> test/jdk/java/util/Random/RandomTestCoverage.java line 195:
> 
>> 193: }
>> 194: 
>> 195: static void coverDefaults() {
> 
> This test method appears to test the calls to `getDefault()` methods on 
> `RandomGeneratorFactory` and `RandomGenerator` classes, but it isn't being 
> called in the test. We should call this method from `main()` to have test 
> coverage for those methods.

Right

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1603274191
PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1603277648

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

[Raffaello Giulietti]

On Thu, 16 May 2024 10:33:55 GMT, Jaikiran Pai  wrote:

>> Raffaello Giulietti has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Small documentation changes.
>
> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 
> 204:
> 
>> 202: new RandomGeneratorProperties(rgClass, name, group,
>> 203: i, j, k, equidistribution,
>> 204: flags | 
>> (rgClass.isAnnotationPresent(Deprecated.class) ? DEPRECATED : 0)));
> 
> Hello Raffaello, this is the final remaining reflection usage and even this I 
> think isn't required now that all the random generator implementations reside 
> within java.base as an implementation detail.
> 
> I think we should just skip this annotation check here and set `DEPRECATED` 
> bit on the `flags` to `0` for all implementations. When/if we do deprecate 
> any of the random generators, we can just come here and switch that bit to on 
> for the specific random generator when instantiating this 
> `RandomGeneratorProperties` record. I had a brief look at the code and the 
> documentation in `package-info.java` of `java/util/random` and we don't 
> mention that we rely on the `@Deprecated`  annotation to determine whether an 
> algorithm is deprecated. I think that's a good thing.

Yes, I thought about this the other day but decided for a bit more conservative 
approach, relying on the annotation.

But I agree that, since the meta-information now resides in 
`RandomGeneratorProperties`, we might "migrate" the deprecation status here as 
well.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1603264183

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

[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 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.staticArrayListthrpt   10   7614.741 ?  
> 564.777  ops/us
> StableListElementBe...

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

  Simplify background thread handling

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18794/files
  - new: https://git.openjdk.org/jdk/pull/18794/files/3e1ab5e9..2af0168e

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18794=14
 - incr: https://webrevs.openjdk.org/?repo=jdk=18794=13-14

  Stats: 7 lines in 1 file changed: 0 ins; 4 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) [v14]

[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 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.staticArrayListthrpt   10   7614.741 ?  
> 564.777  ops/us
> StableListElementBe...

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

  Improve toString and simplify offset calculations

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18794/files
  - new: https://git.openjdk.org/jdk/pull/18794/files/dbbefea5..3e1ab5e9

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18794=13
 - incr: https://webrevs.openjdk.org/?repo=jdk=18794=12-13

  Stats: 51 lines in 2 files changed: 18 ins; 27 del; 6 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: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v7]

[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:

  Add note on --illegal-native-access default value in the launcher help

-

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

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19213=06
 - incr: https://webrevs.openjdk.org/?repo=jdk=19213=05-06

  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: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v6]

[Maurizio Cimadamore]

On Thu, 16 May 2024 11:55:35 GMT, Jaikiran Pai  wrote:

>> We already do, see
>> https://github.com/openjdk/jdk/pull/19213/files/1c45e5d56c429205ab8185481bc1044a86ab3bc6#diff-d05029afe6aed86f860a10901114402f1f6af4fe1e4b46d883141ab1d2a527b8R582
>
> This is slightly different from what we do in the other PR for unsafe memory 
> access where we specify the default in the launcher's help text too 
> https://github.com/openjdk/jdk/pull/19174/files#diff-799093930b698e97b23ead98c6496261af1e2e33ec7aa9261584870cbee8a5eaR219.
> 
> I don't have a strong opinion on this, I think either one is fine.

Ah, apologies, I was looking in the wrong place. I agree that we should specify 
default in the launcher, as well as in the man pages.

-

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

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

[Aman Sharma]

Hi,


Thanks for your response, Liang!


> I think you meant CVE-2021-42392 instead of 2022.


Sorry of the error. I indeed meant 
CVE-2021-42392.


> Leyden mainly avoids this unstable generation by performing a training run to 
> collect classes loaded


Would love to know the details of Project Leyden and how they worked so far to 
focus on this goal. In our case, the training run is the test suite.


> GeneratedConstructorAccessor is already retired by JEP 416 [2] in Java 18


I did see them not appearing in my allowlist when I ran my study subject 
(Apache PDFBox) with Java 21. Thanks for letting me know about this JEP. I see 
they are re-implemented with method handles.


> How are you checking the classes?


To detect runtime generated code, we have javaagent that is hooked statically 
to the test suite execution. It gives us all classes that that is loaded post 
the JVM and the javaagent are loaded. So we only check the classes loaded for 
the purpose of running the application. This is also why we did not choose 
-agentlib as it would give classes for the setting up JVM and javaagent and we 
the user of our tool must the classes they load.


Next, we have a `ClassFileTransformer` hook in the agent where we produce the 
checksum using the bytecode. And we compare the checksum with the one existing 
in the allowlist. The checksum computation algorithm is same for both steps. 
Let me describe how I compute the checksum.


  1.  I get the 
CONSTANT_Class_info
 entry corresponding to `this_class` and rewrite the 
CONSTANT_Utf8_info
 corresponding to a fix String constant, say "foo".
  2.  Since, the name of the class is used to refer to its types members 
(fields/method), I get all 
CONSTANT_Fieldref_info
 and if its `class_index` corresponds to the old `this_class`, we rewrite the 
UTF8 value of class_index to the same constant "foo".
  3.  Next, since the naming of the fields, in Proxy classes, are also suffixed 
by numbers, for example, `private static Method m4`, we rewrite the UTF8 value 
of name in the 
CONSTANT_NameAndType_info.
  4.  These fields can also have a random order so we simply sort the entire 
byte code using `Arrays.sort(byte[])` to eliminate any differences due to 
ordering of fields/methods.
  5.  Simply sorting the byte array still had minute differences. I could not 
understand why they existed even though values in constant pool of the bytecode 
in allowlist and at runtime were exactly the same after rewriting. The 
differences existed in the bytes of the Code attribute of methods. I concluded 
that the bytes stored some position information. To avoid this, I created a 
subarray where I considered the bytes corresponding to 
`CONSTANT_Utf8_info.bytes` only. Computing a checksum for it resulted in the 
same checksums for both classfiles.

Let's understand the whole approach with an example of Proxy class.

`

public final class $Proxy42 extends Proxy implements 
org.apache.logging.log4j.core.config.plugins.Plugin {

`

The will go in the allowlist as "Proxy_Plugin: ".

When the same class is intercepted at runtime, say "$Proxy10", we look for 
"Proxy_Plugin" in the allowlist and since the checksum algorithm is same in 
both cases, we get a match and let the class load.

This approach has seemed to work well for Proxy classes, Generated Constructor 
Accessor (which is removed as you said). I also looked at the species generated 
by method handles. I did not notice any modification in them. Their name 
generation seemed okay to me. If some new Species are generated, it is of 
course detected since it is not in the allowlist.

I have not looked into LambdaMetafactory because I did not encounter it as a 
problem so far, but I am aware its name generation is also unstable. I have run 
my approach only a few projects only. And for hidden classes, I assume the the 
agent won't be able to intercept them so detecting them would be really hard.


Regards,
Aman Sharma

PhD Student
KTH Royal Institute of Technology
School of Electrical Engineering and Computer Science (EECS)
Department of Theoretical Computer Science (TCS)

https://algomaster99.github.io/

From: liangchenb...@gmail.com 
Sent: Thursday, May 16, 2024 5:52:03 AM
To: Aman Sharma; core-libs-dev
Cc: Martin Monperrus
Subject: 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 

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

[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 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.staticArrayListthrpt   10   7614.741 ?  
> 564.777  ops/us
> StableListElementBe...

Per Minborg has updated the pull request incrementally with two additional 
commits since the last revision:

 - Cleanup switch rakes
 - Update null benchmarks

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18794/files
  - new: https://git.openjdk.org/jdk/pull/18794/files/b40ebfad..dbbefea5

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18794=12
 - incr: https://webrevs.openjdk.org/?repo=jdk=18794=11-12

  Stats: 19 lines in 4 files changed: 3 ins; 6 del; 10 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: 8331876: JFR: Move file read and write events to java.base [v5]

[Erik Gahlin]

> Hi,
> 
> Could I have a review of a change that moves the jdk.FileRead and 
> jdk.FileWrite events to java.base to remove the use of the ASM 
> instrumentation.
> 
> Testing: jdk/jdk/jfr
> 
> Thanks
> Erik

Erik Gahlin has updated the pull request incrementally with one additional 
commit since the last revision:

  Remove dependency on JFR for retransformation

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19129/files
  - new: https://git.openjdk.org/jdk/pull/19129/files/f2439ac3..c4c64774

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19129=04
 - incr: https://webrevs.openjdk.org/?repo=jdk=19129=03-04

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

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

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

[Per Minborg]

On Thu, 16 May 2024 11:14:16 GMT, Chen Liang  wrote:

>> The idea here is to have the most likely value in the middle... Not sure if 
>> that motivates the added complexity though.
>
> Is there any refernce on how/why the middle entry in a tableswitch 
> instruction is the fastest?

It is only in a _lookupswitch_ that this becomes relevant. The above code will 
generate a *tableswitch* so I think it is safe to simplify the code and remove 
the DUMMY.


  private V orThrowVolatile();
descriptor: ()Ljava/lang/Object;
flags: (0x0002) ACC_PRIVATE
Code:
  stack=1, locals=1, args_size=1
 0: aload_0
 1: invokevirtual #15 // Method stateVolatile:()I
 4: tableswitch   { // 0 to 4
   0: 40
   1: 44
   2: 46
   3: 51
   4: 56
 default: 60
}
...

-

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

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

[Jaikiran Pai]

On Thu, 16 May 2024 11:47:13 GMT, Maurizio Cimadamore  
wrote:

>> src/java.base/share/classes/sun/launcher/resources/launcher.properties line 
>> 72:
>> 
>>> 70: \  by code in modules for which native access is not 
>>> explicitly enabled.\n\
>>> 71: \   is one of "deny", "warn" or "allow".\n\
>>> 72: \  This option will be removed in a future release.\n\
>> 
>> Should this specify the current default value for this option if it isn't 
>> set?
>
> We already do, see
> https://github.com/openjdk/jdk/pull/19213/files/1c45e5d56c429205ab8185481bc1044a86ab3bc6#diff-d05029afe6aed86f860a10901114402f1f6af4fe1e4b46d883141ab1d2a527b8R582

This is slightly different from what we do in the other PR for unsafe memory 
access where we specify the default in the launcher's help text too 
https://github.com/openjdk/jdk/pull/19174/files#diff-799093930b698e97b23ead98c6496261af1e2e33ec7aa9261584870cbee8a5eaR219.

I don't have a strong opinion on this, I think either one is fine.

-

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

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

[Per Minborg]

On Thu, 16 May 2024 11:13:24 GMT, Chen Liang  wrote:

>> It seems reasonable to assume `null` values are not constant-folded. For 
>> straight-out-of-the-box usage, there is no apparent significant difference 
>> as indicated by a new benchmark I just added:
>> 
>> 
>> Benchmark  Mode  Cnt  Score  Error   
>> Units
>> StableStaticBenchmark.atomic  thrpt   10   5729.683 ?  502.023  
>> ops/us
>> StableStaticBenchmark.dcl thrpt   10   6069.222 ?  951.784  
>> ops/us
>> StableStaticBenchmark.dclHolder   thrpt   10   5502.102 ? 1630.627  
>> ops/us
>> StableStaticBenchmark.stable  thrpt   10  12737.158 ? 1746.456  
>> ops/us <- Non-null benchmark
>> StableStaticBenchmark.stableHolderthrpt   10  12053.978 ? 1421.527  
>> ops/us
>> StableStaticBenchmark.stableList  thrpt   10  12443.870 ? 2084.607  
>> ops/us
>> StableStaticBenchmark.stableNull  thrpt   10  13164.232 ?  591.284  
>> ops/us <- Added null benchmark
>> StableStaticBenchmark.stableRecordHolder  thrpt   10  13638.893 ? 1250.895  
>> ops/us
>> StableStaticBenchmark.staticCHI   thrpt   10  13639.220 ? 1190.922  
>> ops/us
>> 
>> 
>> If the `null` value participates in a much larger constant-folding tree, 
>> there might be a significant difference. I am afraid moving the order would 
>> have detrimental effects on instance performance:
>> 
>> Checking value first:
>> 
>> 
>> Benchmark   Mode  Cnt Score  Error   Units
>> StableBenchmark.atomic thrpt   10   246.460 ?   75.417  ops/us
>> StableBenchmark.dclthrpt   10   243.481 ?   35.021  ops/us
>> StableBenchmark.stable thrpt   10  4977.693 ?  675.926  ops/us  
>> <- Non-null
>> StableBenchmark.stableHoldingList  thrpt   10  3614.460 ?  275.140  ops/us
>> StableBenchmark.stableList thrpt   10  3328.155 ?  898.202  ops/us
>> StableBenchmark.stableListStored   thrpt   10  3842.174 ?  535.902  ops/us
>> StableBenchmark.stableNull thrpt   10  6217.737 ?  840.376  ops/us 
>> <- null
>> StableBenchmark.supplier   thrpt   10  9369.934 ? 1449.182  ops/us
>> 
>> 
>> Checking null first:
>> 
>> 
>> Benchmark   Mode  Cnt Score  Error   Units
>> StableBenchmark.atomic thrpt   10   287.640 ?   17.858  ops/us
>> StableBenchmark.dclthrpt   10   250.398 ?   20.874  ops/us
>> StableBenchmark.stable thrpt   10  3745.885 ? 1040.534  ops/us 
>> <- Non-null
>> StableBenchmark.stableHoldingList  thrpt   10  2982.129 ?  503.492  ops/us
>> StableBenchmar...
>
> I think the result would be more convincing if the stable case is changed to 
> `sum += (stable.orThrow() == null ? 0 : 1) + (stable2.orThrow() == null ? 0 : 
> 1);` as adding by 1 might be somewhat better optimized by JIT.

I have instead changed parts of the `stableNull()` body to:


sum += (stableNull.orThrow() == null ? VALUE : VALUE2) + (stableNull2.orThrow() 
== null ? VALUE : VALUE2);

-

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

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

[Chen Liang]

On Thu, 16 May 2024 07:29:21 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:
> 
>   Simplify exception handling and add benchmarks

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

> 398: @Override
> 399: public void run() {
> 400: stable.computeIfUnset(supplier);

We can just declare this runnable as a capturing lambda (or an anonymous class 
if you fear initialization issues) and leave this comment there. The thread 
field can be removed.

-

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

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

[Maurizio Cimadamore]

On Thu, 16 May 2024 11:42:48 GMT, Jaikiran Pai  wrote:

> Hello Maurizio, in the current mainline, we have code in `LauncherHelper` 
> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/launcher/LauncherHelper.java#L636
>  where we enable native access to all unnamed modules if an executable jar 
> with `Enable-Native-Access: ALL-UNNAMED` manifest is being launched. For such 
> executable jars, what is the expected semantics when the launch also 
> explicitly has a `--enable-native-access=M1,M2` option. Something like:
> 
> ```
> java --enable-native-access=M1,M2 -jar foo.jar
> ```
> 
> where `foo.jar` has `Enable-Native-Access: ALL-UNNAMED` in its manifest.

The options are additive - e.g. the enable-native-access in the manifest will 
add up to the enable-native-access in the command line, so effectively it will 
be as if you were running with --enable-native-access=M1,M2,ALL-UNNAMED

> src/java.base/share/classes/sun/launcher/resources/launcher.properties line 
> 72:
> 
>> 70: \  by code in modules for which native access is not 
>> explicitly enabled.\n\
>> 71: \   is one of "deny", "warn" or "allow".\n\
>> 72: \  This option will be removed in a future release.\n\
> 
> Should this specify the current default value for this option if it isn't set?

We already do, see
https://github.com/openjdk/jdk/pull/19213/files/1c45e5d56c429205ab8185481bc1044a86ab3bc6#diff-d05029afe6aed86f860a10901114402f1f6af4fe1e4b46d883141ab1d2a527b8R582

-

PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2115012361
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603195671

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

[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 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.staticArrayListthrpt   10   7614.741 ?  
> 564.777  ops/us
> StableListElementBe...

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

  Clean up

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18794/files
  - new: https://git.openjdk.org/jdk/pull/18794/files/923e1877..b40ebfad

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18794=11
 - incr: https://webrevs.openjdk.org/?repo=jdk=18794=10-11

  Stats: 64 lines in 3 files changed: 14 ins; 21 del; 29 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: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v6]

[Jaikiran Pai]

On Wed, 15 May 2024 16:08:17 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:
> 
>   Address review comment

Hello Maurizio, in the current mainline, we have code in `LauncherHelper` 
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/launcher/LauncherHelper.java#L636
 where we enable native access to all unnamed modules if an executable jar with 
`Enable-Native-Access: ALL-UNNAMED` manifest is being launched. For such 
executable jars, what is the expected semantics when the launch also explicitly 
has a `--enable-native-access=M1,M2` option. Something like:


java --enable-native-access=M1,M2 -jar foo.jar

where `foo.jar` has `Enable-Native-Access: ALL-UNNAMED` in its manifest.

-

PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2115005638

Re: RFR: 8331865: Consolidate size and alignment checks in LayoutPath

[Per Minborg]

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

> When creating a nested memory access var handle, we ensure that the segment 
> is accessed at the correct alignment for the root layout being accessed. But 
> we do not ensure that the segment has at least the size of the accessed root 
> layout. Example:
> 
> 
> MemoryLayout LAYOUT = sequenceLayout(2, structLayout(JAVA_INT.withName("x"), 
> JAVA_INT.withName("y")));
> VarHandle X_HANDLE = LAYOUT.varHandle(PathElement.sequenceElement(), 
> PathElement.groupElement("x"));
> 
> 
> If I have a memory segment whose size is 12, I can successfully call X_HANDLE 
> on it with index 1, like so:
> 
> 
> MemorySegment segment = Arena.ofAuto().allocate(12);
> int x = (int)X_HANDLE.get(segment, 0, 1);
> 
> 
> This seems incorrect: after all, the provided segment doesn't have enough 
> space to fit _two_ elements of the nested structs. 
> 
> This PR adds checks to detect this kind of scenario earlier, thus improving 
> usability. To achieve this we could, in principle, just tweak 
> `LayoutPath::checkEnclosingLayout` and add the required size check.
> 
> But doing so will add yet another redundant check on top of the other already 
> added by [pull/19124](https://git.openjdk.org/jdk/pull/19124). Instead, this 
> PR rethinks how memory segment var handles are created, in order to eliminate 
> redundant checks.
> 
> The main observation is that size and alignment checks depend on an 
> _enclosing_ layout. This layout *might* be the very same value layout being 
> accessed (this is the case when e.g. using `JAVA_INT.varHandle()`). But, in 
> the general case, the accessed value layout and the enclosing layout might 
> differ (e.g. think about accessing a struct field).
> 
> Furthermore, the enclosing layout check depends on the _base_ offset at which 
> the segment is accessed, _prior_ to any index computation that occurs if the 
> accessed layout path has any open elements. It is important to notice that 
> this base offset is only available when looking at the var handle that is 
> returned to the user. For instance, an indexed var handle with coordinates:
> 
> 
> (MemorySegment, long /* base */, long /* index 1 */, long /* index 2 */, long 
> /* index 3 */)
> 
> 
> Is obtained through adaptation, by taking a more basic var handle of the form:
> 
> 
> (MemorySegment, long /* offset */)
> 
> 
> And then injecting the result of the index multiplication into `offset`. As 
> such, we can't add an enclosing layout check inside the var handle returned 
> by `VarHandles::memorySegmentViewHandle`, as doing so will end up seeing the 
> *wrong* offset (e.g. an offset in which the index part has already been mixed 
> in)...

Do we have any performance tests available to see if there are any potential 
impacts?

src/java.base/share/classes/java/lang/invoke/VarHandleSegmentViewBase.java line 
48:

> 46: final long alignmentMask;
> 47: 
> 48: VarHandleSegmentViewBase(VarForm form, boolean be, long 
> alignmentMask, boolean exact) {

Nit: Copyright year. This also applies to some other files in this PR.

-

PR Comment: https://git.openjdk.org/jdk/pull/19251#issuecomment-2114955648
PR Review Comment: https://git.openjdk.org/jdk/pull/19251#discussion_r1603156019

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

[Jaikiran Pai]

On Wed, 15 May 2024 16:08:17 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:
> 
>   Address review comment

src/java.base/share/classes/sun/launcher/resources/launcher.properties line 72:

> 70: \  by code in modules for which native access is not 
> explicitly enabled.\n\
> 71: \   is one of "deny", "warn" or "allow".\n\
> 72: \  This option will be removed in a future release.\n\

Should this specify the current default value for this option if it isn't set?

-

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

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

[Chen Liang]

On Thu, 16 May 2024 07:11:20 GMT, Per Minborg  wrote:

>> 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?
>
> The idea here is to have the most likely value in the middle... Not sure if 
> that motivates the added complexity though.

Is there any refernce on how/why the middle entry in a tableswitch instruction 
is the fastest?

>> 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?
>
> We considered recording the entire exception instance but for security 
> reasons, we ended up just recording the type of exception. I will add a 
> comment explaining this in the code.

Thanks for this clarification. Makes sense.

-

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

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

[Chen Liang]

On Thu, 16 May 2024 06:54:26 GMT, Per Minborg  wrote:

>> 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
>
> It seems reasonable to assume `null` values are not constant-folded. For 
> straight-out-of-the-box usage, there is no apparent significant difference as 
> indicated by a new benchmark I just added:
> 
> 
> Benchmark  Mode  Cnt  Score  Error   
> Units
> StableStaticBenchmark.atomic  thrpt   10   5729.683 ?  502.023  
> ops/us
> StableStaticBenchmark.dcl thrpt   10   6069.222 ?  951.784  
> ops/us
> StableStaticBenchmark.dclHolder   thrpt   10   5502.102 ? 1630.627  
> ops/us
> StableStaticBenchmark.stable  thrpt   10  12737.158 ? 1746.456  
> ops/us <- Non-null benchmark
> StableStaticBenchmark.stableHolderthrpt   10  12053.978 ? 1421.527  
> ops/us
> StableStaticBenchmark.stableList  thrpt   10  12443.870 ? 2084.607  
> ops/us
> StableStaticBenchmark.stableNull  thrpt   10  13164.232 ?  591.284  
> ops/us <- Added null benchmark
> StableStaticBenchmark.stableRecordHolder  thrpt   10  13638.893 ? 1250.895  
> ops/us
> StableStaticBenchmark.staticCHI   thrpt   10  13639.220 ? 1190.922  
> ops/us
> 
> 
> If the `null` value participates in a much larger constant-folding tree, 
> there might be a significant difference. I am afraid moving the order would 
> have detrimental effects on instance performance:
> 
> Checking value first:
> 
> 
> Benchmark   Mode  Cnt Score  Error   Units
> StableBenchmark.atomic thrpt   10   246.460 ?   75.417  ops/us
> StableBenchmark.dclthrpt   10   243.481 ?   35.021  ops/us
> StableBenchmark.stable thrpt   10  4977.693 ?  675.926  ops/us  
> <- Non-null
> StableBenchmark.stableHoldingList  thrpt   10  3614.460 ?  275.140  ops/us
> StableBenchmark.stableList thrpt   10  3328.155 ?  898.202  ops/us
> StableBenchmark.stableListStored   thrpt   10  3842.174 ?  535.902  ops/us
> StableBenchmark.stableNull thrpt   10  6217.737 ?  840.376  ops/us <- 
> null
> StableBenchmark.supplier   thrpt   10  9369.934 ? 1449.182  ops/us
> 
> 
> Checking null first:
> 
> 
> Benchmark   Mode  Cnt Score  Error   Units
> StableBenchmark.atomic thrpt   10   287.640 ?   17.858  ops/us
> StableBenchmark.dclthrpt   10   250.398 ?   20.874  ops/us
> StableBenchmark.stable thrpt   10  3745.885 ? 1040.534  ops/us <- 
> Non-null
> StableBenchmark.stableHoldingList  thrpt   10  2982.129 ?  503.492  ops/us
> StableBenchmark.stableList thrpt   10  3125.045 ?  416.792  ops/us
> StableBenchmark.sta...

I think the result would be more convincing if the stable case is changed to 
`sum += (stable.orThrow() == null ? 0 : 1) + (stable2.orThrow() == null ? 0 : 
1);` as adding by 1 might be somewhat better optimized by JIT.

-

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

Re: RFR: 8331865: Consolidate size and alignment checks in LayoutPath

[Maurizio Cimadamore]

On Thu, 16 May 2024 10:54:15 GMT, Maurizio Cimadamore  
wrote:

>> When creating a nested memory access var handle, we ensure that the segment 
>> is accessed at the correct alignment for the root layout being accessed. But 
>> we do not ensure that the segment has at least the size of the accessed root 
>> layout. Example:
>> 
>> 
>> MemoryLayout LAYOUT = sequenceLayout(2, structLayout(JAVA_INT.withName("x"), 
>> JAVA_INT.withName("y")));
>> VarHandle X_HANDLE = LAYOUT.varHandle(PathElement.sequenceElement(), 
>> PathElement.groupElement("x"));
>> 
>> 
>> If I have a memory segment whose size is 12, I can successfully call 
>> X_HANDLE on it with index 1, like so:
>> 
>> 
>> MemorySegment segment = Arena.ofAuto().allocate(12);
>> int x = (int)X_HANDLE.get(segment, 0, 1);
>> 
>> 
>> This seems incorrect: after all, the provided segment doesn't have enough 
>> space to fit _two_ elements of the nested structs. 
>> 
>> This PR adds checks to detect this kind of scenario earlier, thus improving 
>> usability. To achieve this we could, in principle, just tweak 
>> `LayoutPath::checkEnclosingLayout` and add the required size check.
>> 
>> But doing so will add yet another redundant check on top of the other 
>> already added by [pull/19124](https://git.openjdk.org/jdk/pull/19124). 
>> Instead, this PR rethinks how memory segment var handles are created, in 
>> order to eliminate redundant checks.
>> 
>> The main observation is that size and alignment checks depend on an 
>> _enclosing_ layout. This layout *might* be the very same value layout being 
>> accessed (this is the case when e.g. using `JAVA_INT.varHandle()`). But, in 
>> the general case, the accessed value layout and the enclosing layout might 
>> differ (e.g. think about accessing a struct field).
>> 
>> Furthermore, the enclosing layout check depends on the _base_ offset at 
>> which the segment is accessed, _prior_ to any index computation that occurs 
>> if the accessed layout path has any open elements. It is important to notice 
>> that this base offset is only available when looking at the var handle that 
>> is returned to the user. For instance, an indexed var handle with 
>> coordinates:
>> 
>> 
>> (MemorySegment, long /* base */, long /* index 1 */, long /* index 2 */, 
>> long /* index 3 */)
>> 
>> 
>> Is obtained through adaptation, by taking a more basic var handle of the 
>> form:
>> 
>> 
>> (MemorySegment, long /* offset */)
>> 
>> 
>> And then injecting the result of the index multiplication into `offset`. As 
>> such, we can't add an enclosing layout check inside the var handle returned 
>> by `VarHandles::memorySegmentViewHandle`, as doing so will end up seeing the 
>> *wrong* off...
>
> src/java.base/share/classes/java/lang/invoke/X-VarHandleSegmentView.java.template
>  line 100:
> 
>> 98: 
>> 99: @ForceInline
>> 100: static AbstractMemorySegmentImpl checkReadOnly(Object obb, boolean 
>> ro) {
> 
> This and the following methods are the bulk of the changes in this template. 
> That is, we no longer check size and alignment of the accessed segment. Every 
> other change in this template is needed to get rid of fields and parameters 
> that are no longer used.

Note: we don't even check for overflows, e.g. `offset < 0`. The reasoning here 
is that all layouts check for overflow on construction, so it's never possible 
to construct a layout whose size is greater than `Long.MAX_VALUE`. This 
constraint also affects computation for indexed var handles, since all the 
`long` indices corresponding to open path elements are checked against their 
bounds (and their bounds must be small enough so that the enclosing layout has 
a size smaller than `Long.MAX_VALUE`).

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19251#discussion_r1603142096

Re: RFR: 8331865: Consolidate size and alignment checks in LayoutPath

[Maurizio Cimadamore]

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

> When creating a nested memory access var handle, we ensure that the segment 
> is accessed at the correct alignment for the root layout being accessed. But 
> we do not ensure that the segment has at least the size of the accessed root 
> layout. Example:
> 
> 
> MemoryLayout LAYOUT = sequenceLayout(2, structLayout(JAVA_INT.withName("x"), 
> JAVA_INT.withName("y")));
> VarHandle X_HANDLE = LAYOUT.varHandle(PathElement.sequenceElement(), 
> PathElement.groupElement("x"));
> 
> 
> If I have a memory segment whose size is 12, I can successfully call X_HANDLE 
> on it with index 1, like so:
> 
> 
> MemorySegment segment = Arena.ofAuto().allocate(12);
> int x = (int)X_HANDLE.get(segment, 0, 1);
> 
> 
> This seems incorrect: after all, the provided segment doesn't have enough 
> space to fit _two_ elements of the nested structs. 
> 
> This PR adds checks to detect this kind of scenario earlier, thus improving 
> usability. To achieve this we could, in principle, just tweak 
> `LayoutPath::checkEnclosingLayout` and add the required size check.
> 
> But doing so will add yet another redundant check on top of the other already 
> added by [pull/19124](https://git.openjdk.org/jdk/pull/19124). Instead, this 
> PR rethinks how memory segment var handles are created, in order to eliminate 
> redundant checks.
> 
> The main observation is that size and alignment checks depend on an 
> _enclosing_ layout. This layout *might* be the very same value layout being 
> accessed (this is the case when e.g. using `JAVA_INT.varHandle()`). But, in 
> the general case, the accessed value layout and the enclosing layout might 
> differ (e.g. think about accessing a struct field).
> 
> Furthermore, the enclosing layout check depends on the _base_ offset at which 
> the segment is accessed, _prior_ to any index computation that occurs if the 
> accessed layout path has any open elements. It is important to notice that 
> this base offset is only available when looking at the var handle that is 
> returned to the user. For instance, an indexed var handle with coordinates:
> 
> 
> (MemorySegment, long /* base */, long /* index 1 */, long /* index 2 */, long 
> /* index 3 */)
> 
> 
> Is obtained through adaptation, by taking a more basic var handle of the form:
> 
> 
> (MemorySegment, long /* offset */)
> 
> 
> And then injecting the result of the index multiplication into `offset`. As 
> such, we can't add an enclosing layout check inside the var handle returned 
> by `VarHandles::memorySegmentViewHandle`, as doing so will end up seeing the 
> *wrong* offset (e.g. an offset in which the index part has already been mixed 
> in)...

src/java.base/share/classes/java/lang/invoke/X-VarHandleSegmentView.java.template
 line 100:

> 98: 
> 99: @ForceInline
> 100: static AbstractMemorySegmentImpl checkReadOnly(Object obb, boolean 
> ro) {

This and the following methods are the bulk of the changes in this template. 
That is, we no longer check size and alignment of the accessed segment. Every 
other change in this template is needed to get rid of fields and parameters 
that are no longer used.

src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java 
line 831:

> 829: @Override
> 830: public void set(AddressLayout layout, long offset, MemorySegment 
> value) {
> 831: Objects.requireNonNull(value);

This has been added, otherwise it would be possible to pass a `null` as the 
value of `value`, and not get an NPE, in case e.g. the alignment of the segment 
is incorrect (because we now check that before we even try to perform the 
memory access).

test/jdk/java/foreign/TestAccessModes.java line 62:

> 60: } catch (IllegalArgumentException ex) {
> 61: // access is unaligned
> 62: assertTrue(segment.maxByteAlignment() < 
> layout.byteAlignment());

Note: this change is required because, before this PR, we used to issue UOE for 
a bad access mode regardless of the alignment with which we accessed the 
segment (well, only for toplevel var handles). Now we uniformly check alignment 
_before_ access mode, for both toplevel and nested var handles, so this 
assertion needed to be tweaked.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19251#discussion_r1603127240
PR Review Comment: https://git.openjdk.org/jdk/pull/19251#discussion_r1603120915
PR Review Comment: https://git.openjdk.org/jdk/pull/19251#discussion_r1603124981

RFR: 8331865: Consolidate size and alignment checks in LayoutPath

[Maurizio Cimadamore]

When creating a nested memory access var handle, we ensure that the segment is 
accessed at the correct alignment for the root layout being accessed. But we do 
not ensure that the segment has at least the size of the accessed root layout. 
Example:


MemoryLayout LAYOUT = sequenceLayout(2, structLayout(JAVA_INT.withName("x"), 
JAVA_INT.withName("y")));
VarHandle X_HANDLE = LAYOUT.varHandle(PathElement.sequenceElement(), 
PathElement.groupElement("x"));


If I have a memory segment whose size is 12, I can successfully call X_HANDLE 
on it with index 1, like so:


MemorySegment segment = Arena.ofAuto().allocate(12);
int x = (int)X_HANDLE.get(segment, 0, 1);


This seems incorrect: after all, the provided segment doesn't have enough space 
to fit _two_ elements of the nested structs. 

This PR adds checks to detect this kind of scenario earlier, thus improving 
usability. To achieve this we could, in principle, just tweak 
`LayoutPath::checkEnclosingLayout` and add the required size check.

But doing so will add yet another redundant check on top of the other already 
added by [pull/19124](https://git.openjdk.org/jdk/pull/19124). Instead, this PR 
rethinks how memory segment var handles are created, in order to eliminate 
redundant checks.

The main observation is that size and alignment checks depend on an _enclosing_ 
layout. This layout *might* be the very same value layout being accessed (this 
is the case when e.g. using `JAVA_INT.varHandle()`). But, in the general case, 
the accessed value layout and the enclosing layout might differ (e.g. think 
about accessing a struct field).

Furthermore, the enclosing layout check depends on the _base_ offset at which 
the segment is accessed, _prior_ to any index computation that occurs if the 
accessed layout path has any open elements. It is important to notice that this 
base offset is only available when looking at the var handle that is returned 
to the user. For instance, an indexed var handle with coordinates:


(MemorySegment, long /* base */, long /* index 1 */, long /* index 2 */, long 
/* index 3 */)


Is obtained through adaptation, by taking a more basic var handle of the form:


(MemorySegment, long /* offset */)


And then injecting the result of the index multiplication into `offset`. As 
such, we can't add an enclosing layout check inside the var handle returned by 
`VarHandles::memorySegmentViewHandle`, as doing so will end up seeing the 
*wrong* offset (e.g. an offset in which the index part has already been mixed 
in).

The only possibility then, is to remove size and alignment checks from the 
*raw* var handles returned by `VarHandles::memorySegmentViewHandle`, and 
perform such checks outside (e.g. in `LayoutPath::dereferenceHandle`). The only 
checks left in the raw var handles are:

* an alignment check to make sure that the access mode selected by the client 
is really available - this alignment check is against the alignment of the 
value layout being selected, and not the enclosing layout alignment (which 
might be stricter)
* a read-only check, to make sure that write access modes are blocked if the 
accessed segment is read-only
* liveness/confinement checks, as mandated by ScopedMemoryAccess

Since these check depends on the particular access mode selected by the client, 
we can't move these checks away from the raw var handle.

These changes come with some consequences:

* Now it is always necessary to adapt a raw var handle, and to insert 
appropriate size and alignment checks (otherwise OOB access might be possible). 
As such, `ValueLayouts` also need to call the path layout variant of 
`MemoryLayout::varHandle`, to make sure that the raw var handle is adapted 
accordingly, before it is cached in its stable field.
* The var handle cache has been moved from `Utils` to 
`ValueLayouts::varHandle`. The cache is used (a) to reduce the number of var 
handle instances created and (b) to make sure that the cached var handle in the 
`ValueLayout` has stable identity. With respect to (a), while it makes sense to 
cache "toplevel" var handles (e.g. `JAVA_INT.varHandle()`) the cost/benefit 
ratio for caching nested var handles seem more unfavourable, as the same var 
handle can be reused with different enclosing layouts, leading to different 
checks. Ultimately, all nested var handles will require some kind of 
adaptation, so it doesn't seem too crucial to have a deeper level of caching 
here.
* The order in which exceptions are thrown might be slightly different. This is 
because the size/alignment checks now take place _before_ the raw var handle is 
even called. This caused a bunch of small updates in code and tests.
* It used to be possible to create a sequence layout with maximal size, like 
`MemoryLayout.sequenceLayout(Long.MAX_VALUE, JAVA_LONG)`, and derive an indexed 
var handle from that layout. Since there used to be no enclosing layout size 
check, access to the sequence element was allowed, as long as the index 
computation did 

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

[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 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.staticArrayListthrpt   10   7614.741 ?  
> 564.777  ops/us
> StableListElementBe...

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

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

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18794/files
  - new: https://git.openjdk.org/jdk/pull/18794/files/80b7e081..923e1877

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18794=10
 - incr: https://webrevs.openjdk.org/?repo=jdk=18794=09-10

  Stats: 6 lines in 1 file changed: 0 ins; 0 del; 6 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: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory [v5]

[Jaikiran Pai]

On Wed, 15 May 2024 09:51:23 GMT, Raffaello Giulietti  
wrote:

>> 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.

test/jdk/java/util/Random/RandomTestCoverage.java line 195:

> 193: }
> 194: 
> 195: static void coverDefaults() {

This test method appears to test the calls to `getDefault()` methods on 
`RandomGeneratorFactory` and `RandomGenerator` classes, but it isn't being 
called in the test. We should call this method from `main()` to have test 
coverage for those methods.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1603131062

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

[Jaikiran Pai]

On Wed, 15 May 2024 09:51:23 GMT, Raffaello Giulietti  
wrote:

>> 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.

test/jdk/java/util/Random/RandomTestCoverage.java line 179:

> 177: try {
> 178: rng = factory.create(12345L);
> 179: } catch (UnsupportedOperationException ignore) {

I think now that we know for sure which algorithm instances don't support which 
type of seed value and since throwing `UnsupportedOperationException` is now a 
specification of the `create(...)` methods, we should probably do something 
like this:


diff --git a/test/jdk/java/util/Random/RandomTestCoverage.java 
b/test/jdk/java/util/Random/RandomTestCoverage.java
index be12d188198..6e5c36e13c3 100644
--- a/test/jdk/java/util/Random/RandomTestCoverage.java
+++ b/test/jdk/java/util/Random/RandomTestCoverage.java
@@ -171,8 +171,37 @@ static void coverFactory(RandomGeneratorFactory factory) {
 boolean isSplittable = factory.isSplittable();
 
 coverRandomGenerator(factory.create());
-coverRandomGenerator(factory.create(12345L));
-coverRandomGenerator(factory.create(new byte[] {1, 2, 3, 4, 5, 6, 7, 
8}));
+
+String algo = factory.name();
+// test create(long)
+switch (algo) {
+// SecureRandom doesn't have long constructors so we expect
+// UnsupportedOperationException
+case "SecureRandom" -> {
+try {
+factory.create(12345L);
+throw new 
AssertionError("RandomGeneratorFactory.create(long) was expected" +
+"to throw UnsupportedOperationException for " + 
algo + " but didn't");
+} catch (UnsupportedOperationException uoe) {
+// expected
+}
+}
+default -> coverRandomGenerator(factory.create(12345L));
+}
+// test create(byte[])
+switch (algo) {
+// these don't have byte[] constructors so we expect 
UnsupportedOperationException
+case "Random", "SplittableRandom" -> {
+try {
+factory.create(new byte[] {1, 2, 3, 4, 5, 6, 7, 8});
+throw new 
AssertionError("RandomGeneratorFactory.create(byte[]) was expected" +
+"to throw UnsupportedOperationException for " + 
algo + " but didn't");
+} catch (UnsupportedOperationException uoe) {
+// expected
+}
+}
+default -> coverRandomGenerator(factory.create(new byte[] {1, 2, 
3, 4, 5, 6, 7, 8}));
+}
 }

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1603123836

Integrated: 8332101: Add an `@since` to `StandardOperation:REMOVE` in `jdk.dynalink`

[Nizar Benalla]

On Sat, 11 May 2024 15:39:04 GMT, Nizar Benalla  wrote:

> Code cleanup, please review this simple change. I split my changes into 1 PR 
> per module to make reviewing simpler.
> To help with the review, this was added back in 
> https://bugs.openjdk.org/browse/JDK-8191905
> TIA

This pull request has now been integrated.

Changeset: f9f8d0b4
Author:Nizar Benalla 
Committer: Jaikiran Pai 
URL:   
https://git.openjdk.org/jdk/commit/f9f8d0b48057a02923e36c8e11286b57cc72279e
Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod

8332101: Add an `@since` to `StandardOperation:REMOVE` in `jdk.dynalink`

Reviewed-by: jpai

-

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

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

[ExE Boss]

On Thu, 16 May 2024 09:01:22 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:
> 
>   Use byte for storing state and compute flags

src/java.base/share/classes/jdk/internal/lang/stable/StableAccess.java line 63:

> 61:Function extends R> original) {
> 62: return new MemoizedFunction<>(stableMap, original);
> 63: }

Maybe also rename these?
Suggestion:

public static  Supplier memoizedSupplier(StableValue stable,
   Supplier 
original) {
return new MemoizedSupplier<>(stable, original);
}

public static  IntFunction memoizedIntFunction(List> 
stableList,
 IntFunction original) {
return new MemoizedIntFunction<>(stableList, original);
}

public static  Function memoizedFunction(Map> 
stableMap,
 Function original) {
return new MemoizedFunction<>(stableMap, original);
}

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

> 727: }
> 728: }
> 729: }

Given that the [`Class​::forName​(String, boolean, ClassLoader)`] method 
doesn’t care about whether the requested class is actually exported to the 
caller, it’s possible to do the following:
Suggestion:

final class Holder {
static final Class TRUSTED_FIELD_TYPE;
static {
PrivilegedAction getPlatformClassLoader = 
ClassLoader::getPlatformClassLoader;
@SuppressWarnings("removal")
ClassLoader platformClassLoader = 
AccessController.doPrivileged(getPlatformClassLoader);

try {
TRUSTED_FIELD_TYPE = 
Class.forName("jdk.internal.lang.stable.TrustedFieldType",
false, platformClassLoader);
} catch (ClassNotFoundException e) {
throw new AssertionError(e);
}
}
}

Class declaringClass = f.getDeclaringClass();
if (declaringClass.isHidden()) {
throw new UnsupportedOperationException("can't get base address on 
a hidden class: " + f);
  

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

[Jaikiran Pai]

On Wed, 15 May 2024 09:51:23 GMT, Raffaello Giulietti  
wrote:

>> 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.

src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 
204:

> 202: new RandomGeneratorProperties(rgClass, name, group,
> 203: i, j, k, equidistribution,
> 204: flags | 
> (rgClass.isAnnotationPresent(Deprecated.class) ? DEPRECATED : 0)));

Hello Raffaello, this is the final remaining reflection usage and even this I 
think isn't required now that all the random generator implementations reside 
within java.base as an implementation detail.

I think we should just skip this annotation check here and set `DEPRECATED` bit 
on the `flags` to `0` for all implementations. When/if we do deprecate any of 
the random generators, we can just come here and switch that bit to on for the 
specific random generator when instantiating this `RandomGeneratorProperties` 
record. I had a brief look at the code and the documentation in 
`package-info.java` of `java/util/random` and we don't mention that we rely on 
the `@Deprecated`  annotation to determine whether an algorithm is deprecated. 
I think that's a good thing.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1603101449

Re: RFR: 8322732: ForkJoinPool may underutilize cores in async mode [v2]

[Sunmisc Unsafe]

On Sun, 12 May 2024 13:12:24 GMT, Doug Lea  wrote:

>> This set of changes address causes of poor utilization with small numbers of 
>> cores due to overly aggressive contention avoidance. A number of further 
>> adjustments were needed to still avoid most contention effects in 
>> deployments with large numbers of cores
>
> Doug Lea has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Address review comments

Maybe I don't quite understand, or I don't have proof, But wouldn't it be 
better if invokeAll in FIFO mode (asyncMode) traverses the Future list as a 
FIFO (currently it traverses in LIFO order)

-

PR Comment: https://git.openjdk.org/jdk/pull/19131#issuecomment-2114836374

Re: RFR: 8332101: Add an `@since` to `StandardOperation:REMOVE` in `jdk.dynalink`

[Jaikiran Pai]

On Sat, 11 May 2024 15:39:04 GMT, Nizar Benalla  wrote:

> Code cleanup, please review this simple change. I split my changes into 1 PR 
> per module to make reviewing simpler.
> To help with the review, this was added back in 
> https://bugs.openjdk.org/browse/JDK-8191905
> TIA

The change looks good to me. `StandardOperation.REMOVE` was introduced through 
https://bugs.openjdk.org/browse/JDK-8191905 which was a JDK 10 change, so 
matches with the proposed change in this PR.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19189#pullrequestreview-2060312959

Re: [External] : Re: Generalizing binary search

[Pavel Rappo]

Since you replied to my message rather than to David's, I assume you had seen 
the [issue] I linked.

Masquerading an arbitrary binary search as a binary search on a list -- a 
random access list, as Louis noted correctly -- can do something for you:

class Element implements Comparable { /* ... */ }

class MyList extends AbstractList
implements java.util.RandomAccess {

@Override
public int size() {
return Integer.MAX_VALUE;
}

@Override
public Element get(int index) {
return /* arbitrary computation */
}
}

int idx = Collections.binarySearch(new MyList().subList(0, 1024),
new Element( /* ... */ ));

But it's a trick which can do only so much; for instance:

  - it cannot help you find a lower or an upper bound (i.e. the left-most or 
right-most occurrence of a non-unique key),
  - it is hard-to-read and error-prone in its own way

[issue]: https://bugs.openjdk.org/browse/JDK-8326330

> On 15 May 2024, at 23:19, Mateusz Romanowski  
> 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 

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

[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 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.staticArrayListthrpt   10   7614.741 ?  
> 564.777  ops/us
> StableListElementBe...

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

  Use byte for storing state and compute flags

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18794/files
  - new: https://git.openjdk.org/jdk/pull/18794/files/058cfddf..80b7e081

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18794=09
 - incr: https://webrevs.openjdk.org/?repo=jdk=18794=08-09

  Stats: 42 lines in 2 files changed: 11 ins; 17 del; 14 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: 8331670: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal [v2]

[Maurizio Cimadamore]

On Thu, 16 May 2024 07:14:53 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.
>
> Alan Bateman has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains ten additional 
> commits since the last revision:
> 
>  - Add removal to DISABLED_WARNINGS
>Fail at startup if bad value specified to option
>  - Merge
>  - Update man page
>  - Update man page
>  - Fix comment
>  - Merge
>  - Merge
>  - Whitespace
>  - Initial commit

Marked as reviewed by mcimadamore (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19174#pullrequestreview-2060041163

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

[Per Minborg]

On Wed, 15 May 2024 16:26:57 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 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?

We had `byte` before but converted to `int` as the `byte` will get promoted to 
an `int` anyhow in the code logic. However, one idea would be to go back to 
using a `byte` again and also use a `byte` for the flag of computation 
invocation. This would reduce the footprint for these two fields from 8 bytes 
to 2 bytes.

-

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

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

[Per Minborg]

On Wed, 15 May 2024 15:49:22 GMT, Chen Liang  wrote:

>> 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.

Let's keep it internal.

-

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

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

[Julian Waters]

On Wed, 15 May 2024 13:32:38 GMT, Magnus Ihse Bursie  wrote:

> 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.

Oh, no worries. Sorry for sounding a little impatient.

The problem is that there are areas in the build system that will need changing 
to support hsdis compilation, not just the hsdis Makefile and autoconf file 
itself. I can try to work on minimizing the amount of changes to non-hsdis 
files (I was hoping the current changes were small enough, but it seems they 
are not), but I believe it's impossible to achieve this by only touching the 
hsdis Makefile and lib-hsdis.m4. That is, there will necessarily be changes, no 
matter how minimal, to non-hsdis files.

As for why do this at all, I was mainly driven by seeing the hack in place for 
the binutils backend on Windows. It only supports Cygwin, of which the gcc 
installation is subpar compared to the newer gcc provided by some MSYS2 
subsystems (MINGW64 gcc is primarily battle tested on MSYS2, on Cygwin it 
doesn't get as much attention and misses out further fixes and enhancements 
from the MSYS2 team, for instance it even links to the obsolete msvcrt 
library), and the hack itself has several flaws that don't seem apparent at 
first (For instance, -lpthread will link to libwinpthread.dll and result in an 
invisible dependency on that dll depending on the Windows platform, which will 
cause loading hsdis to silently fail depending on how it's loaded for seemingly 
random reasons!). I wanted to replace that (or I guess provide a better 
alternative) by adding semi proper support to the hsdis build for the more 
modern and better battle tested gcc that some MSYS2 subsystems provide, to 
streamline comp
 iling the binutils hsdis on Windows. So this is mainly about hsdis-binutils on 
Windows. hsdis-capstone I also decided to support because it's small and 
relatively easy to pile on top of the existing work, and MSYS2 also provides 
Capstone as well. The same unfortunately could not be said for hsdis-llvm, 
which was significantly more complex than Capstone is

I'm not sure where to go from here. I could support gcc for hsdis binutils by 
extending the hack that exists for Cygwin, but I _really_ don't want to do 
that. I could enhance the build system to semi properly support gcc for hsdis 
only, as I've done, but the changes will necessarily touch non hsdis files as 
well, no matter how minimal they are. I'll return this to draft for the time 
being I suppose, I'd be interested in hearing which way forward you prefer 
though

But while I work on making this change even smaller and easier to review, could 
I ask the above questions again? (Well, except for the FIXPATH one, you can 
ignore that)



> @magicus I think I might need some help here. Currently all the Cygwin stuff 
> is gated behind ifeq ($(TOOLCHAIN_TYPE), microsoft) checks... should they be 
> behind isBuildOsEnv cygwin checks instead? I don't know how to add support 
> for Cygwin gcc for most of hsdis, since it is quite different from the more 
> modern gcc distributions that are found in places like MSYS2 and MINGW64. But 
> most of the existing logic seems to accomodate more for the Microsoft 
> compiler than it is concerned about the OS environment being used, and for 
> this reason I can't tell which of the 2 checks to use for the existing hack 
> that switches from microsoft to gcc. Also, gcc doesn't require FIXPATH, but 
> Microsoft does, but I don't want to check for microsoft inside 
> TOOLCHAIN_FIND_COMPILER, what should I do instead to ensure gcc doesn't get 
> FIXPATH while Microsoft does?



> Also @magicus, what is the typical path passed to --with-binutils like on 
> Windows? Something like 
> --with-binutils=/c/Users/vertig0/Downloads/binutils-2.42 doesn't work 
> correctly, since the include path to dis-asm.h 

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

[Per Minborg]

On Wed, 15 May 2024 16:29:08 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 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?

We considered recording the entire exception instance but for security reasons, 
we ended up just recording the type of exception. I will add a comment 
explaining this in the code.

-

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

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

[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 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.staticArrayListthrpt   10   7614.741 ?  
> 564.777  ops/us
> StableListElementBe...

Per Minborg has updated the pull request incrementally with two additional 
commits since the last revision:

 - Add comment on security precaution
 - Share code paths

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18794/files
  - new: https://git.openjdk.org/jdk/pull/18794/files/d8875db7..058cfddf

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18794=08
 - incr: https://webrevs.openjdk.org/?repo=jdk=18794=07-08

  Stats: 55 lines in 1 file changed: 1 ins; 53 del; 1 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: 8331670: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal [v2]

[Jaikiran Pai]

On Thu, 16 May 2024 07:14:53 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.
>
> Alan Bateman has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains ten additional 
> commits since the last revision:
> 
>  - Add removal to DISABLED_WARNINGS
>Fail at startup if bad value specified to option
>  - Merge
>  - Update man page
>  - Update man page
>  - Fix comment
>  - Merge
>  - Merge
>  - Whitespace
>  - Initial commit

The changes in `bd74a21d` look good to me. I haven't paid close attention to 
the new benchmark.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19174#pullrequestreview-2059867530

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

[Per Minborg]

On Wed, 15 May 2024 16:31:15 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 256:
> 
>> 254: 
>> 255: @ForceInline
>> 256: private  V computeIfUnsetShared(Object provider, K key) {
> 
> Can we let suppliers share this path too, with a null key? I see this path 
> supports suppliers but supplier code path doesn't call this path.

Good suggestion!

-

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

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

[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 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.staticArrayListthrpt   10   7614.741 ?  
> 564.777  ops/us
> StableListElementBe...

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

  Rework the way compute invocation is recorded

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18794/files
  - new: https://git.openjdk.org/jdk/pull/18794/files/b845c589..d8875db7

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18794=07
 - incr: https://webrevs.openjdk.org/?repo=jdk=18794=06-07

  Stats: 40 lines in 1 file changed: 3 ins; 12 del; 25 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) [v5]

[Per Minborg]

On Wed, 15 May 2024 19:07:26 GMT, Chen Liang  wrote:

>> 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

Somehow the `@Stable` annotation sneaked into the `supplying` field. But 
actually keeping it that way and just set the value once would be better.

-

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

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

[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 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.staticArrayListthrpt   10   7614.741 ?  
> 564.777  ops/us
> StableListElementBe...

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

  Rename memoized factories

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18794/files
  - new: https://git.openjdk.org/jdk/pull/18794/files/befb2751..b845c589

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18794=06
 - incr: https://webrevs.openjdk.org/?repo=jdk=18794=05-06

  Stats: 13 lines in 5 files changed: 0 ins; 0 del; 13 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

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

[Adam Sotona]

On Thu, 16 May 2024 06:44:29 GMT, Adam Sotona  wrote:

> 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

This pull request has now been integrated.

Changeset: ee4a9d34
Author:Adam Sotona 
URL:   
https://git.openjdk.org/jdk/commit/ee4a9d34827166ff9ac04e2375058fdc08e43194
Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod

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

Reviewed-by: jpai

-

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

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

[Adam Sotona]

On Thu, 16 May 2024 07:28:20 GMT, Adam Sotona  wrote:

>> 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
>
> Adam Sotona has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   updated copyright year

Thank you for the review.

Yes, the `@see` links to the C++ sources in the verifier implementation help a 
lot and are intentional.

-

PR Comment: https://git.openjdk.org/jdk/pull/19258#issuecomment-2114271187

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

[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 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.staticArrayListthrpt   10   7614.741 ?  
> 564.777  ops/us
> StableListElementBe...

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

  Simplify exception handling and add benchmarks

-

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

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18794=05
 - incr: https://webrevs.openjdk.org/?repo=jdk=18794=04-05

  Stats: 30 lines in 3 files changed: 19 ins; 6 del; 5 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: 8321622: ClassFile.verify(byte[] bytes) throws unexpected ConstantPoolException, IAE [v2]

[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

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

  updated copyright year

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19258/files
  - new: https://git.openjdk.org/jdk/pull/19258/files/235875ca..950493a3

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19258=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=19258=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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: RFR: 8330465: Stable Values and Collections (Internal) [v5]

[Per Minborg]

On Thu, 16 May 2024 07:19:54 GMT, Per Minborg  wrote:

>> 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.
>
> One alternative would be to expose the types `StableList` and `StableMap`. 
> This would allow detection of these types if declared. This would also allow 
> us to expose the methods `computeIfUnset` as instance methods and remove the 
> somewhat strange static counterparts. I agree strict finals are better in the 
> long run.

We had other names for the memoized factories before but some people did not 
like names like `asMemoized`. 

Maybe `ofSupplier` -> `memoizedSupplier` etc. ?

-

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

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

[Per Minborg]

On Wed, 15 May 2024 16:25:04 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/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.

One alternative would be to expose the types `StableList` and `StableMap`. This 
would allow detection of these types if declared. This would also allow us to 
expose the methods `computeIfUnset` as instance methods and remove the somewhat 
strange static counterparts. I agree strict finals are better in the long run.

-

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

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

[Per Minborg]

On Wed, 15 May 2024 16:19:05 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 403:
> 
>> 401: stable.computeIfUnset(supplier);
>> 402: } catch (Throwable throwable) {
>> 403: final Thread.UncaughtExceptionHandler 
>> uncaughtExceptionHandler =
> 
> Does this exception handling differ from the default one for threads? If not, 
> I think we can safely remove this catch block, as all exceptions are just 
> propagated and computeIfUnset doesn't declare any checked exception.

Nice catch. This will reduce complexity:


@Override
public void run() {
stable.computeIfUnset(supplier);
// Exceptions are implicitly captured by the tread's
// uncaught exception handler.
}

-

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

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

[Per Minborg]

On Wed, 15 May 2024 16:11:56 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 139:
> 
>> 137: case NON_NULL: { return valueVolatile(); }
>> 138: case ERROR:{ throw StableUtil.error(this); }
>> 139: case DUMMY:{ throw shouldNotReachHere(); }
> 
> Redundant branch?

The idea here is to have the most likely value in the middle... Not sure if 
that motivates the added complexity though.

-

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

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

[Alan Bateman]

On Wed, 15 May 2024 10:21:14 GMT, Maurizio Cimadamore  
wrote:

> 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?

That might be better, I assumed the micros were being compiled an empty 
DISABLED_WARNINGS list. It means that the FFM micros won't need to be updated.

-

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

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

[Alan Bateman]

> 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.

Alan Bateman has updated the pull request with a new target base due to a merge 
or a rebase. The incremental webrev excludes the unrelated changes brought in 
by the merge/rebase. The pull request contains ten additional commits since the 
last revision:

 - Add removal to DISABLED_WARNINGS
   Fail at startup if bad value specified to option
 - Merge
 - Update man page
 - Update man page
 - Fix comment
 - Merge
 - Merge
 - Whitespace
 - Initial commit

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19174/files
  - new: https://git.openjdk.org/jdk/pull/19174/files/5d3ca0aa..bd74a21d

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19174=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=19174=00-01

  Stats: 4378 lines in 194 files changed: 2791 ins; 962 del; 625 mod
  Patch: https://git.openjdk.org/jdk/pull/19174.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19174/head:pull/19174

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

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

[Jaikiran Pai]

On Thu, 16 May 2024 06:44:29 GMT, Adam Sotona  wrote:

> 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

Hello Adam, the change looks OK to me. The file needs a copyright year update 
before integrating.

On an unrelated note, the class level javadoc has some `@see` usages which use 
URLs pointing to `https://raw.githubusercontent.com/openjdk/jdk...`. Are those 
intentional? Of course, this is an internal class so the javadoc won't be 
published.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19258#pullrequestreview-2059775369

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

[Per Minborg]

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

>> 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

It seems reasonable to assume `null` values are not constant-folded. For 
straight-out-of-the-box usage, there is no apparent significant difference as 
indicated by a new benchmark I just added:


Benchmark  Mode  Cnt  Score  Error   
Units
StableStaticBenchmark.atomic  thrpt   10   5729.683 ?  502.023  
ops/us
StableStaticBenchmark.dcl thrpt   10   6069.222 ?  951.784  
ops/us
StableStaticBenchmark.dclHolder   thrpt   10   5502.102 ? 1630.627  
ops/us
StableStaticBenchmark.stable  thrpt   10  12737.158 ? 1746.456  
ops/us <- Non-null benchmark
StableStaticBenchmark.stableHolderthrpt   10  12053.978 ? 1421.527  
ops/us
StableStaticBenchmark.stableList  thrpt   10  12443.870 ? 2084.607  
ops/us
StableStaticBenchmark.stableNull  thrpt   10  13164.232 ?  591.284  
ops/us <- Added null benchmark
StableStaticBenchmark.stableRecordHolder  thrpt   10  13638.893 ? 1250.895  
ops/us
StableStaticBenchmark.staticCHI   thrpt   10  13639.220 ? 1190.922  
ops/us


If the `null` value participates in a much larger constant-folding tree, there 
might be a significant difference. I was afraid moving the order would have 
detrimental effects on instance performance but that does not seem to be the 
case:

Checking value first:


Benchmark   Mode  Cnt Score  Error   Units
StableBenchmark.atomic thrpt   10   246.460 ?   75.417  ops/us
StableBenchmark.dclthrpt   10   243.481 ?   35.021  ops/us
StableBenchmark.stable thrpt   10  4977.693 ?  675.926  ops/us  <- 
Non-null
StableBenchmark.stableHoldingList  thrpt   10  3614.460 ?  275.140  ops/us
StableBenchmark.stableList thrpt   10  3328.155 ?  898.202  ops/us
StableBenchmark.stableListStored   thrpt   10  3842.174 ?  535.902  ops/us
StableBenchmark.stableNull thrpt   10  6217.737 ?  840.376  ops/us <- 
null
StableBenchmark.supplier   thrpt   10  9369.934 ? 1449.182  ops/us


Checking null first:


Benchmark   Mode  Cnt Score  Error   Units
StableBenchmark.atomic thrpt   10   275.952 ?   39.480  ops/us
StableBenchmark.dclthrpt   10   252.697 ?   18.645  ops/us
StableBenchmark.stable thrpt   10  5211.552 ?  315.307  ops/us <- 
Non-null
StableBenchmark.stableHoldingList  thrpt   10  3764.202 ?  224.325  ops/us
StableBenchmark.stableList thrpt   10  3689.870 ?  419.858  ops/us
StableBenchmark.stableListStored   thrpt   10  3676.182 ?  938.485  ops/us
StableBenchmark.stableNull thrpt   10  6046.935 ? 1512.391  ops/us <- 
null
StableBenchmark.supplier   thrpt   10  9202.202 ? 1479.950  ops/us


So, swapping order seems to be the right move.

-

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

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=19258=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