Re: RFR: 8274658: ISO 4217 Amendment 170 Update

[Iris Clark]

On Fri, 1 Oct 2021 18:57:28 GMT, Naoto Sato  wrote:

> This is to incorporate the ISO 4217 amendment #170, which has been released 
> today, effective immediately.

Marked as reviewed by iris (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/5790

Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v3]

[Phil Race]

On Wed, 29 Sep 2021 03:39:09 GMT, Phil Race  wrote:

>> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set 
>> the name of the application in the system menu bar.
>> 
>> Because this set shortly after the VM is running, it causes a thread safety 
>> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549
>> 
>> Since the AWT already looks for the system property 
>> "apple.awt.application.name" for this same purpose,
>> we can set that instead of the env. var and avoid the threading issue.
>> 
>> This trades the JAVA_MAIN_CLASS_ pollution of the environment for 
>> setting a system property which is visible to apps as well but it seems a 
>> reasonable trade-off since we already (implicitly) support this variable 
>> anyway in preference to the env. var.
>
> Phil Race has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8274397: Stop setting env. var JAVA_MAIN_CLASS_ in launcher code

I've now pushed the new test to verify the system property.
I've verified the test passes on my local machine but can't (until Monday) be 
sure it passes in the CI test framework because of power maintenance that has 
just started.

-

PR: https://git.openjdk.java.net/jdk/pull/5724

Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v4]

[Phil Race]

> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set 
> the name of the application in the system menu bar.
> 
> Because this set shortly after the VM is running, it causes a thread safety 
> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549
> 
> Since the AWT already looks for the system property 
> "apple.awt.application.name" for this same purpose,
> we can set that instead of the env. var and avoid the threading issue.
> 
> This trades the JAVA_MAIN_CLASS_ pollution of the environment for 
> setting a system property which is visible to apps as well but it seems a 
> reasonable trade-off since we already (implicitly) support this variable 
> anyway in preference to the env. var.

Phil Race has updated the pull request incrementally with one additional commit 
since the last revision:

  8274397: Stop setting env. var JAVA_MAIN_CLASS_ in launcher code

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5724/files
  - new: https://git.openjdk.java.net/jdk/pull/5724/files/96a5590c..4c8fb9af

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=5724=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5724=02-03

  Stats: 112 lines in 2 files changed: 112 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5724.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5724/head:pull/5724

PR: https://git.openjdk.java.net/jdk/pull/5724

Re: RFR: 8274658: ISO 4217 Amendment #170 Update

[Lance Andersen]

On Fri, 1 Oct 2021 18:57:28 GMT, Naoto Sato  wrote:

> This is to incorporate the ISO 4217 amendment #170, which has been released 
> today, effective immediately.

Marked as reviewed by lancea (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/5790

RFR: 8274658: ISO 4217 Amendment #170 Update

[Naoto Sato]

This is to incorporate the ISO 4217 amendment #170, which has been released 
today, effective immediately.

-

Commit messages:
 - 8274658: ISO 4217 Amendment #170 Update

Changes: https://git.openjdk.java.net/jdk/pull/5790/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5790=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8274658
  Stats: 15 lines in 6 files changed: 3 ins; 0 del; 12 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5790.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5790/head:pull/5790

PR: https://git.openjdk.java.net/jdk/pull/5790

Re: RFR: 8274544: Langtools command's usage were garbled on Japanese Windows

[Naoto Sato]

On Thu, 30 Sep 2021 09:36:34 GMT, Ichiroh Takiguchi  
wrote:

> JEP-400 (UTF-8 by Default) was eabled on JDK18-b13.
> After JDK18-b13, javac and some other langtool command's usage were garbled 
> on Japanese Windows.
> These commands use PrintWriter instead of standard out/err with PrintStream.

The encoding used in `Log.java` should first check whether it is run within 
console or not. If `System.console()` returns the console, its `.charset()` 
should be used instead of `native.encoding` value.
As to the jshell issue, it is a different issue than javac, so I would expect 
it to be handled with a different issue.

-

PR: https://git.openjdk.java.net/jdk/pull/5771

Re: RFR: 8274349: ForkJoinPool.commonPool() does not work with 1 CPU [v2]

[Martin Buchholz]

On Fri, 1 Oct 2021 05:10:17 GMT, David Holmes  wrote:

>> A regression introduced in Java 17 will give the default FJ pool a 
>> parallelism of zero in a uniprocessor environment. The fix restores this to 
>> a value of 1. See bug report for details.
>> 
>> Testing:
>>  - new regression test
>>  - tiers 1-3
>> 
>> Thanks,
>> David
>
> David Holmes has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Updated TCK test component from @martin

Marked as reviewed by martin (Reviewer).

I always try to avoid Thread.sleep or polling in tests, so I would write the 
test like this:

import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ForkJoinPool;

public class ForkJoinUniProcessor {
public static void main(String[] args) throws InterruptedException {
// If the default parallelism were zero then this task would not
// complete and the test will timeout.
CountDownLatch ran = new CountDownLatch(1);
ForkJoinPool.commonPool().submit(() -> ran.countDown());
ran.await();
}
}

-

PR: https://git.openjdk.java.net/jdk/pull/5784

Re: StringCoding.hasNegatives

[Claes Redestad]

On 2021-10-01 16:53, Aleksey Shipilev wrote:

On 10/1/21 4:46 PM, Brett Okken wrote:
The current pure Java implementation does two things: it provides a 
fallback
for pure-interpreter JVMs and it provides the reader with a simple 
implementation.

I'm not at all sure we'd want a complex implementation.


I thought this might be the case.


Having said that, if I were looking at a faster pure Java version of
this logic, I'd look at MethodHandles.byteArrayViewVarHandle().


I considered that, but had 2 concerns:
1. creating potential dependency/initialization order issues
2. creating/managing a constant VarHandle that would never be used
with normal compiler behavior


Also, interpreter-only VMs are likely to be pessimized by 
Var/MethodHandles, as they would not be able to fold a lot of code (e.g. 
checks) on the hotpaths. I have seen this after SHA-2 was rewritten to 
use byteViews (that one was done for simplicity, rather than directly 
for performance, I think).


Guilty as charged! I did experiment with some slightly wonky ways to
soften the blow to interpreter performance[1], but ultimately opted to
maximize simplicity.



Optimizing this does not seem worth it, in my opinion. The only reason 
to do this would be showing that it improves the interpreter performance 
so much it improves startup until compilers are coming up with the 
optimized code.




I agree with this assessment.

Thanks!
/Claes

[1] 
https://github.com/openjdk/jdk/pull/1855/commits/4b4fb2dd078eb71bd41ad3cf61b87a93575171a0

Re: StringCoding.hasNegatives

[Aleksey Shipilev]

On 10/1/21 4:46 PM, Brett Okken wrote:

The current pure Java implementation does two things: it provides a fallback
for pure-interpreter JVMs and it provides the reader with a simple 
implementation.
I'm not at all sure we'd want a complex implementation.


I thought this might be the case.


Having said that, if I were looking at a faster pure Java version of
this logic, I'd look at MethodHandles.byteArrayViewVarHandle().


I considered that, but had 2 concerns:
1. creating potential dependency/initialization order issues
2. creating/managing a constant VarHandle that would never be used
with normal compiler behavior


Also, interpreter-only VMs are likely to be pessimized by Var/MethodHandles, as they would not be 
able to fold a lot of code (e.g. checks) on the hotpaths. I have seen this after SHA-2 was rewritten 
to use byteViews (that one was done for simplicity, rather than directly for performance, I think).


Optimizing this does not seem worth it, in my opinion. The only reason to do this would be showing 
that it improves the interpreter performance so much it improves startup until compilers are coming 
up with the optimized code.


--
Thanks,
-Aleksey

Re: StringCoding.hasNegatives

[Brett Okken]

> The current pure Java implementation does two things: it provides a fallback
> for pure-interpreter JVMs and it provides the reader with a simple 
> implementation.
> I'm not at all sure we'd want a complex implementation.

I thought this might be the case.

> Having said that, if I were looking at a faster pure Java version of
> this logic, I'd look at MethodHandles.byteArrayViewVarHandle().

I considered that, but had 2 concerns:
1. creating potential dependency/initialization order issues
2. creating/managing a constant VarHandle that would never be used
with normal compiler behavior

Overall it sounds like this is not worthwhile. Thanks for taking a look.

Brett

Re: StringCoding.hasNegatives

[Andrew Haley]

On 10/1/21 1:57 PM, Brett Okken wrote:
> I know java.lang.StringCoding.hasNegatives has a
> HotSpotIntrinsicCandidate annotation/implementation, but is there
> interest/value in a faster pure java implementation?
> 
> Using Unsafe to read and compare 8 bytes at a time as a long is faster
> than the current simple implementation both in interpreter only mode
> and default hotspot compiler mode. I can provide jmh bencmark and
> results if this is worthwhile to pursue.

The current pure Java implementation does two things: it provides a fallback
for pure-interpreter JVMs and it provides the reader with a simple 
implementation.
I'm not at all sure we'd want a complex implementation.

Having said that, if I were looking at a faster pure Java version of
this logic, I'd look at MethodHandles.byteArrayViewVarHandle().

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

StringCoding.hasNegatives

[Brett Okken]

I know java.lang.StringCoding.hasNegatives has a
HotSpotIntrinsicCandidate annotation/implementation, but is there
interest/value in a faster pure java implementation?

Using Unsafe to read and compare 8 bytes at a time as a long is faster
than the current simple implementation both in interpreter only mode
and default hotspot compiler mode. I can provide jmh bencmark and
results if this is worthwhile to pursue.

Thanks,

Brett

private static final long NEGATIVE_MASK = 0x8080808080808080L;

public static boolean hasNegatives(byte[] ba, int off, int len) {
int i = off;
final int endIdx = off + len;

// align to 8 byte reads
while ((i & 7) != 0 && i < endIdx) {
if (ba[i++] < 0) {
return true;
}
}

// read 8 bytes at a time as a long (platform endian-ness does
not matter)
// to check if any have negative bit set
final Unsafe unsafe = Unsafe.getUnsafe();
for (int j = endIdx - 7; i < j; i += 8) {
final long val = unsafe.getLong(ba,
Unsafe.ARRAY_BYTE_BASE_OFFSET + i);
if ((val & NEGATIVE_MASK) != 0) {
return true;
}
}

// process trailing bytes 1 by 1
for ( ; i < endIdx; i++) {
if (ba[i] < 0) {
return true;
}
}

return false;
}

Re: RFR: 8274544: Langtools command's usage were garbled on Japanese Windows

[Ichiroh Takiguchi]

On Fri, 1 Oct 2021 12:13:03 GMT, Pavel Rappo  wrote:

>> JEP-400 (UTF-8 by Default) was eabled on JDK18-b13.
>> After JDK18-b13, javac and some other langtool command's usage were garbled 
>> on Japanese Windows.
>> These commands use PrintWriter instead of standard out/err with PrintStream.
>
> For searchability, you could fix the typo in the PR title and JBS summary: 
> grABled -> gARbled. A nit, really.

@pavelrappo Many thanks.
I changed PR and JBS.

-

PR: https://git.openjdk.java.net/jdk/pull/5771

Re: RFR: 8274544: Langtools command's usage were garbled on Japanese Windows

[Jan Lahoda]

On Fri, 1 Oct 2021 11:56:03 GMT, Jan Lahoda  wrote:

> Regarding javac, the patch to `Log.java` seems to be in a reasonable 
> direction: the write is to the physical `System.out/err` which should be 
> done(?) using the native encoding. The order of the changed lines should be 
> fixed, so that the javadoc is kept above the `initWriters` method. (As a 
> secondary comment, it maybe a matter of discussion on whether keeping the 
> native encoding in a static field is warranted here, but I don't mind it 
> much.)

I've forgot to write a note on the test, sorry: simply add `native.encoding` 
into `noResourceRequired` set in the test. The test checks that there are not 
hardcoded string that should be part of the resource bundle (and the resource 
bundle does not have unused keys), but names of system properties should be 
excluded, which is what the `noResourceRequired` set does.

-

PR: https://git.openjdk.java.net/jdk/pull/5771

Re: RFR: 8274544: Langtools command's usage were grabled on Japanese Windows

[Pavel Rappo]

On Thu, 30 Sep 2021 09:36:34 GMT, Ichiroh Takiguchi  
wrote:

> JEP-400 (UTF-8 by Default) was eabled on JDK18-b13.
> After JDK18-b13, javac and some other langtool command's usage were garbled 
> on Japanese Windows.
> These commands use PrintWriter instead of standard out/err with PrintStream.

For searchability, you could fix the typo in the PR title and JBS summary: 
grABled -> gARbled. A nit, really.

-

PR: https://git.openjdk.java.net/jdk/pull/5771

Re: RFR: 8274544: Langtools command's usage were grabled on Japanese Windows

[Jan Lahoda]

On Thu, 30 Sep 2021 09:36:34 GMT, Ichiroh Takiguchi  
wrote:

> JEP-400 (UTF-8 by Default) was eabled on JDK18-b13.
> After JDK18-b13, javac and some other langtool command's usage were garbled 
> on Japanese Windows.
> These commands use PrintWriter instead of standard out/err with PrintStream.

Regarding javac, the patch to `Log.java` seems to be in a reasonable direction: 
the write is to the physical `System.out/err` which should be done(?) using the 
native encoding. The order of the changed lines should be fixed, so that the 
javadoc is kept above the `initWriters` method. (As a secondary comment, it 
maybe a matter of discussion on whether keeping the native encoding in a static 
field is warranted here, but I don't mind it much.)

Regarding JShell, I guess I need to try to find a way to reproduce for me, so 
that I can debug. AFAIK the main process does not read/write from/to the 
console using `System.in`/`System.out`, so encoding plays no real role for the 
console, but it plays role in the communication between the agent and the main 
process.

-

PR: https://git.openjdk.java.net/jdk/pull/5771

Re: RFR: 8274606: Fix jaxp/javax/xml/jaxp/unittest/transform/SurrogateTest.java test

[Aleksey Shipilev]

On Thu, 30 Sep 2021 18:32:17 GMT, Alex Kasko  wrote:

> I was working on backporting JDK-8268457 and found minor problems with the 
> test introduced there:
> 
> 1. `compareWith*` helper methods are used without `Assert.assertTrue()` 
> wrapping, so they are effectively ignored
> 
> 2. `this.getClass().getResourceAsStream()` is used to load test input data, 
> it actually returns null in test run, so transformation is done without input 
> data
> 
> Note, that SurrogateTest.zip reproducer attached to JDK-8268457 is valid and 
> fully functional, problems likely were introduced when it was adapted into 
> test.
> 
> The change is to the test only, it wraps `compareWith*` helpers and loads 
> data the same way as XSL is loaded. Additionally `compareStringWithGold` was 
> changed to `compareLinesWithGold` to exclude possible line endings problems.
> 
> Testing: checked that the test fails when JDK-8268457 code fix is reverted, 
> checked that is passes on master.

Looks fine to me.

-

Marked as reviewed by shade (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/5779

Re: RFR: 8274525: Replace uses of StringBuffer with StringBuilder in java.xml

[Daniel Fuchs]

On Wed, 29 Sep 2021 17:56:49 GMT, Andrey Turbanov 
 wrote:

> StringBuffer is a legacy synchronized class. There are more modern 
> alternatives which perform better:
> 1. Plain String concatenation should be preferred
> 2. StringBuilder is a direct replacement to StringBuffer which generally have 
> better performance
> 
> Similar cleanups:
> 1. [JDK-8264029](https://bugs.openjdk.java.net/browse/JDK-8264029) java.base
> 2. [JDK-8264428](https://bugs.openjdk.java.net/browse/JDK-8264428) 
> java.desktop
> 3. [JDK-8264484](https://bugs.openjdk.java.net/browse/JDK-8264484) 
> jdk.hotspot.agent

Marked as reviewed by dfuchs (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/5759

Re: RFR: 8274606: Fix jaxp/javax/xml/jaxp/unittest/transform/SurrogateTest.java test

[Alex Kasko]

On Thu, 30 Sep 2021 18:32:17 GMT, Alex Kasko  wrote:

> I was working on backporting JDK-8268457 and found minor problems with the 
> test introduced there:
> 
> 1. `compareWith*` helper methods are used without `Assert.assertTrue()` 
> wrapping, so they are effectively ignored
> 
> 2. `this.getClass().getResourceAsStream()` is used to load test input data, 
> it actually returns null in test run, so transformation is done without input 
> data
> 
> Note, that SurrogateTest.zip reproducer attached to JDK-8268457 is valid and 
> fully functional, problems likely were introduced when it was adapted into 
> test.
> 
> The change is to the test only, it wraps `compareWith*` helpers and loads 
> data the same way as XSL is loaded. Additionally `compareStringWithGold` was 
> changed to `compareLinesWithGold` to exclude possible line endings problems.
> 
> Testing: checked that the test fails when JDK-8268457 code fix is reverted, 
> checked that is passes on master.

Thanks for the review!

-

PR: https://git.openjdk.java.net/jdk/pull/5779

Re: RFR: 8274349: ForkJoinPool.commonPool() does not work with 1 CPU [v2]

[Aleksey Shipilev]

On Fri, 1 Oct 2021 05:10:17 GMT, David Holmes  wrote:

>> A regression introduced in Java 17 will give the default FJ pool a 
>> parallelism of zero in a uniprocessor environment. The fix restores this to 
>> a value of 1. See bug report for details.
>> 
>> Testing:
>>  - new regression test
>>  - tiers 1-3
>> 
>> Thanks,
>> David
>
> David Holmes has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Updated TCK test component from @martin

Oh wow. Looks good!

-

Marked as reviewed by shade (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/5784