Re: DirectBufferAllocTest fails after replacement of Thread.sleep() with Thread.onSpinWait()

[Alan Bateman]

On 20/06/2022 22:58, David Holmes wrote:


Thread.onSpinWait _is_ busy-waiting, but it may (depending on 
architecture) actually execute a more (power) efficient form of 
busy-wait.


The takeaway from this exercise is that loops with sleeps can't always 
be replaced by loops that busy-wait. Functionally the logic is the 
same, but you can't just ignore the concurrency aspects.
Right, and in this case I don't think a busy wait would be very 
efficient as it would require a very high max spin count. To date 
Peter's exponential back-off has worked well and it's rare to hear about 
OOME cases caused by code that that is allocating direct buffers for 
single use. In time, I assume we will see more code using the new FFM 
API which can support deterministic memory release.


-Alan

Re: RFR: 8287982: Concurrent implicit attach from native threads crashes VM

[David Holmes]

On Thu, 16 Jun 2022 13:34:18 GMT, Alan Bateman  wrote:

> If several native threads attach to the VM at around the same time, and 
> before any threads get an automatically generated name are created, then the 
> VM may crash attempting to access the thread status. The issue exists for 
> native threads that attach explicitly with JNI AttachCurrentThread without a 
> thread name, or native threads that attach implicitly by using a function 
> pointer to do an up call.
> 
> The issue that raises its head periodically is that native threads that JNI 
> attach do the initializaiton of the Thread object in the context of the 
> attaching thread. Great care must be taken because Java code is executing in 
> the context of a Thread that is not fully initialized. The right thing is 
> probably to create the Thread object in another thread, using the service 
> thread has been mentioned. The issue at this time arises when two or more 
> native threads attempt to attach without thread names at around the same 
> time. The first thread that needs an automatically generated name triggers 
> the loading and initialization of a helper class.  If there are other threads 
> attaching at the same time then they may have to wait on the monitor which 
> can trigger the crash because the field holder with the thread status is not 
> created at this time. Crashes in monitor enter and notify have been observed. 
> Coleen has changed this code so that linking and initialization uses a mutex 
> (JDK-8288064) so
  this specific crash doesn't duplicate in the main line. The short term fix 
for openjdk/jdk19 is to reorder the initialization so that field holder with 
the thread status is created before setting the name.
> 
> Creating a jtreg test with the conditions to duplicate this issue is 
> complicated. The jtreg main wrapper creates the main thread with an 
> automatically generated thread name before it runs the test main method. This 
> is the reason that the test needs to launch a new VM with the right setup to 
> exercise both explicit and implicit attach.

Looks good.

And I confirmed that the VM code will correctly handle a null name from the 
current Java thread.

Thanks.

-

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.org/jdk19/pull/28

Re: RFR: 8287982: Concurrent implicit attach from native threads crashes VM

[Alan Bateman]

On Tue, 21 Jun 2022 07:09:50 GMT, David Holmes  wrote:

> And I confirmed that the VM code will correctly handle a null name from the 
> current Java thread.

Thanks, I checked that one.

One thing that I'm wondering for the test is whether to move it to 
test/hotspot/jtreg/runtime/jni/ so it lives with the other tests for JNI. I had 
expected to find other tests for AttachCurrentThread in that location but we 
don't.

-

PR: https://git.openjdk.org/jdk19/pull/28

Integrated: 8288628: Unnecessary Hashtable usage in ConditionalSpecialCasing

[Andrey Turbanov]

On Wed, 15 Jun 2022 19:49:52 GMT, Andrey Turbanov  wrote:

> If a thread-safe implementation is not needed, it is recommended to use 
> HashMap in place of Hashtable.
> `ConditionalSpecialCasing.entryTable` is read-only Map which is modified only 
> in `static` block. It means we can safely replace it with HashMap.

This pull request has now been integrated.

Changeset: 0f801fe6
Author:Andrey Turbanov 
URL:   
https://git.openjdk.org/jdk/commit/0f801fe6fd2fcc181121f9846f6869ca3a03e18a
Stats: 4 lines in 1 file changed: 1 ins; 1 del; 2 mod

8288628: Unnecessary Hashtable usage in ConditionalSpecialCasing

Reviewed-by: naoto, jpai

-

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

Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v3]

[Severin Gehwolf]

On Wed, 18 May 2022 18:14:52 GMT, Severin Gehwolf  wrote:

>> Please review this change to the cgroup v1 subsystem which makes it more 
>> resilient on some of the stranger systems. Unfortunately, I wasn't able to 
>> re-create a similar system as the reporter. The idea of using the longest 
>> substring match for the cgroupv1 file paths was based on the fact that on 
>> systemd systems processes run in separate scopes and the maven forked test 
>> runner might exhibit this property. For that it makes sense to use the 
>> common ancestor path. Nothing changes in the common cases where the 
>> `cgroup_path` matches `_root` and when the `_root` is `/` (container the 
>> former, host system the latter).
>> 
>> In addition, the code paths which are susceptible to throw NPE have been 
>> hardened to catch those situations. Should it happen in the future it makes 
>> more sense (to me) to not have accurate container detection in favor of 
>> continuing to keep running.
>> 
>> Finally, with the added unit-tests a bug was uncovered on the "substring" 
>> match case of cgroup paths in hotspot. `p` returned from `strstr` can never 
>> point to `_root` as it's used as the "needle" to find in "haystack" 
>> `cgroup_path` (not the other way round).
>> 
>> Testing:
>> - [x] Added unit tests
>> - [x] GHA
>> - [x] Container tests on cgroups v1 Linux. Continue to pass
>
> Severin Gehwolf has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - Refactor hotspot gtest
>  - Separate into function. Fix comment.

Don't close this yet please, bot.

-

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

Re: RFR: 8288589: Files.readString ignores encoding errors for UTF-16 [v2]

[Alan Bateman]

On Sat, 18 Jun 2022 00:31:06 GMT, Naoto Sato  wrote:

>> This is a regression caused by the fix to 
>> [JDK-8286287](https://bugs.openjdk.org/browse/JDK-8286287), which assumed 
>> the method `String.decodeWithDecoder()` was only invoked with cs.REPLACE 
>> mode based on the comment "should not happen". Possibly this refers to the 
>> `String(byte[], int, int, Charset)` constructor, which specifically mentions 
>> the `REPLACE` mode. However, the method is invoked with 
>> `String.newStringNoRepl()` and it should NOT replace the malformed input 
>> (duh!). The fix is to throw an `Error` for the former case as before the 
>> regression, and `CharacterCodingException` for the latter via an 
>> `IllegalArgumentException`.
>> In fact, `Files.readString()` stopped throwing a `MalformedInputException` 
>> since JDK17 with the fix to JDK-8259842, which started throwing an `Error`.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added c2b test

> I looked for similar test cases but ended up finding nothing. Thus I created 
> a new test case here. Problem is that they are issued through 
> `SharedSecrets`, which are effectively _APIs_ but treated as private methods 
> which leads to insufficient testing. I now think that I would add not only 
> b2c test, but also c2b test (for getBytesNoRepl() method) is needed. I will 
> modify the test case to include it.
 
My comment was mostly asking if we need to add more tests for 
Files.writeString. I would have expected a test for that method to fail with 
this bug. Maybe we need to create a new issue to expand the tests for this 
method.


> BTW, I found a spec bug in `Files.writeString()` w/o `Charset` argument, 
> where the `@throws` clause reads: 
> "[IOException](https://urldefense.com/v3/__https://download.java.net/java/early_access/jdk19/docs/api/java.base/java/io/IOException.html__;!!ACWV5N9M2RV99hQ!PzgRBNWQwotDtM0GEFtu0XuT7pUqLpKjdwt6awkfFaeZEhXxhdEPL5FhuTeNGYrUHdaeM-_qWB2PccxVdZIFLQ$
>  ) - if an I/O error occurs writing to or creating the file, or the text 
> cannot be encoded using the specified charset", but there is no specified 
> charset there.

It looks like description for IOException was copied from the 4-arg writeString 
to the 3-arg writeString. I've created JDK-8288836 to track this.

One other thing, this is a regression in 19 so I assume the PR should be 
against openjdk/jdk19 rather than the main line.

-

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

Re: RFR: JDK-8288746: HttpClient resources could be reclaimed more eagerly

[Daniel Fuchs]

On Mon, 20 Jun 2022 18:28:36 GMT, Сергей Цыпанов  wrote:

> Shouldn't we use `Reference.refersTo()` instead of `Reference.get() == smth` 
> in lines 428 and 501?

Hi Sergey, we could, and I'll do it for consistency.
Though in those two cases it doesn't really matter since the facade referent is 
not supposed to be null (it can't since there still exist strong references), 
so keeping the reference alive longer than it should is not a concern there.

-

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

Re: RFR: 8288723: Avoid redundant ConcurrentHashMap.get call in java.time

[Attila Szegedi]

On Mon, 20 Jun 2022 15:01:55 GMT, liach  wrote:

>> src/java.base/share/classes/java/time/format/DateTimeTextProvider.java line 
>> 319:
>> 
>>> 317: store = prev;
>>> 318: }
>>> 319: }
>> 
>> You could do better here and use `computeIfAbsent` with `createStore` as its 
>> lambda. You could even change the signature of `createStore` to take 
>> `Entry` as its parameter and then you could have this 
>> method be:
>> 
>> private Object findStore(TemporalField field, Locale locale) {
>> return CACHE.computeIfAbsent(createEntry(field, locale), 
>> this::createStore);
>> }
>> 
>> ...
>> 
>> private Object createStore(Entry entry) {
>> ...
>> }
>> 
>> This applies to most other changes you made, the one in `ZoneOffset` is the 
>> only one that's slightly different because there you have adjustment of 
>> `ID_CACHE` too.
>
> This behaves slightly different from the old initialization; the concurrent 
> hash map blocks when the mapping function is run, just in case if 
> non-blocking instantiation is what we want. If that's not a problem, I would 
> prefer szegedi's suggestion.

@liach advance apologies for nitpicking: `ConcurrentHashMap` doesn't in general 
block while the mapping function runs. It can block _some_ concurrent updates, 
namely those that [hash to the same 
bin](https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/0f801fe6fd2fcc181121f9846f6869ca3a03e18a/src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java*L324__;Iw!!ACWV5N9M2RV99hQ!JBDYAOKXAgaJoVrQyfzX6Cpak1HLEt3HG254q8COTDzpz7Ui1_Jm8iUTLqBdiluzP1PMfTEA4n31sDYGYuZr2t8$
 ) and it won't block concurrent reads. I'm not saying the concern you raised 
isn't valid, just wanted to clarify that the situation is not nearly as grave 
as overall blocking; after all it ain't a `Collections.synchronizedMap` :-)

-

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

RFR: 8288838: jpackage: file association additional arguments

[Alex Kasko]

jpackage implementation of file association on Windows currently passes a 
selected filename as an only argument to associated executable.

It is proposed to introduce additional option in file association property file 
to allow optionally support additional arguments using `%*` batch wildcard.

Note, current implementation, while fully functional, is only a **DRAFT** one, 
it is not ready for integration in this form. I would appreciate any guidance 
on the following points:

 - option naming inside a properties file, currently `pass-all-args` is used
 - option naming in a bundler parameter implementation, it is not clear if it 
should introduce a new group of "file association windows specific options" 
next to the existing "file association mac specific options" group
 - test organization to cover the new option: currently it is included inside 
`FileAssociationTest` and piggybacks on the existing (and unrelated) 
`includeDescription` parameter; it is not clear whether it should be done in a 
separate test and whether to include runs for every parameter combination
 - test run implementation: currently arguments are checked when a file with 
associated extension is invoked from command line; it is not clear whether it 
would be more appropriate instead to create a desktop shortcut with the same 
command as a target and to invoke it with `java.aws.Desktop`

Also please note, that full install/uninstall run is currently enabled in 
`FileAssociationTest`, it is intended to be used only in a draft code during 
the development and to be removed (to use the same "install or unpack" logic as 
other tests) in a final version.

Testing:
 
- [x] test to cover new logic is included
- [x] ran jtreg:jdk/tools/jpackage with no new failures

-

Commit messages:
 - jpackage: file association additional arguments (draft)

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

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

[JMH results for IndexedLinkedList]

[Rodion Efremov]

Hi,

Data structure: IndexedLinkedList

Benchmark: IndexedLinkedListBenchmark


Benchmark output:
https://github.com/coderodde/indexedLinkedList/#benchmark-output

>From the benchmark output, we can judge that IndexedLinkedList outperforms
both java.util.LinkedList and the Apache Commons Collections TreeList. It,
however, does not seem to supersede the java.util.ArrayList.

Basically, I would expect the IndexedLinkedList to beat the ArrayList where
we have a lot of following operations:

   - addFirst(E)
   - add(int, E)
   - remove(int)

So, what do you think? I am getting anywhere with that?

Best regards,
rodde

Re: DirectBufferAllocTest fails after replacement of Thread.sleep() with Thread.onSpinWait()

[Сергей Цыпанов]

> The takeaway from this exercise is that loops with sleeps can't always be 
> replaced by loops that busy-wait.
> Functionally the logic is the same, but you can't just ignore the concurrency 
> aspects.

Thanks for explanation!

> Which indicates yielding is having a similar effect to sleeping - though I 
> would expect it to be less pronounced.

May I ask one last question about this: in general case when we don't care 
about particular wait time (like in j.n.Bits.reserveMemory())
should I prefer Thread.yield() over both Thread.sleep() and Thread.onSpinWait() 
providing that Thread.sleep() is expensive
and Thread.onSpinWait() might cause undesirable side effects?

Regards,
Sergey Tsypanov

RFR: 8288840: StructureViolationException should not link to fork method

[Alan Bateman]

StructureViolationException has a `@see` link to a fork method that does not 
throw this exception. The link should be removed for JDK 19. We'll add the link 
back once the JEP for Extent-Local Variables is integrated.

-

Commit messages:
 - Initial commit

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

PR: https://git.openjdk.org/jdk19/pull/52

Re: Handling of USR2 signal in JVM on Linux

[David Lloyd]

On Fri, Jun 17, 2022 at 1:05 AM Thomas Stüfe  wrote:
>>
>>
>> > >
>> > > I see your point. I dislike crashes, especially ones they can be
>> > > reliably triggered from outside. You never can be sure about the
>> > > security implications either.
>> > >
>> > > If you dislike ignoring the signal, and since the default action for
>> > > SIGUSR1+2 is process termination, why not exit the VM with an error
>> > > message instead? At least we don’t have what looks like a random
>> > > segfault, and we could print out the sender pid  too.
>> >
>> > We could ... but then do we do that for every other signal that can be
>> > thrown at the JVM from externally and which will leads to crashes? Where
>> > do you stop?
>>
>
> It's a very limited set. I'd say we limit this to
>
> 1) signal for which we registered handlers: the handlers should at least not 
> crash or vanish the VM without a trace
> 2) signals which may conceivably be sent to the VM in the normal course of 
> events: if the default action is to terminate the VM, we should handle them
>
> Note that for (2), we already do this for some signals. We explicitly handle 
> SIGPIPE (https://bugs.openjdk.org/browse/JDK-4229104) and SIGXFSZ 
> (https://bugs.openjdk.org/browse/JDK-6499219) because they kill the VM.
>
>>
>> Well, playing devil's advocate, why not? Would it be more complex than 
>> adding a
>>
>> if (si.si_pid != my_cached_pid) {
>> // it came from Outside, let's exit with 128+signum
>> // ...
>> }
>>
>> to the top of the signal handler(s)?
>>
>> --
>> - DML • he/him
>>
>
> I filed a JBS issue and a PR: 
> https://github.com/openjdk/jdk/pull/9181#issuecomment-1157353776.
>
> I went with a different approach, just using Thread::current=NULL as an 
> indicator. I was worried that Unix implementations may leave the si_pid field 
> empty, or that it gets filled with the sender's kernel thread id, not the 
> process pid. Just verified that on Linux, its really actually the process id 
> (so the main threads id).

`siginfo_t` is POSIX so I wouldn't worry much about it being anything
other than the sender's process ID in the year 2022. Testing that
`Thread::current=NULL` tells you that the thread that the signal was
delivered to doesn't have a Java thread, I guess, but it doesn't tell
you if the signal was invalid in the case where the target thread does
have a corresponding Java thread; after all, a process signal sent
from outside could be sent to any thread as you say. Checking the
sending PID tells you with certainty whether or not the signal did
come from outside, which is really the salient question AFAICT.

-- 
- DML • he/him

Re: RFR: 8287982: Concurrent implicit attach from native threads crashes VM

[Robbin Ehn]

On Thu, 16 Jun 2022 13:34:18 GMT, Alan Bateman  wrote:

> If several native threads attach to the VM at around the same time, and 
> before any threads get an automatically generated name are created, then the 
> VM may crash attempting to access the thread status. The issue exists for 
> native threads that attach explicitly with JNI AttachCurrentThread without a 
> thread name, or native threads that attach implicitly by using a function 
> pointer to do an up call.
> 
> The issue that raises its head periodically is that native threads that JNI 
> attach do the initializaiton of the Thread object in the context of the 
> attaching thread. Great care must be taken because Java code is executing in 
> the context of a Thread that is not fully initialized. The right thing is 
> probably to create the Thread object in another thread, using the service 
> thread has been mentioned. The issue at this time arises when two or more 
> native threads attempt to attach without thread names at around the same 
> time. The first thread that needs an automatically generated name triggers 
> the loading and initialization of a helper class.  If there are other threads 
> attaching at the same time then they may have to wait on the monitor which 
> can trigger the crash because the field holder with the thread status is not 
> created at this time. Crashes in monitor enter and notify have been observed. 
> Coleen has changed this code so that linking and initialization uses a mutex 
> (JDK-8288064) so
  this specific crash doesn't duplicate in the main line. The short term fix 
for openjdk/jdk19 is to reorder the initialization so that field holder with 
the thread status is created before setting the name.
> 
> Creating a jtreg test with the conditions to duplicate this issue is 
> complicated. The jtreg main wrapper creates the main thread with an 
> automatically generated thread name before it runs the test main method. This 
> is the reason that the test needs to launch a new VM with the right setup to 
> exercise both explicit and implicit attach.

Looks good.

-

Marked as reviewed by rehn (Reviewer).

PR: https://git.openjdk.org/jdk19/pull/28

Re: Bug JDK-8176553

[Aleksei Efimov]

Hi Ricardo,

Thank you for discovering and reproducing the issue - it looks like JDK-8176553 
was incomplete in solving how referrals are limited. At first glance the 
attached patch (fix + test) looks like a good change to have - I think it is PR 
worthy :)
I've logged a bug for your change - https://bugs.openjdk.org/browse/JDK-8288895.

Best,
Aleksei


From: core-libs-dev  on behalf of Sean Mullan 

Sent: Friday, June 17, 2022 3:15 PM
To: core-libs-dev 
Subject: Fwd: Bug JDK-8176553

[reposting to core-libs-dev as this is in the JNDI/LDAP component]


 Forwarded Message 
Subject: Bug JDK-8176553
Date: Fri, 17 Jun 2022 14:23:11 +0200
From: Ricardo Martin Camarero 
To: security-...@openjdk.org

Hi!

I decided to send an email to the security-dev email list as ldap is
involved. Please redirect me to other list if it is not the proper audience.

The last last days I have faced the same issue that is commented in
JDK-8176553 [1]. Although it is cataloged as fixed in 12, the issue is
not solved in the openjdk master branch yet. You can test with this
simple project [2]. The project is using apache-ds and creating 12
branches with redirects from one to the other. The search should be
limited to 5 hops but you will see that all of them are followed (12).
Therefore, If there are loops, the search hangs indefinitely. So
JDK-8176553 is not fixed completely. You just need `mvn clean test` to
reproduce the problem in that project.

I have investigated and I think the attached little patch fixes the
issue. Mainly the `LdapReferralException` is not stopping the referral
loop in some situations. I have added a test in the jndi jtreg
test-suite to check everything works OK; `make test
TEST=jtreg:jdk/com/sun/jndi/ldap/ReferralLimitSearchTest.java`

WDYT? Is the PR worthy?

Thanks in advance!


[1] https://bugs.openjdk.org/browse/JDK-8176553
[2]
https://urldefense.com/v3/__https://github.com/rmartinc/apache-ds-referrals__;!!ACWV5N9M2RV99hQ!IZkp5q_gOAeIP8Y9Gvr8aniLloG51lZJwlG1yN6BRDyHHLpyr0W64TDMUPAzoPu7dOBOyJrNcKYmwaOF9REM3oA$

Re: RFR: 8288697: Clarify lifecycle of buffer segments and loader lookup

[Alan Bateman]

On Fri, 17 Jun 2022 21:39:16 GMT, Maurizio Cimadamore  
wrote:

> This is a dependent PR containing a cleanup of the so called *heap sessions*. 
> Heap sessions are used in two cases:
> 
> * when a buffer segment is created, to keep the original buffer instance 
> reachable
> * when a loader symbol lookup is created, to keep the classloader instance 
> reachable
> 
> Spinning new sessions in these cases seems unnecessary, and inconsistent with 
> what we do for segments backed by on-heap arrays, whose session is set to the 
> global session. In that case, it's up to the segment to keep the underlying 
> array reachable. I think that's a better model.
> 
> This patch adds the ability to attach Object references to native and mapped 
> memory segments, so that we can attach loader/byte buffer instances to these 
> segments, keeping them alive. This means that, in these cases we can go back 
> to just use the global memory session, like we do for array segments.
> 
> This simplifies the implementation, makes the documentation more consistent, 
> and also simplifies the user model, as it removes a concept (of heap session) 
> that is not really documented anywhere. In fact, the javadoc for 
> MemorySegment::ofBuffer claims, (wrongly!) that the session of the resulting 
> segment is the global session - but that's not the case.

src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 148:

> 146:  * 
> 147:  * The symbols obtained from the returned loader lookup are backed 
> by the {@linkplain MemorySession#global() global session}.
> 148:  * 

Does the spec update mean this should have a CSR?

-

PR: https://git.openjdk.org/jdk19/pull/39

Re: RFR: 8288697: Clarify lifecycle of buffer segments and loader lookup

[Maurizio Cimadamore]

On Tue, 21 Jun 2022 16:10:24 GMT, Alan Bateman  wrote:

>> This is a dependent PR containing a cleanup of the so called *heap 
>> sessions*. Heap sessions are used in two cases:
>> 
>> * when a buffer segment is created, to keep the original buffer instance 
>> reachable
>> * when a loader symbol lookup is created, to keep the classloader instance 
>> reachable
>> 
>> Spinning new sessions in these cases seems unnecessary, and inconsistent 
>> with what we do for segments backed by on-heap arrays, whose session is set 
>> to the global session. In that case, it's up to the segment to keep the 
>> underlying array reachable. I think that's a better model.
>> 
>> This patch adds the ability to attach Object references to native and mapped 
>> memory segments, so that we can attach loader/byte buffer instances to these 
>> segments, keeping them alive. This means that, in these cases we can go back 
>> to just use the global memory session, like we do for array segments.
>> 
>> This simplifies the implementation, makes the documentation more consistent, 
>> and also simplifies the user model, as it removes a concept (of heap 
>> session) that is not really documented anywhere. In fact, the javadoc for 
>> MemorySegment::ofBuffer claims, (wrongly!) that the session of the resulting 
>> segment is the global session - but that's not the case.
>
> src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 148:
> 
>> 146:  * 
>> 147:  * The symbols obtained from the returned loader lookup are backed 
>> by the {@linkplain MemorySession#global() global session}.
>> 148:  * 
> 
> Does the spec update mean this should have a CSR?

Yep - note that PR is in draft mode as we need to think through what changes we 
actually want to pursue.

-

PR: https://git.openjdk.org/jdk19/pull/39

Re: RFR: 8288761: SegmentAllocator:allocate(long bytesSize) not throwing IAEx when bytesSize < 0

[Paul Sandoz]

On Mon, 20 Jun 2022 21:22:42 GMT, Maurizio Cimadamore  
wrote:

> The various SegmentAllocator subclasses do not check for the constraints 
> mentioned in the javadoc.
> This patch pulls the checks into a common routine in the Utils class, and 
> then call that check routine from all the `allocate` implementations.

Marked as reviewed by psandoz (Reviewer).

-

PR: https://git.openjdk.org/jdk19/pull/50

Re: RFR: 8288840: StructureViolationException should not link to fork method

[Paul Sandoz]

On Tue, 21 Jun 2022 11:20:49 GMT, Alan Bateman  wrote:

> StructureViolationException has a `@see` link to a fork method that does not 
> throw this exception. The link should be removed for JDK 19. We'll add the 
> link back once the JEP for Extent-Local Variables is integrated.

Marked as reviewed by psandoz (Reviewer).

-

PR: https://git.openjdk.org/jdk19/pull/52

Re: RFR: 8288840: StructureViolationException should not link to fork method

[Mandy Chung]

On Tue, 21 Jun 2022 11:20:49 GMT, Alan Bateman  wrote:

> StructureViolationException has a `@see` link to a fork method that does not 
> throw this exception. The link should be removed for JDK 19. We'll add the 
> link back once the JEP for Extent-Local Variables is integrated.

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.org/jdk19/pull/52

RFR: 8288529: broken link in java.xml

[Joe Wang]

Some of the links cause redirection, iso.org in particular took 1-2 seconds, 
that looks like was longer than the wait time of the doccheck and docs link 
checker. Changed it to the current homepage to avoid redirection. Also changed 
other links within this package. 

I'm limiting the change to this package only as iso.org only appears in it and 
other redirections have not caused any issue (tests showed they were almost 
instant).

-

Commit messages:
 - 8288529: broken link in java.xml

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

PR: https://git.openjdk.org/jdk19/pull/55

Re: RFR: 8287868: Localized names update in COMPAT locale provider

[Naoto Sato]

Ping.

Naoto

On 6/10/22 2:05 PM, Naoto Sato wrote:

As the name suggests, `COMPAT` locale provider keeps compatibility with JDK8's 
locale data (before CLDR).  This is useful for legacy applications, but some of 
its data got obsolete for later locale data updates, such as the country name 
change for `Eswatini` (formerly known as `Swaziland`). This PR is to fix those 
names up-to-date from CLDR. More specifically, changes are made for `language`, 
`country`, `script` display names in `Locale` class, and `Currency` display 
names. Localized names used for `TimeZone`s and `Currency` symbols (such as 
`$`) are not modified so that `format`/`parse` should work as with JDK8.

-

Commit messages:
  - 8287868: Localized names update in COMPAT locale provider

Changes: https://git.openjdk.org/jdk/pull/9134/files
  Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=9134&range=00
   Issue: https://bugs.openjdk.org/browse/JDK-8287868
   Stats: 9930 lines in 65 files changed: 17 ins; 7 del; 9906 mod
   Patch: https://git.openjdk.org/jdk/pull/9134.diff
   Fetch: git fetch https://git.openjdk.org/jdk pull/9134/head:pull/9134

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

Re: RFR: 8288529: broken link in java.xml

[Iris Clark]

On Tue, 21 Jun 2022 16:52:52 GMT, Joe Wang  wrote:

> Some of the links cause redirection, iso.org in particular took 1-2 seconds, 
> that looks like was longer than the wait time of the doccheck and docs link 
> checker. Changed it to the current homepage to avoid redirection. Also 
> changed other links within this package. 
> 
> I'm limiting the change to this package only as iso.org only appears in it 
> and other redirections have not caused any issue (tests showed they were 
> almost instant).

Marked as reviewed by iris (Reviewer).

-

PR: https://git.openjdk.org/jdk19/pull/55

Re: RFR: 8288723: Avoid redundant ConcurrentHashMap.get call in java.time

[liach]

On Tue, 21 Jun 2022 09:35:34 GMT, Attila Szegedi  wrote:

>> This behaves slightly different from the old initialization; the concurrent 
>> hash map blocks when the mapping function is run, just in case if 
>> non-blocking instantiation is what we want. If that's not a problem, I would 
>> prefer szegedi's suggestion.
>
> @liach advance apologies for nitpicking: `ConcurrentHashMap` doesn't in 
> general block while the mapping function runs. It can block _some_ concurrent 
> updates, namely those that [hash to the same 
> bin](https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/0f801fe6fd2fcc181121f9846f6869ca3a03e18a/src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java*L324__;Iw!!ACWV5N9M2RV99hQ!M8p-0NV01EacnHQD8Y5232_Rt40p54kS34M63XfK0EUN2_xI4QBo2zVeveUTw_ZurSoBFa079sXr7GdmArz-$
>  ) and it won't block concurrent reads. I'm not saying the concern you raised 
> isn't valid, just wanted to clarify that the situation is not nearly as grave 
> as overall blocking; after all it ain't a `Collections.synchronizedMap` :-)

Yeah, since `putIfAbsent` may block `get` calls, the blocking of 
`computeIfAbsent` is minuscule as long as the creation code is efficient 
enough. Just be careful that when writing the lambda for `computeIfAbsent`, 
preferably write static lambdas (that doesn't use `this`, instances, or local 
variables) so that one lambda can be reused over time than having to create a 
new one on each invocation.

-

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

Re: RFR: 8288529: broken link in java.xml

[Lance Andersen]

On Tue, 21 Jun 2022 16:52:52 GMT, Joe Wang  wrote:

> Some of the links cause redirection, iso.org in particular took 1-2 seconds, 
> that looks like was longer than the wait time of the doccheck and docs link 
> checker. Changed it to the current homepage to avoid redirection. Also 
> changed other links within this package. 
> 
> I'm limiting the change to this package only as iso.org only appears in it 
> and other redirections have not caused any issue (tests showed they were 
> almost instant).

Marked as reviewed by lancea (Reviewer).

-

PR: https://git.openjdk.org/jdk19/pull/55

Re: RFR: 8287868: Localized names update in COMPAT locale provider [v3]

[Iris Clark]

On Mon, 13 Jun 2022 22:58:43 GMT, Naoto Sato  wrote:

>> As the name suggests, `COMPAT` locale provider keeps compatibility with 
>> JDK8's locale data (before CLDR).  This is useful for legacy applications, 
>> but some of its data got obsolete for later locale data updates, such as the 
>> country name change for `Eswatini` (formerly known as `Swaziland`). This PR 
>> is to fix those names up-to-date from CLDR. More specifically, changes are 
>> made for `language`, `country`, `script` display names in `Locale` class, 
>> and `Currency` display names. Localized names used for `TimeZone`s and 
>> `Currency` symbols (such as `$`) are not modified so that `format`/`parse` 
>> should work as with JDK8.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed some failing tests

Marked as reviewed by iris (Reviewer).

-

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

Re: RFR: JDK-8288746: HttpClient resources could be reclaimed more eagerly

[Brent Christian]

On Mon, 20 Jun 2022 14:09:27 GMT, Daniel Fuchs  wrote:

> Hi,
> 
> Please find here a patch that should help the HttpClient's SelectorManager 
> thread to terminate more timely, allowing the resources consumed by the 
> client to be released earlier.
> 
> The idea is to use a Cleaner registered with the HttpClientFacade to wakeup 
> the Selector when the facade is being gc'ed.
> Some tests have been modified to wait for the selector manager thread to 
> shutdown, and some of them have been observed to timeout when the fix is not 
> in place.

test/jdk/java/net/httpclient/AsFileDownloadTest.java line 207:

> 205: System.gc();
> 206: if (queue.remove(100) == ref) break;
> 207: }

You might consider using the ForceGC test utility, here and in 
DigestEchoClient.java. E.g.:

 * @library /test/lib/
...
ForceGC gc = new ForceGC();
gc.await(ref.refersTo(null));

-

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

Re: RFR: JDK-8288746: HttpClient resources could be reclaimed more eagerly

[Daniel Fuchs]

On Tue, 21 Jun 2022 17:31:59 GMT, Brent Christian  wrote:

>> Hi,
>> 
>> Please find here a patch that should help the HttpClient's SelectorManager 
>> thread to terminate more timely, allowing the resources consumed by the 
>> client to be released earlier.
>> 
>> The idea is to use a Cleaner registered with the HttpClientFacade to wakeup 
>> the Selector when the facade is being gc'ed.
>> Some tests have been modified to wait for the selector manager thread to 
>> shutdown, and some of them have been observed to timeout when the fix is not 
>> in place.
>
> test/jdk/java/net/httpclient/AsFileDownloadTest.java line 207:
> 
>> 205: System.gc();
>> 206: if (queue.remove(100) == ref) break;
>> 207: }
> 
> You might consider using the ForceGC test utility, here and in 
> DigestEchoClient.java. E.g.:
> 
>  * @library /test/lib/
> ...
> ForceGC gc = new ForceGC();
> gc.await(ref.refersTo(null));

Well - no - because ForceGC is using a different object and a different Cleaner 
thread. So it's much more efficient to wait on the reference queue (= the time 
to wait will be shorter, so risks of exceeding the global jtreg timeout will be 
lower).

-

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

Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v31]

[Joe Darcy]

> This is an early review of changes to better model JVM access flags, that is 
> "modifiers" like public, protected, etc. but explicitly at a VM level.
> 
> Language level modifiers and JVM level access flags are closely related, but 
> distinct. There are concepts that overlap in the two domains (public, 
> private, etc.), others that only have a language-level modifier (sealed), and 
> still others that only have an access flag (synthetic).
> 
> The existing java.lang.reflect.Modifier class is inadequate to model these 
> subtleties. For example, the bit positions used by access flags on different 
> kinds of elements overlap (such as "volatile" for fields and "bridge" for 
> methods. Just having a raw integer does not provide sufficient context to 
> decode the corresponding language-level string. Methods like 
> Modifier.methodModifiers() were introduced to cope with this situation.
> 
> With additional modifiers and flags on the horizon with projects like 
> Valhalla, addressing the existent modeling deficiency now ahead of time is 
> reasonable before further strain is introduced.
> 
> This PR in its current form is meant to give the overall shape of the API. It 
> is missing implementations to map from, say, method modifiers to access 
> flags, taking into account overlaps in bit positions.
> 
> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in 
> once the API is further along.

Joe Darcy 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 47 additional commits since the 
last revision:

 - Expand scope of tests.
 - Merge branch 'master' into JDK-8266670
 - Add module-related tests.
 - Fix typo Exe-Boss spotted in review feedback.
 - Merge branch 'master' into JDK-8266670
 - Appease jcheck.
 - Expand regression tests.
 - Merge branch 'master' into JDK-8266670
 - Respond to review feedback from Roger Riggs.
 - Respond to review feedback.
 - ... and 37 more: https://git.openjdk.org/jdk/compare/ccdd6f93...98f12bb9

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/7445/files
  - new: https://git.openjdk.org/jdk/pull/7445/files/81bfc6d2..98f12bb9

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=7445&range=30
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=7445&range=29-30

  Stats: 804 lines in 36 files changed: 279 ins; 458 del; 67 mod
  Patch: https://git.openjdk.org/jdk/pull/7445.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/7445/head:pull/7445

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

Re: RFR: 8287868: Localized names update in COMPAT locale provider [v3]

[Joe Wang]

On Mon, 13 Jun 2022 22:58:43 GMT, Naoto Sato  wrote:

>> As the name suggests, `COMPAT` locale provider keeps compatibility with 
>> JDK8's locale data (before CLDR).  This is useful for legacy applications, 
>> but some of its data got obsolete for later locale data updates, such as the 
>> country name change for `Eswatini` (formerly known as `Swaziland`). This PR 
>> is to fix those names up-to-date from CLDR. More specifically, changes are 
>> made for `language`, `country`, `script` display names in `Locale` class, 
>> and `Currency` display names. Localized names used for `TimeZone`s and 
>> `Currency` symbols (such as `$`) are not modified so that `format`/`parse` 
>> should work as with JDK8.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed some failing tests

Marked as reviewed by joehw (Reviewer).

-

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

Re: RFR: 8288589: Files.readString ignores encoding errors for UTF-16 [v3]

[Naoto Sato]

> This is a regression caused by the fix to 
> [JDK-8286287](https://bugs.openjdk.org/browse/JDK-8286287), which assumed the 
> method `String.decodeWithDecoder()` was only invoked with cs.REPLACE mode 
> based on the comment "should not happen". Possibly this refers to the 
> `String(byte[], int, int, Charset)` constructor, which specifically mentions 
> the `REPLACE` mode. However, the method is invoked with 
> `String.newStringNoRepl()` and it should NOT replace the malformed input 
> (duh!). The fix is to throw an `Error` for the former case as before the 
> regression, and `CharacterCodingException` for the latter via an 
> `IllegalArgumentException`.
> In fact, `Files.readString()` stopped throwing a `MalformedInputException` 
> since JDK17 with the fix to JDK-8259842, which started throwing an `Error`.

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

  Adding a test case in ReadWriteString.java

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/9193/files
  - new: https://git.openjdk.org/jdk/pull/9193/files/e1afd207..f6520503

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

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

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

Re: RFR: 8288529: broken link in java.xml

[Naoto Sato]

On Tue, 21 Jun 2022 16:52:52 GMT, Joe Wang  wrote:

> Some of the links cause redirection, iso.org in particular took 1-2 seconds, 
> that looks like was longer than the wait time of the doccheck and docs link 
> checker. Changed it to the current homepage to avoid redirection. Also 
> changed other links within this package. 
> 
> I'm limiting the change to this package only as iso.org only appears in it 
> and other redirections have not caused any issue (tests showed they were 
> almost instant).

Marked as reviewed by naoto (Reviewer).

-

PR: https://git.openjdk.org/jdk19/pull/55

Re: RFR: 8288589: Files.readString ignores encoding errors for UTF-16 [v2]

[Naoto Sato]

On Tue, 21 Jun 2022 08:55:09 GMT, Alan Bateman  wrote:

> My comment was mostly asking if we need to add more tests for 
> Files.writeString. I would have expected a test for that method to fail with 
> this bug. Maybe we need to create a new issue to expand the tests for this 
> method.

Added a test case in ReadWriteString.java, which is the unit test for 
`Files.read/writeString()` methods.

> It looks like description for IOException was copied from the 4-arg 
> writeString to the 3-arg writeString. I've created JDK-8288836 to track this.

Thanks for filing the issue.

> One other thing, this is a regression in 19 so I assume the PR should be 
> against openjdk/jdk19 rather than the main line.

Since this PR already got a few approvals, I will backport the changeset to the 
jdk19 line after this PR gets integrated.

-

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

Re: RFR: 8288589: Files.readString ignores encoding errors for UTF-16 [v3]

[Andrey Turbanov]

On Tue, 21 Jun 2022 18:12:23 GMT, Naoto Sato  wrote:

>> This is a regression caused by the fix to 
>> [JDK-8286287](https://bugs.openjdk.org/browse/JDK-8286287), which assumed 
>> the method `String.decodeWithDecoder()` was only invoked with cs.REPLACE 
>> mode based on the comment "should not happen". Possibly this refers to the 
>> `String(byte[], int, int, Charset)` constructor, which specifically mentions 
>> the `REPLACE` mode. However, the method is invoked with 
>> `String.newStringNoRepl()` and it should NOT replace the malformed input 
>> (duh!). The fix is to throw an `Error` for the former case as before the 
>> regression, and `CharacterCodingException` for the latter via an 
>> `IllegalArgumentException`.
>> In fact, `Files.readString()` stopped throwing a `MalformedInputException` 
>> since JDK17 with the fix to JDK-8259842, which started throwing an `Error`.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Adding a test case in ReadWriteString.java

test/jdk/java/lang/String/NoReplTest.java line 47:

> 45: private final static Charset WINDOWS_1252 = 
> Charset.forName("windows-1252");
> 46: 
> 47: @Test

Looks confusing that annotation goes before javadoc

-

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

Integrated: 8288761: SegmentAllocator:allocate(long bytesSize) not throwing IAEx when bytesSize < 0

[Maurizio Cimadamore]

On Mon, 20 Jun 2022 21:22:42 GMT, Maurizio Cimadamore  
wrote:

> The various SegmentAllocator subclasses do not check for the constraints 
> mentioned in the javadoc.
> This patch pulls the checks into a common routine in the Utils class, and 
> then call that check routine from all the `allocate` implementations.

This pull request has now been integrated.

Changeset: d7b43af5
Author:Maurizio Cimadamore 
URL:   
https://git.openjdk.org/jdk19/commit/d7b43af5914d88e5410f33db8b56f4dabdfec25d
Stats: 59 lines in 5 files changed: 43 ins; 13 del; 3 mod

8288761: SegmentAllocator:allocate(long bytesSize) not throwing IAEx when 
bytesSize < 0

Reviewed-by: psandoz

-

PR: https://git.openjdk.org/jdk19/pull/50

Integrated: 8287971: Throw exception for missing values in .jpackage.xml

[Alexander Matveev]

On Mon, 13 Jun 2022 17:01:48 GMT, Alexander Matveev  
wrote:

> - Error will be thrown if app image is generated with another JDK version or 
> has malformed .jpackage.xml.
>  - Re-fixed as Alexey suggested in 
> https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/9098__;!!ACWV5N9M2RV99hQ!MCPPHJCD6t-Japw98nbgIUCpUdM03Z6WDLlmGq6mT87cGQ31fpSgwcvIJwM7-dHEgrXUuQYej_UD5bWVI3a6r1MKgw$
>  .

This pull request has now been integrated.

Changeset: 70008da6
Author:Alexander Matveev 
URL:   
https://git.openjdk.org/jdk19/commit/70008da6b47c371c4d15162ca38e1521cd09acf9
Stats: 207 lines in 11 files changed: 96 ins; 76 del; 35 mod

8287971: Throw exception for missing values in .jpackage.xml

Reviewed-by: asemenyuk

-

PR: https://git.openjdk.org/jdk19/pull/9

Re: RFR: 8288589: Files.readString ignores encoding errors for UTF-16 [v4]

[Naoto Sato]

> This is a regression caused by the fix to 
> [JDK-8286287](https://bugs.openjdk.org/browse/JDK-8286287), which assumed the 
> method `String.decodeWithDecoder()` was only invoked with cs.REPLACE mode 
> based on the comment "should not happen". Possibly this refers to the 
> `String(byte[], int, int, Charset)` constructor, which specifically mentions 
> the `REPLACE` mode. However, the method is invoked with 
> `String.newStringNoRepl()` and it should NOT replace the malformed input 
> (duh!). The fix is to throw an `Error` for the former case as before the 
> regression, and `CharacterCodingException` for the latter via an 
> `IllegalArgumentException`.
> In fact, `Files.readString()` stopped throwing a `MalformedInputException` 
> since JDK17 with the fix to JDK-8259842, which started throwing an `Error`.

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

  Refined the test, adding more variations.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/9193/files
  - new: https://git.openjdk.org/jdk/pull/9193/files/f6520503..84d127be

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

  Stats: 25 lines in 1 file changed: 14 ins; 4 del; 7 mod
  Patch: https://git.openjdk.org/jdk/pull/9193.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/9193/head:pull/9193

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

Re: RFR: 8288589: Files.readString ignores encoding errors for UTF-16 [v5]

[Naoto Sato]

> This is a regression caused by the fix to 
> [JDK-8286287](https://bugs.openjdk.org/browse/JDK-8286287), which assumed the 
> method `String.decodeWithDecoder()` was only invoked with cs.REPLACE mode 
> based on the comment "should not happen". Possibly this refers to the 
> `String(byte[], int, int, Charset)` constructor, which specifically mentions 
> the `REPLACE` mode. However, the method is invoked with 
> `String.newStringNoRepl()` and it should NOT replace the malformed input 
> (duh!). The fix is to throw an `Error` for the former case as before the 
> regression, and `CharacterCodingException` for the latter via an 
> `IllegalArgumentException`.
> In fact, `Files.readString()` stopped throwing a `MalformedInputException` 
> since JDK17 with the fix to JDK-8259842, which started throwing an `Error`.

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

  Moved `@Test` annotations

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/9193/files
  - new: https://git.openjdk.org/jdk/pull/9193/files/84d127be..864806ef

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

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

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

Re: RFR: 8288589: Files.readString ignores encoding errors for UTF-16 [v3]

[Naoto Sato]

On Tue, 21 Jun 2022 20:59:40 GMT, Andrey Turbanov  wrote:

>> Naoto Sato has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Adding a test case in ReadWriteString.java
>
> test/jdk/java/lang/String/NoReplTest.java line 47:
> 
>> 45: private final static Charset WINDOWS_1252 = 
>> Charset.forName("windows-1252");
>> 46: 
>> 47: @Test
> 
> Looks confusing that annotation goes before javadoc

Fixed.

-

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

RFR: Merge jdk19

[Jesper Wilhelmsson]

Forwardport JDK 19 -> JDK 20

-

Commit messages:
 - Merge
 - 8287971: Throw exception for missing values in .jpackage.xml
 - 8288761: SegmentAllocator:allocate(long bytesSize) not throwing IAEx when 
bytesSize < 0
 - 8288754: GCC 12 fails to build zReferenceProcessor.cpp
 - 8286103: VThreadMonitorTest fails "assert(!current->cont_fastpath() || 
(current->cont_fastpath_thread_state() && 
!interpreted_native_or_deoptimized_on_stack(current))) failed"
 - 8278053: 
serviceability/jvmti/vthread/ContStackDepthTest/ContStackDepthTest.java failing 
in loom repo with Xcomp
 - 8288532: additional review changes for JDK-8286830
 - 8288139: JavaThread touches oop after GC barrier is detached
 - 8288497: add support for JavaThread::is_oop_safe()
 - 8288531: Empty spans in mobile navigation markup
 - ... and 2 more: https://git.openjdk.org/jdk/compare/9f8bfab2...cfe52474

The webrevs contain the adjustments done while merging with regards to each 
parent branch:
 - master: https://webrevs.openjdk.org/?repo=jdk&pr=9227&range=00.0
 - jdk19: https://webrevs.openjdk.org/?repo=jdk&pr=9227&range=00.1

Changes: https://git.openjdk.org/jdk/pull/9227/files
  Stats: 363 lines in 31 files changed: 197 ins; 103 del; 63 mod
  Patch: https://git.openjdk.org/jdk/pull/9227.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/9227/head:pull/9227

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

Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v32]

[Joe Darcy]

> This is an early review of changes to better model JVM access flags, that is 
> "modifiers" like public, protected, etc. but explicitly at a VM level.
> 
> Language level modifiers and JVM level access flags are closely related, but 
> distinct. There are concepts that overlap in the two domains (public, 
> private, etc.), others that only have a language-level modifier (sealed), and 
> still others that only have an access flag (synthetic).
> 
> The existing java.lang.reflect.Modifier class is inadequate to model these 
> subtleties. For example, the bit positions used by access flags on different 
> kinds of elements overlap (such as "volatile" for fields and "bridge" for 
> methods. Just having a raw integer does not provide sufficient context to 
> decode the corresponding language-level string. Methods like 
> Modifier.methodModifiers() were introduced to cope with this situation.
> 
> With additional modifiers and flags on the horizon with projects like 
> Valhalla, addressing the existent modeling deficiency now ahead of time is 
> reasonable before further strain is introduced.
> 
> This PR in its current form is meant to give the overall shape of the API. It 
> is missing implementations to map from, say, method modifiers to access 
> flags, taking into account overlaps in bit positions.
> 
> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in 
> once the API is further along.

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

  Improve support and tests for Class objects representing arrays and 
primitives.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/7445/files
  - new: https://git.openjdk.org/jdk/pull/7445/files/98f12bb9..9d72436e

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=7445&range=31
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=7445&range=30-31

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

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

Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]

[Joe Darcy]

On Fri, 29 Apr 2022 18:49:26 GMT, Mandy Chung  wrote:

>> Joe Darcy 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 26 additional commits 
>> since the last revision:
>> 
>>  - Respond to review feedback.
>>  - Merge branch 'master' into JDK-8266670
>>  - Make workding changes suggested in review feedback.
>>  - Merge branch 'master' into JDK-8266670
>>  - Typo fix; add implSpec to Executable.
>>  - Appease jcheck.
>>  - Fix some bugs found by inspection, docs cleanup.
>>  - Merge branch 'master' into JDK-8266670
>>  - Initial support for accessFlags methods
>>  - Add mask to access flag functionality.
>>  - ... and 16 more: https://git.openjdk.org/jdk/compare/217d0507...14980605
>
> src/java.base/share/classes/java/lang/Class.java line 1328:
> 
>> 1326:  * @since 19
>> 1327:  */
>> 1328: public Set accessFlags() {
> 
> The access flags of a hidden class has no difference than that of a normal 
> class.  A hidden class is created with normal `ClassFile` except that it's 
> created via `Lookup::defineHiddenClass` API. 
> 
> I think the `Class::accessFlags` method should specify the access flags for 
> primitive type, void, and array classes as `Class::getModifiers` specified.

Expanded the spec of Class.accessFlags() to explicitly cover primitives and 
arrays. Small implementation update needed; test coverage expanded accordingly. 
Thanks.

-

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

Re: RFR: Merge jdk19 [v2]

[Jesper Wilhelmsson]

> Forwardport JDK 19 -> JDK 20

Jesper Wilhelmsson 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 106 additional 
commits since the last revision:

 - Merge
 - 8288537: Move Devirtualizer out of hotspot/share/memory/iterator.hpp
   
   Reviewed-by: stefank, coleenp
 - 8288599: com/sun/management/OperatingSystemMXBean/TestTotalSwap.java: 
Expected total swap size ... but getTotalSwapSpaceSize returned ...
   
   Reviewed-by: sspitsyn, kevinw
 - 8288687: (fc) Unix version ofFileChannelImpl.transferTo0() should should 
return IOS_UNSUPPORTED if not Linux, macOS, nor AIX
   
   Reviewed-by: alanb
 - 8288209: SSL debug message wrong about unsupported authentication scheme
   
   Reviewed-by: djelinski, jnimeh
 - 8288628: Unnecessary Hashtable usage in ConditionalSpecialCasing
   
   Reviewed-by: naoto, jpai
 - 8288556: VM crashes if it gets sent SIGUSR2 from outside
   
   Reviewed-by: dholmes, lucy
 - 8288724: Prevent NullPointerException in 
serviceability/tmtools/jstack/DaemonThreadTest.java if jstack process fails
   
   Reviewed-by: kevinw, sspitsyn
 - 8288601: Consolidate static/dynamic archive tables
   
   Reviewed-by: ccheung
 - 8287672: jtreg test com/sun/jndi/ldap/LdapPoolTimeoutTest.java fails 
intermittently in nightly run
   
   Reviewed-by: dfuchs, aefimov
 - ... and 96 more: https://git.openjdk.org/jdk/compare/af364408...cfe52474

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/9227/files
  - new: https://git.openjdk.org/jdk/pull/9227/files/cfe52474..cfe52474

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

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

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

Integrated: Merge jdk19

[Jesper Wilhelmsson]

On Tue, 21 Jun 2022 21:47:17 GMT, Jesper Wilhelmsson  
wrote:

> Forwardport JDK 19 -> JDK 20

This pull request has now been integrated.

Changeset: 2bf5c9a6
Author:Jesper Wilhelmsson 
URL:   
https://git.openjdk.org/jdk/commit/2bf5c9a6877b51377a535c6021a9e38549c89029
Stats: 363 lines in 31 files changed: 197 ins; 103 del; 63 mod

Merge

-

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

Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v33]

[Joe Darcy]

> This is an early review of changes to better model JVM access flags, that is 
> "modifiers" like public, protected, etc. but explicitly at a VM level.
> 
> Language level modifiers and JVM level access flags are closely related, but 
> distinct. There are concepts that overlap in the two domains (public, 
> private, etc.), others that only have a language-level modifier (sealed), and 
> still others that only have an access flag (synthetic).
> 
> The existing java.lang.reflect.Modifier class is inadequate to model these 
> subtleties. For example, the bit positions used by access flags on different 
> kinds of elements overlap (such as "volatile" for fields and "bridge" for 
> methods. Just having a raw integer does not provide sufficient context to 
> decode the corresponding language-level string. Methods like 
> Modifier.methodModifiers() were introduced to cope with this situation.
> 
> With additional modifiers and flags on the horizon with projects like 
> Valhalla, addressing the existent modeling deficiency now ahead of time is 
> reasonable before further strain is introduced.
> 
> This PR in its current form is meant to give the overall shape of the API. It 
> is missing implementations to map from, say, method modifiers to access 
> flags, taking into account overlaps in bit positions.
> 
> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in 
> once the API is further along.

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

  Remove implSpec tag from Executable.accessFlags since the class is sealed.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/7445/files
  - new: https://git.openjdk.org/jdk/pull/7445/files/9d72436e..617c9aaf

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=7445&range=32
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=7445&range=31-32

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

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

Re: RFR: 8283726: x86_64 intrinsics for compareUnsigned method in Integer and Long [v3]

[Quan Anh Mai]

> Hi,
> 
> This patch implements intrinsics for `Integer/Long::compareUnsigned` using 
> the same approach as the JVM does for long and floating-point comparisons. 
> This allows efficient and reliable usage of unsigned comparison in Java, 
> which is a basic operation and is important for range checks such as 
> discussed in #8620 .
> 
> Thank you very much.

Quan Anh Mai has updated the pull request incrementally with one additional 
commit since the last revision:

  add comparison for direct value of compare

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/9068/files
  - new: https://git.openjdk.org/jdk/pull/9068/files/b5627135..0ab881ac

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

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

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

Re: RFR: 8283726: x86_64 intrinsics for compareUnsigned method in Integer and Long

[Quan Anh Mai]

On Fri, 10 Jun 2022 00:05:53 GMT, Sandhya Viswanathan 
 wrote:

>> I have added a benchmark for the intrinsic. The result is as follows, thanks 
>> a lot:
>> 
>> Before  After
>> Benchmark (size)  Mode  Cnt  Score   Error  Score   
>> Error  Units
>> Integers.compareUnsigned 500  avgt   15  0.527 ± 0.002  0.498 ± 
>> 0.011  us/op
>> Longs.compareUnsigned500  avgt   15  0.677 ± 0.014  0.561 ± 
>> 0.006  us/op
>
> @merykitty Could you please also add the micro benchmark where 
> compareUnsigned result is stored directly in an integer and show the 
> performance of that?

Thanks @sviswa7 for the suggestion, the results of getting the value of 
`compareUnsigned` directly is as follow:

   Before  After
Benchmark   (size)  Mode  Cnt   Score   Error  Score   
Error  Units
Integers.compareUnsignedDirect 500  avgt   15   0.639 ± 0.022  0.626 ± 
0.002  us/op
Longs.compareUnsignedDirect500  avgt   15   0.672 ± 0.011  0.609 ± 
0.004  us/op

-

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

Integrated: 8288529: broken link in java.xml

[Joe Wang]

On Tue, 21 Jun 2022 16:52:52 GMT, Joe Wang  wrote:

> Some of the links cause redirection, iso.org in particular took 1-2 seconds, 
> that looks like was longer than the wait time of the doccheck and docs link 
> checker. Changed it to the current homepage to avoid redirection. Also 
> changed other links within this package. 
> 
> I'm limiting the change to this package only as iso.org only appears in it 
> and other redirections have not caused any issue (tests showed they were 
> almost instant).

This pull request has now been integrated.

Changeset: 9e2d9ac5
Author:Joe Wang 
URL:   
https://git.openjdk.org/jdk19/commit/9e2d9ac59a19caa52fc661542d4257a7473636d7
Stats: 7 lines in 1 file changed: 0 ins; 0 del; 7 mod

8288529: broken link in java.xml

Reviewed-by: iris, lancea, naoto

-

PR: https://git.openjdk.org/jdk19/pull/55

Re: RFR: 8288744: Remove tools/jlink/plugins/CompressorPluginTest.java from problemlist

[Jaikiran Pai]

On Tue, 21 Jun 2022 06:40:36 GMT, Jaikiran Pai  wrote:

> Can I please get a review for this change which proposes to remove the 
> `CompressorPluginTest` from the problemlist?
> 
> This test was problem listed since it had intermittently failed with odd 
> exception in the past (https://bugs.openjdk.org/browse/JDK-8247407). Over the 
> past several months I have attempted multiple runs of this test (with very 
> high test repeats) both locally as well as a CI setup to see if this is still 
> reproducible. So far, I haven't been able to reproduce it. Given the odd 
> exception it was throwing originally, it's hard to say what the issue could 
> have been (and whether it got fixed in recent times), but since it isn't 
> reproducible now, I would like to remove this from the problemlist. If it 
> does fail again, I'll go ahead and collect some additional details to help 
> narrow this down.

Thank you Alan for the review.

-

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

Integrated: 8288744: Remove tools/jlink/plugins/CompressorPluginTest.java from problemlist

[Jaikiran Pai]

On Tue, 21 Jun 2022 06:40:36 GMT, Jaikiran Pai  wrote:

> Can I please get a review for this change which proposes to remove the 
> `CompressorPluginTest` from the problemlist?
> 
> This test was problem listed since it had intermittently failed with odd 
> exception in the past (https://bugs.openjdk.org/browse/JDK-8247407). Over the 
> past several months I have attempted multiple runs of this test (with very 
> high test repeats) both locally as well as a CI setup to see if this is still 
> reproducible. So far, I haven't been able to reproduce it. Given the odd 
> exception it was throwing originally, it's hard to say what the issue could 
> have been (and whether it got fixed in recent times), but since it isn't 
> reproducible now, I would like to remove this from the problemlist. If it 
> does fail again, I'll go ahead and collect some additional details to help 
> narrow this down.

This pull request has now been integrated.

Changeset: affbd72a
Author:Jaikiran Pai 
URL:   
https://git.openjdk.org/jdk/commit/affbd72aa3dce80e2ad54ff775c6f7469f38b05b
Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod

8288744: Remove tools/jlink/plugins/CompressorPluginTest.java from problemlist

Reviewed-by: alanb

-

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

Re: DirectBufferAllocTest fails after replacement of Thread.sleep() with Thread.onSpinWait()

[David Holmes]

On 21/06/2022 9:35 pm, Сергей Цыпанов wrote:

The takeaway from this exercise is that loops with sleeps can't always be 
replaced by loops that busy-wait.
Functionally the logic is the same, but you can't just ignore the concurrency 
aspects.


Thanks for explanation!


Which indicates yielding is having a similar effect to sleeping - though I 
would expect it to be less pronounced.


May I ask one last question about this: in general case when we don't care 
about particular wait time (like in j.n.Bits.reserveMemory())
should I prefer Thread.yield() over both Thread.sleep() and Thread.onSpinWait() 
providing that Thread.sleep() is expensive
and Thread.onSpinWait() might cause undesirable side effects?


Thread.yield() may also have "undesirable side-effects" as you have no 
idea for how long the thread that yields will be off-cpu.


If you need to slow down the current thread and guarantee some other 
thread has a chance to make progress then Thread.sleep is best.


Busy-waits/yields/onSpinWait should only be used to impose very brief 
pauses on a thread that needs another thread to do something to enable 
forward progress - e.g for spinlocks on multicore systems. Using them 
when flow control is needed can be problematic as you found in this test.


Cheers,
David
-


Regards,
Sergey Tsypanov