Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v13]

2021-06-11 Thread Yi Yang
On Fri, 11 Jun 2021 18:05:45 GMT, Igor Veresov  wrote:

> I guess you need to do the "integrate" command again.

Okay,thank you all for taking time to look at this

-

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


Integrated: 8265518: C1: Intrinsic support for Preconditions.checkIndex

2021-06-11 Thread Yi Yang
On Thu, 22 Apr 2021 06:55:41 GMT, Yi Yang  wrote:

> The JDK codebase re-created many variants of checkIndex(`grep -I -r 
> 'cehckIndex' jdk/`). A notable variant is java.nio.Buffer.checkIndex, which 
> annotated with @IntrinsicCandidate and it only has a corresponding C1 
> intrinsic version.
> 
> In fact, there is an utility method 
> `jdk.internal.util.Preconditions.checkIndex`(wrapped by 
> java.lang.Objects.checkIndex) that behaves the same as these variants of 
> checkIndex, we can replace these re-created variants of checkIndex by 
> Objects.checkIndex, it would significantly reduce duplicated code and enjoys 
> performance improvement because Preconditions.checkIndex is 
> @IntrinsicCandidate and it has a corresponding intrinsic method in HotSpot.
> 
> But, the problem is currently HotSpot only implements the C2 version of 
> Preconditions.checkIndex. To reuse it global-widely in JDK code, I think we 
> can firstly implement its C1 counterpart. There are also a few kinds of stuff 
> we can do later:
> 
> 1. Replace all variants of checkIndex by Objects.checkIndex in the whole JDK 
> codebase.
> 2. Remove Buffer.checkIndex and obsolete/deprecate InlineNIOCheckIndex flag
> 
> Testing: cds, compiler and jdk

This pull request has now been integrated.

Changeset: 5cee23a9
Author:Yi Yang 
URL:   
https://git.openjdk.java.net/jdk/commit/5cee23a9ed0b7fe2657be7492d9c1f78fcd02ebf
Stats: 347 lines in 11 files changed: 250 ins; 78 del; 19 mod

8265518: C1: Intrinsic support for Preconditions.checkIndex

Reviewed-by: dfuchs, iveresov

-

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


Re: RFR: 8268626: Remove native pre-jdk9 support for jtreg failure handler

2021-06-11 Thread Erik Joelsson
On Fri, 11 Jun 2021 18:44:38 GMT, Leonid Mesnik  wrote:

> jtreg failure handler uses native lib to implement getPid for preJDK9. 
> Currently. it is not needed and might be removed.

Marked as reviewed by erikj (Reviewer).

-

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


RFR: 8268626: Remove native pre-jdk9 support for jtreg failure handler

2021-06-11 Thread Leonid Mesnik
jtreg failure handler uses native lib to implement getPid for preJDK9. 
Currently. it is not needed and might be removed.

-

Commit messages:
 - native lib support removed.

Changes: https://git.openjdk.java.net/jdk/pull/4476/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4476=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8268626
  Stats: 100 lines in 4 files changed: 0 ins; 96 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4476.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4476/head:pull/4476

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


Re: [jdk17] RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling [v2]

2021-06-11 Thread Vicente Romero
On Fri, 11 Jun 2021 18:17:10 GMT, Vicente Romero  wrote:

>> This PR is a copy of [PR-4416](https://github.com/openjdk/jdk/pull/4416) 
>> which was intended to openjdk/jdk.
>> 
>> Please review this PR which is syncing the implementation of 
>> `DirectMethodHandleDesc.Kind.valueOf(int)` and 
>> `DirectMethodHandleDesc.Kind.valueOf(int, boolean)` with its spec. My 
>> reading of the method's spec is that if the value of the refKind parameter 
>> is: MethodHandleInfo.REF_invokeInterface, then 
>> DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a 
>> value of true for its second argument, currently it is invoked with false 
>> regardless of the value of the refKind parameter
>> 
>> TIA
>
> Vicente Romero has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   updating after review comments

@mlchung I have uploaded a new commit, thanks for your comments

-

PR: https://git.openjdk.java.net/jdk17/pull/29


Re: [jdk17] RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling [v2]

2021-06-11 Thread Vicente Romero
> This PR is a copy of [PR-4416](https://github.com/openjdk/jdk/pull/4416) 
> which was intended to openjdk/jdk.
> 
> Please review this PR which is syncing the implementation of 
> `DirectMethodHandleDesc.Kind.valueOf(int)` and 
> `DirectMethodHandleDesc.Kind.valueOf(int, boolean)` with its spec. My reading 
> of the method's spec is that if the value of the refKind parameter is: 
> MethodHandleInfo.REF_invokeInterface, then 
> DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a 
> value of true for its second argument, currently it is invoked with false 
> regardless of the value of the refKind parameter
> 
> TIA

Vicente Romero has updated the pull request incrementally with one additional 
commit since the last revision:

  updating after review comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk17/pull/29/files
  - new: https://git.openjdk.java.net/jdk17/pull/29/files/b6b3f87c..8ed4cdb3

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk17=29=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=29=00-01

  Stats: 12 lines in 1 file changed: 0 ins; 9 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/29.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/29/head:pull/29

PR: https://git.openjdk.java.net/jdk17/pull/29


Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v13]

2021-06-11 Thread Igor Veresov
On Wed, 9 Jun 2021 08:53:40 GMT, Yi Yang  wrote:

>> The JDK codebase re-created many variants of checkIndex(`grep -I -r 
>> 'cehckIndex' jdk/`). A notable variant is java.nio.Buffer.checkIndex, which 
>> annotated with @IntrinsicCandidate and it only has a corresponding C1 
>> intrinsic version.
>> 
>> In fact, there is an utility method 
>> `jdk.internal.util.Preconditions.checkIndex`(wrapped by 
>> java.lang.Objects.checkIndex) that behaves the same as these variants of 
>> checkIndex, we can replace these re-created variants of checkIndex by 
>> Objects.checkIndex, it would significantly reduce duplicated code and enjoys 
>> performance improvement because Preconditions.checkIndex is 
>> @IntrinsicCandidate and it has a corresponding intrinsic method in HotSpot.
>> 
>> But, the problem is currently HotSpot only implements the C2 version of 
>> Preconditions.checkIndex. To reuse it global-widely in JDK code, I think we 
>> can firstly implement its C1 counterpart. There are also a few kinds of 
>> stuff we can do later:
>> 
>> 1. Replace all variants of checkIndex by Objects.checkIndex in the whole JDK 
>> codebase.
>> 2. Remove Buffer.checkIndex and obsolete/deprecate InlineNIOCheckIndex flag
>> 
>> Testing: cds, compiler and jdk
>
> Yi Yang has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   more comment

I guess you need to do the "integrate" command again.

-

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


Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v13]

2021-06-11 Thread Igor Veresov
On Wed, 9 Jun 2021 08:53:40 GMT, Yi Yang  wrote:

>> The JDK codebase re-created many variants of checkIndex(`grep -I -r 
>> 'cehckIndex' jdk/`). A notable variant is java.nio.Buffer.checkIndex, which 
>> annotated with @IntrinsicCandidate and it only has a corresponding C1 
>> intrinsic version.
>> 
>> In fact, there is an utility method 
>> `jdk.internal.util.Preconditions.checkIndex`(wrapped by 
>> java.lang.Objects.checkIndex) that behaves the same as these variants of 
>> checkIndex, we can replace these re-created variants of checkIndex by 
>> Objects.checkIndex, it would significantly reduce duplicated code and enjoys 
>> performance improvement because Preconditions.checkIndex is 
>> @IntrinsicCandidate and it has a corresponding intrinsic method in HotSpot.
>> 
>> But, the problem is currently HotSpot only implements the C2 version of 
>> Preconditions.checkIndex. To reuse it global-widely in JDK code, I think we 
>> can firstly implement its C1 counterpart. There are also a few kinds of 
>> stuff we can do later:
>> 
>> 1. Replace all variants of checkIndex by Objects.checkIndex in the whole JDK 
>> codebase.
>> 2. Remove Buffer.checkIndex and obsolete/deprecate InlineNIOCheckIndex flag
>> 
>> Testing: cds, compiler and jdk
>
> Yi Yang has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   more comment

Alright, tests pass now. I think we're good to go.

-

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


Re: [jdk17] RFR: 8268342: java/foreign/channels/TestAsyncSocketChannels.java fails with "IllegalStateException: This segment is already closed"

2021-06-11 Thread Chris Hegarty
On Fri, 11 Jun 2021 16:27:07 GMT, Daniel Fuchs  wrote:

>> There is the possibility for a race in the test, where the outbound socket 
>> buffer is still being filled, after 5 seconds, when the main test thread 
>> tries to close the resource scope.
>> 
>> The test has been updated to set the send/receive buffer sizes in an 
>> attempt/hint to limit the accepted/connected socket buffer sizes. Actual 
>> buffer sizes in use will likely be larger due to TCP auto-tuning, but the 
>> hint typically reduces the overall scaled sizes. This is primarily to 
>> stabilize outstanding write operations. Additionally, to ameliorate the 
>> wait-for-writebuf-to-fill situation, the dumb sleep 5secs has been replaced 
>> with a heuristic that checks that the number of bytes written quiesces. With 
>> these changes, the test successfully passes several thousands of runs.
>
> test/jdk/java/foreign/channels/TestAsyncSocketChannels.java line 283:
> 
>> 281: readNBytes(asc2, bytesWritten.get());
>> 282: assertTrue(scope.isAlive());
>> 283: awaitOutstandingWrites(outstandingWriteOps);
> 
> An alternative would be to delegate the call to latch.countDown() to the 
> TestHandler subclass, and call it at the end of completed() only when 
> outstandingWriteOps.decrementAndGet() == 0; As it stands the latch is 
> released with the first call to `completed` (instead of being released with 
> the last one) - and that's what makes this method necessary.

Strictly speaking it wasn't necessary to touch this area of code to resolve the 
failures that we're seeing - I decided to refactor this into a separate method 
to improve readability of the test logic. Here again, awaiting for completion 
is not strictly necessary, just good practice to ensure that all threads and 
operations associated with the test scenario complete before the next scenario. 
I'll see if this can be simplified as you suggest.

-

PR: https://git.openjdk.java.net/jdk17/pull/30


Re: [jdk17] RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling

2021-06-11 Thread Vicente Romero
On Fri, 11 Jun 2021 15:15:12 GMT, Vicente Romero  wrote:

>> This PR is a copy of [PR-4416](https://github.com/openjdk/jdk/pull/4416) 
>> which was intended to openjdk/jdk.
>> 
>> Please review this PR which is syncing the implementation of 
>> `DirectMethodHandleDesc.Kind.valueOf(int)` and 
>> `DirectMethodHandleDesc.Kind.valueOf(int, boolean)` with its spec. My 
>> reading of the method's spec is that if the value of the refKind parameter 
>> is: MethodHandleInfo.REF_invokeInterface, then 
>> DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a 
>> value of true for its second argument, currently it is invoked with false 
>> regardless of the value of the refKind parameter
>> 
>> TIA
>
> src/java.base/share/classes/java/lang/constant/DirectMethodHandleDesc.java 
> line 143:
> 
>> 141: }
>> 142: if (kind.refKind == refKind &&
>> 143: (refKind != REF_invokeStatic || refKind != 
>> REF_invokeSpecial || kind.isInterface == isInterface)){
> 
> @mlchung 13 hours ago Member
> 
> It reads to me that the static initializer tries to set up the table such 
> that valueOf(refKind, isInterface) should find the proper kind to return 
> except this:
> 
> // There is not a specific Kind for interfaces
> if (kind == VIRTUAL) kind = INTERFACE_VIRTUAL;
> 
> This changes the entry for REF_invokeVirtual to kind INTERFACE_VIRTUAL. Do 
> you know why? If this entry is set to VIRTUAL, then each refKind has two 
> entries in the table corresponding to the correct result for valueOf.

this is the table that is being generated right now:

Table has 20 entries
0: null
1: null
2: Kind: [name: GETTER, refKind: 1, isInterface: false]
3: Kind: [name: GETTER, refKind: 1, isInterface: false]
4: Kind: [name: STATIC_GETTER, refKind: 2, isInterface: false]
5: Kind: [name: STATIC_GETTER, refKind: 2, isInterface: false]
6: Kind: [name: SETTER, refKind: 3, isInterface: false]
7: Kind: [name: SETTER, refKind: 3, isInterface: false]
8: Kind: [name: STATIC_SETTER, refKind: 4, isInterface: false]
9: Kind: [name: STATIC_SETTER, refKind: 4, isInterface: false]
10: Kind: [name: VIRTUAL, refKind: 5, isInterface: false]
11: Kind: [name: INTERFACE_VIRTUAL, refKind: 9, isInterface: true]
12: Kind: [name: STATIC, refKind: 6, isInterface: false]
13: Kind: [name: INTERFACE_STATIC, refKind: 6, isInterface: true]
14: Kind: [name: SPECIAL, refKind: 7, isInterface: false]
15: Kind: [name: INTERFACE_SPECIAL, refKind: 7, isInterface: true]
16: Kind: [name: CONSTRUCTOR, refKind: 8, isInterface: false]
17: Kind: [name: CONSTRUCTOR, refKind: 8, isInterface: false]
18: Kind: [name: INTERFACE_VIRTUAL, refKind: 9, isInterface: true]
19: Kind: [name: INTERFACE_VIRTUAL, refKind: 9, isInterface: true]

-

PR: https://git.openjdk.java.net/jdk17/pull/29


Re: [jdk17] RFR: 8268342: java/foreign/channels/TestAsyncSocketChannels.java fails with "IllegalStateException: This segment is already closed"

2021-06-11 Thread Daniel Fuchs
On Fri, 11 Jun 2021 15:26:50 GMT, Chris Hegarty  wrote:

> There is the possibility for a race in the test, where the outbound socket 
> buffer is still being filled, after 5 seconds, when the main test thread 
> tries to close the resource scope.
> 
> The test has been updated to set the send/receive buffer sizes in an 
> attempt/hint to limit the accepted/connected socket buffer sizes. Actual 
> buffer sizes in use will likely be larger due to TCP auto-tuning, but the 
> hint typically reduces the overall scaled sizes. This is primarily to 
> stabilize outstanding write operations. Additionally, to ameliorate the 
> wait-for-writebuf-to-fill situation, the dumb sleep 5secs has been replaced 
> with a heuristic that checks that the number of bytes written quiesces. With 
> these changes, the test successfully passes several thousands of runs.

Looks good but you could make the logic simpler by calling latch.countDown() in 
the last call to completed(), instead of doing it in the first one.

test/jdk/java/foreign/channels/TestAsyncSocketChannels.java line 283:

> 281: readNBytes(asc2, bytesWritten.get());
> 282: assertTrue(scope.isAlive());
> 283: awaitOutstandingWrites(outstandingWriteOps);

An alternative would be to delegate the call to latch.countDown() to the 
TestHandler subclass, and call it at the end of completed() only when 
outstandingWriteOps.decrementAndGet() == 0; As it stands the latch is released 
with the first call to `completed` (instead of being released with the last 
one) - and that's what makes this method necessary.

-

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk17/pull/30


[jdk17] RFR: 8268342: java/foreign/channels/TestAsyncSocketChannels.java fails with "IllegalStateException: This segment is already closed"

2021-06-11 Thread Chris Hegarty
There is the possibility for a race in the test, where the outbound socket 
buffer is still being filled, after 5 seconds, when the main test thread tries 
to close the resource scope.

The test has been updated to set the send/receive buffer sizes in an 
attempt/hint to limit the accepted/connected socket buffer sizes. Actual buffer 
sizes in use will likely be larger due to TCP auto-tuning, but the hint 
typically reduces the overall scaled sizes. This is primarily to stabilize 
outstanding write operations. Additionally, to ameliorate the 
wait-for-writebuf-to-fill situation, the dumb sleep 5secs has been replaced 
with a heuristic that checks that the number of bytes written quiesces. With 
these changes, the test successfully passes several thousands of runs.

-

Commit messages:
 - initial changes

Changes: https://git.openjdk.java.net/jdk17/pull/30/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=30=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8268342
  Stats: 52 lines in 1 file changed: 46 ins; 4 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/30.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/30/head:pull/30

PR: https://git.openjdk.java.net/jdk17/pull/30


Re: [jdk17] RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling

2021-06-11 Thread Vicente Romero
On Fri, 11 Jun 2021 15:01:20 GMT, Vicente Romero  wrote:

> This PR is a copy of [PR-4416](https://github.com/openjdk/jdk/pull/4416) 
> which was intended to openjdk/jdk.
> 
> Please review this PR which is syncing the implementation of 
> `DirectMethodHandleDesc.Kind.valueOf(int)` and 
> `DirectMethodHandleDesc.Kind.valueOf(int, boolean)` with its spec. My reading 
> of the method's spec is that if the value of the refKind parameter is: 
> MethodHandleInfo.REF_invokeInterface, then 
> DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a 
> value of true for its second argument, currently it is invoked with false 
> regardless of the value of the refKind parameter
> 
> TIA

src/java.base/share/classes/java/lang/constant/DirectMethodHandleDesc.java line 
143:

> 141: }
> 142: if (kind.refKind == refKind &&
> 143: (refKind != REF_invokeStatic || refKind != 
> REF_invokeSpecial || kind.isInterface == isInterface)){

@mlchung 13 hours ago Member

It reads to me that the static initializer tries to set up the table such that 
valueOf(refKind, isInterface) should find the proper kind to return except this:

// There is not a specific Kind for interfaces
if (kind == VIRTUAL) kind = INTERFACE_VIRTUAL;

This changes the entry for REF_invokeVirtual to kind INTERFACE_VIRTUAL. Do you 
know why? If this entry is set to VIRTUAL, then each refKind has two entries in 
the table corresponding to the correct result for valueOf.

test/jdk/java/lang/constant/MethodHandleDescTest.java line 364:

> 362: public void testKind() {
> 363: for (Kind k : Kind.values()) {
> 364: assertEquals(Kind.valueOf(k.refKind), 
> Kind.valueOf(k.refKind, k.refKind == MethodHandleInfo.REF_invokeInterface));

@mlchung mlchung 2 days ago Member

Looks like the test does not verify the cases specified by valueOf(int refKind, 
boolean isInterface).
i.e. For most values of refKind, there is an exact match regardless of the 
value of isInterface except REF_invokeStatic and REF_invokeSpecial.

Do you mind adding those cases?

@vicente-romero-oracle vicente-romero-oracle 22 hours ago •

hum, the implementation for valueOf(int refKind, boolean isInterface) is 
incorrect, the behavior does depends on the value of isInterface for example: 
Kind.valueOf(1, false) returns GETTER while Kind.valueOf(1, true) fails with 
java.lang.IllegalArgumentException will fix the implementation of valueOf(int 
refKind, boolean isInterface) for it to match its spec

-

PR: https://git.openjdk.java.net/jdk17/pull/29


Re: RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling [v2]

2021-06-11 Thread Vicente Romero
On Thu, 10 Jun 2021 23:26:21 GMT, Vicente Romero  wrote:

>> Please review this PR which is just syncing the implementation of 
>> DirectMethodHandleDesc.Kind.valueOf(int) with its spec. My reading of the 
>> method's spec is that if the value of the `refKind` parameter is: 
>> MethodHandleInfo.REF_invokeInterface, then 
>> DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a 
>> value of `true` for its second argument, currently it is invoked with 
>> `false` regardless of the value of the `refKind` parameter
>> 
>> TIA
>
> Vicente Romero has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   addressing review changes

I will be withdrawing this one to open a new 
[PR-29](https://github.com/openjdk/jdk17/pull/29) intended to jdk17, please 
let's follow the discussion there

-

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


Withdrawn: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling

2021-06-11 Thread Vicente Romero
On Tue, 8 Jun 2021 16:46:36 GMT, Vicente Romero  wrote:

> Please review this PR which is just syncing the implementation of 
> DirectMethodHandleDesc.Kind.valueOf(int) with its spec. My reading of the 
> method's spec is that if the value of the `refKind` parameter is: 
> MethodHandleInfo.REF_invokeInterface, then 
> DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a 
> value of `true` for its second argument, currently it is invoked with `false` 
> regardless of the value of the `refKind` parameter
> 
> TIA

This pull request has been closed without being integrated.

-

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


[jdk17] RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling

2021-06-11 Thread Vicente Romero
This PR is a copy of [PR-4416](https://github.com/openjdk/jdk/pull/4416) which 
was intended to openjdk/jdk.

Please review this PR which is syncing the implementation of 
`DirectMethodHandleDesc.Kind.valueOf(int)` and 
`DirectMethodHandleDesc.Kind.valueOf(int, boolean)` with its spec. My reading 
of the method's spec is that if the value of the refKind parameter is: 
MethodHandleInfo.REF_invokeInterface, then 
DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a value 
of true for its second argument, currently it is invoked with false regardless 
of the value of the refKind parameter

TIA

-

Commit messages:
 - 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) 
implementation doesn't conform to the spec regarding REF_invokeInterface 
handling

Changes: https://git.openjdk.java.net/jdk17/pull/29/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=29=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8267421
  Stats: 21 lines in 2 files changed: 17 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/29.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/29/head:pull/29

PR: https://git.openjdk.java.net/jdk17/pull/29


RFR: 8268457: XML Transformer outputs Unicode supplementary character incorrectly to HTML

2021-06-11 Thread Masanori Yano
Hi all,

Could you please review the 8268457 bug fixes?

The problem is that ToHTMLStream applies processing for non-surrogate pairs to 
the surrogate pair.
This fix changes the processing for non-surrogate pairs to the else condition.

-

Commit messages:
 - clean up whitespace
 - 8268457: XML Transformer outputs Unicode supplementary character incorrectly 
to HTML

Changes: https://git.openjdk.java.net/jdk/pull/4474/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4474=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8268457
  Stats: 235 lines in 7 files changed: 223 ins; 9 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4474.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4474/head:pull/4474

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


Re: RFR: 8265909 : build.tools.dtdbuilder.DTDBuilder.java failed detecting missing path of dtd_home [v2]

2021-06-11 Thread ScientificWare
On Wed, 9 Jun 2021 14:13:06 GMT, Erik Joelsson  wrote:

>> ScientificWare has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Changes to keep home_dtd null check.
>>   
>>   As suggested in conversation moving `+ File.separator` is more elegant 
>> than changing the `null` check.
>
> Marked as reviewed by erikj (Reviewer).

@erikj79  Thanks.

-

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


Integrated: 8265909 : build.tools.dtdbuilder.DTDBuilder.java failed detecting missing path of dtd_home

2021-06-11 Thread ScientificWare
On Thu, 22 Apr 2021 13:47:03 GMT, ScientificWare 
 wrote:

> This concerns [dtdbuilder 
> tools](https://github.com/openjdk/jdk/tree/master/make/jdk/src/classes/build/tools/dtdbuilder).
> 
> In jshell, try `System.getProperty("html32") + ""` you'll get a `String`.
> 
> So, in `DTDBuilder.java` when none `dtd_home` property is set, the `dtd_home` 
> string value is not `null`, causing an exception with the present test.
> 
> The expected value is `"Must set property 'dtd_home'"`
> 
> And in this case, should'nt we have a `System.exit(1)` too rather than a 
> simple `return` ?

This pull request has now been integrated.

Changeset: 49112fa5
Author:ScientificWare 
Committer: Erik Joelsson 
URL:   
https://git.openjdk.java.net/jdk/commit/49112fa5752174a77fb5cd276fdd4240bf76bf82
Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod

8265909: build.tools.dtdbuilder.DTDBuilder.java failed detecting missing path 
of dtd_home

Reviewed-by: erikj

-

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


Re: RFR: 8266310: deadlock while loading the JNI code [v6]

2021-06-11 Thread Aleksei Voitylov
On Mon, 31 May 2021 23:57:09 GMT, Mandy Chung  wrote:

>> Aleksei Voitylov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   address review comments
>
> test/jdk/java/lang/ClassLoader/loadLibraryDeadlock/TestLoadLibraryDeadlock.java
>  line 165:
> 
>> 163: 
>> 164: runCommandInTestClassPath("rm", "-f", "*.jar")
>> 165: .shouldHaveExitValue(0);
> 
> You can use `jdk.test.lib.util.FileUtils` to delete a directory or a given 
> path.

Fixed.

> test/jdk/java/lang/ClassLoader/loadLibraryUnload/LoadLibraryUnload.java line 
> 83:
> 
>> 81: this.object = fromClass.newInstance();
>> 82: this.method = fromClass.getDeclaredMethod("loadLibrary");
>> 83: } catch (Exception error) {
> 
> Nit: `s/Exception/ReflectiveOperationException/` as ROE is the specific 
> checked exception you want to catch here.

Fixed.

> test/jdk/java/lang/ClassLoader/loadLibraryUnload/LoadLibraryUnload.java line 
> 84:
> 
>> 82: this.method = fromClass.getDeclaredMethod("loadLibrary");
>> 83: } catch (Exception error) {
>> 84: throw new Error(error);
> 
> Error is fine.  Most tests throw `RuntimeException` that can be another 
> choice.

Yes, used RuntimeException.

> test/jdk/java/lang/ClassLoader/loadLibraryUnload/LoadLibraryUnload.java line 
> 92:
> 
>> 90: try {
>> 91: method.invoke(object);
>> 92: } catch (Exception error) {
> 
> Same here to catch the `ReflectiveOperationException`.

Same here.

-

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


Re: RFR: 8266310: deadlock while loading the JNI code [v7]

2021-06-11 Thread Aleksei Voitylov
On Fri, 11 Jun 2021 10:03:44 GMT, Aleksei Voitylov  
wrote:

>> Please review this PR which fixes the deadlock in ClassLoader between the 
>> two lock objects - a lock object associated with the class being loaded, and 
>> the ClassLoader.loadedLibraryNames hash map, locked during the native 
>> library load operation.
>> 
>> Problem being fixed:
>> 
>> The initial reproducer demonstrated a deadlock between the JarFile/ZipFile 
>> and the hash map. That deadlock exists even when the ZipFile/JarFile lock is 
>> removed because there's another lock object in the class loader, associated 
>> with the name of the class being loaded. Such objects are stored in 
>> ClassLoader.parallelLockMap. The deadlock occurs when JNI_OnLoad() loads 
>> exactly the same class, whose signature is being verified in another thread.
>> 
>> Proposed fix:
>> 
>> The proposed patch suggests to get rid of locking loadedLibraryNames hash 
>> map and synchronize on each entry name, as it's done with class names in see 
>> ClassLoader.getClassLoadingLock(name) method.
>> 
>> The patch introduces nativeLibraryLockMap which holds the lock objects for 
>> each library name, and the getNativeLibraryLock() private method is used to 
>> lazily initialize the corresponding lock object. nativeLibraryContext was 
>> changed to ThreadLocal, so that in any concurrent thread it would have a 
>> NativeLibrary object on top of the stack, that's being currently 
>> loaded/unloaded in that thread. nativeLibraryLockMap accumulates the names 
>> of all native libraries loaded - in line with class loading code, it is not 
>> explicitly cleared.
>> 
>> Testing:  jtreg and jck testing with no regressions. A new regression test 
>> was developed.
>
> Aleksei Voitylov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   address review comments

Mandy, thank you for review! I used all of your suggestions except for two 
places. Please see a responses in the conversation.

-

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


Re: RFR: 8266310: deadlock while loading the JNI code [v7]

2021-06-11 Thread Aleksei Voitylov
> Please review this PR which fixes the deadlock in ClassLoader between the two 
> lock objects - a lock object associated with the class being loaded, and the 
> ClassLoader.loadedLibraryNames hash map, locked during the native library 
> load operation.
> 
> Problem being fixed:
> 
> The initial reproducer demonstrated a deadlock between the JarFile/ZipFile 
> and the hash map. That deadlock exists even when the ZipFile/JarFile lock is 
> removed because there's another lock object in the class loader, associated 
> with the name of the class being loaded. Such objects are stored in 
> ClassLoader.parallelLockMap. The deadlock occurs when JNI_OnLoad() loads 
> exactly the same class, whose signature is being verified in another thread.
> 
> Proposed fix:
> 
> The proposed patch suggests to get rid of locking loadedLibraryNames hash map 
> and synchronize on each entry name, as it's done with class names in see 
> ClassLoader.getClassLoadingLock(name) method.
> 
> The patch introduces nativeLibraryLockMap which holds the lock objects for 
> each library name, and the getNativeLibraryLock() private method is used to 
> lazily initialize the corresponding lock object. nativeLibraryContext was 
> changed to ThreadLocal, so that in any concurrent thread it would have a 
> NativeLibrary object on top of the stack, that's being currently 
> loaded/unloaded in that thread. nativeLibraryLockMap accumulates the names of 
> all native libraries loaded - in line with class loading code, it is not 
> explicitly cleared.
> 
> Testing:  jtreg and jck testing with no regressions. A new regression test 
> was developed.

Aleksei Voitylov has updated the pull request incrementally with one additional 
commit since the last revision:

  address review comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3976/files
  - new: https://git.openjdk.java.net/jdk/pull/3976/files/85005d57..f8423b97

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3976=06
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3976=05-06

  Stats: 47 lines in 5 files changed: 19 ins; 6 del; 22 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3976.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3976/head:pull/3976

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


Re: RFR: 8266310: deadlock while loading the JNI code [v6]

2021-06-11 Thread Aleksei Voitylov
On Mon, 31 May 2021 20:56:14 GMT, Mandy Chung  wrote:

>> Aleksei Voitylov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   address review comments
>
> src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java line 207:
> 
>> 205:  *
>> 206:  * We use a static stack to hold the list of libraries we 
>> are
>> 207:  * loading, so that each thread maintains its own stack.
> 
> line 206: no longer a static stack.  line 206-207 can be replaced with:
> 
> 
>  * Each thread maintains its own stack to hold the list of 
> libraries it is loading.

Done.

> src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java line 213:
> 
>> 211:  * a different class loader, we raise UnsatisfiedLinkError.
>> 212:  */
>> 213: for (NativeLibraryImpl lib : NativeLibraryContext.get()) {
> 
> I suggest to rename the `get` method of `NativeLibraryContext` to `current` 
> that returns the current thread's context.

Done.

> test/jdk/java/lang/ClassLoader/loadLibraryDeadlock/LoadLibraryDeadlock.java 
> line 49:
> 
>> 47:  InstantiationException |
>> 48:  IllegalAccessException ignore) {
>> 49: System.out.println("Class Class1 not found.");
> 
> In general a test should let the exception to propagate.   In this case, the 
> threads (both t1 and t2) can warp the exception and throw `RuntimeException` 
> as it's an error.

Fixed.

> test/jdk/java/lang/ClassLoader/loadLibraryDeadlock/LoadLibraryDeadlock.java 
> line 60:
> 
>> 58: System.out.println("Signed jar loaded.");
>> 59: } catch (ClassNotFoundException ignore) {
>> 60: System.out.println("Class Class2 not found.");
> 
> Same as above

Fixed.

> test/jdk/java/lang/ClassLoader/loadLibraryDeadlock/TestLoadLibraryDeadlock.java
>  line 69:
> 
>> 67: 
>> 68: private static OutputAnalyzer genKey() throws Throwable {
>> 69: runCommandInTestClassPath("rm", "-f", KEYSTORE);
> 
> Can you use `jdk.test.lib.util.FileUtils` instead of exec `rm -f`.

Done.

> test/jdk/java/lang/ClassLoader/loadLibraryDeadlock/TestLoadLibraryDeadlock.java
>  line 84:
> 
>> 82: 
>> 83: private static OutputAnalyzer createJar(String outputJar, String... 
>> classes) throws Throwable {
>> 84: String jar = JDKToolFinder.getJDKTool("jar");
> 
> You can consider using `jdk.test.lib.util.JarUtils` to reduce the test 
> execution time so that it can create the jar without exec'ing another process.

I tried, but this doesn't work that well unfortunately with 
JarUtils.createJar() as it only produces a valid jar-file capable of triggering 
the issue when the compiled class files (jtreg @build output) are in the same 
directory as the current directory of the process calling JarUtils.createJar(). 
Jtreg @build outputs the compiled classes to a separate directory. Creation of 
a new jar process allows to change the current directory so that the relative 
paths to the class files are such that it would form the valid output jar-file. 
Alternatively we could copy the class files to the current directory prior to 
creating the jar-file, but that would introduce an extra step. Would you mind 
me keeping the createJar() in the test?

> test/jdk/java/lang/ClassLoader/loadLibraryDeadlock/TestLoadLibraryDeadlock.java
>  line 141:
> 
>> 139: try {
>> 140: return future.get(1000, TimeUnit.MILLISECONDS);
>> 141: } catch (Exception ignoreAll) {
> 
> if this is an unexpected error case, it should throw an exception.

If this threw an exception, it wouldn't be possible to collect all available 
data from the process which is not yet completed. Throwing an exception would 
defeat the purpose of readAvailable() method.

> test/jdk/java/lang/ClassLoader/loadLibraryUnload/LoadLibraryUnload.java line 
> 31:
> 
>> 29:  * @test
>> 30:  * @bug 8266310
>> 31:  * @summary deadlock while loading the JNI code
> 
> please update `@summary` with a description of the test (rather than the 
> synposis of the bug).

Updated.

> test/jdk/java/lang/ClassLoader/loadLibraryUnload/LoadLibraryUnloadTest.java 
> line 32:
> 
>> 30:  * @test
>> 31:  * @bug 8266310
>> 32:  * @summary deadlock while loading the JNI code
> 
> Please update `@summary` with a description of the test (rather than the 
> synposis of the bug)

Updated.

-

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


Re: RFR: 8265909 : build.tools.dtdbuilder.DTDBuilder.java failed detecting missing path of dtd_home [v2]

2021-06-11 Thread ScientificWare
On Wed, 9 Jun 2021 14:13:06 GMT, Erik Joelsson  wrote:

>> ScientificWare has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Changes to keep home_dtd null check.
>>   
>>   As suggested in conversation moving `+ File.separator` is more elegant 
>> than changing the `null` check.
>
> Marked as reviewed by erikj (Reviewer).

Hi @erikj79 ,
Could you sponsor this change ?
Best regards.

-

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


Re: Windows aarch64 (build debug) constantly fails

2021-06-11 Thread David Holmes

On 11/06/2021 4:27 pm, Jaikiran Pai wrote:

Hello David,

On 11/06/21 11:41 am, David Holmes wrote:

Hi,

On 10/06/2021 8:58 pm, Сергей Цыпанов wrote:
Hello, in the pipeline of my JDK fork the mentioned build step 
constantly fails even providing I modify only Java code


I see this message in the log

Compiling 2 files for BUILD_JVMTI_TOOLS
LINK : fatal error LNK1104: cannot open file 'libcpmt.lib'


Yes this has been failing in Github actions for some time now. It 
appears to be a misconfigured Visual Studio installation. However 
there seems to be no way to report a problem in this regard so we seem 
to be stuck unless someone knows how to bring such issues to the 
attention of the Github support team ?


Unrelated to JDK project, last time when I had run into a Github actions 
issue, I had reported it as a issue at 
https://github.com/actions/virtual-environments and they were pretty 
responsive and helpful about it.


Thanks Jaikiran I'll give that a go.

David


-Jaikiran




Re: Windows aarch64 (build debug) constantly fails

2021-06-11 Thread Jaikiran Pai

Hello David,

On 11/06/21 11:41 am, David Holmes wrote:

Hi,

On 10/06/2021 8:58 pm, Сергей Цыпанов wrote:
Hello, in the pipeline of my JDK fork the mentioned build step 
constantly fails even providing I modify only Java code


I see this message in the log

Compiling 2 files for BUILD_JVMTI_TOOLS
LINK : fatal error LNK1104: cannot open file 'libcpmt.lib'


Yes this has been failing in Github actions for some time now. It 
appears to be a misconfigured Visual Studio installation. However 
there seems to be no way to report a problem in this regard so we seem 
to be stuck unless someone knows how to bring such issues to the 
attention of the Github support team ?


Unrelated to JDK project, last time when I had run into a Github actions 
issue, I had reported it as a issue at 
https://github.com/actions/virtual-environments and they were pretty 
responsive and helpful about it.


-Jaikiran




Re: Windows aarch64 (build debug) constantly fails

2021-06-11 Thread David Holmes

Hi,

On 10/06/2021 8:58 pm, Сергей Цыпанов wrote:

Hello, in the pipeline of my JDK fork the mentioned build step constantly fails 
even providing I modify only Java code

I see this message in the log

Compiling 2 files for BUILD_JVMTI_TOOLS
LINK : fatal error LNK1104: cannot open file 'libcpmt.lib'


Yes this has been failing in Github actions for some time now. It 
appears to be a misconfigured Visual Studio installation. However there 
seems to be no way to report a problem in this regard so we seem to be 
stuck unless someone knows how to bring such issues to the attention of 
the Github support team ?


Cheers,
David
-


make[3]: *** [gensrc/GensrcAdlc.gmk:63: 
/cygdrive/d/a/jdk/jdk/jdk/build/windows-aarch64/hotspot/variant-server/tools/adlc/adlc.exe]
 Error 1
make[3]: *** Waiting for unfinished jobs
make[2]: *** [make/Main.gmk:248: hotspot-server-gensrc] Error 2

ERROR: Build failed for target 'default (hotspot)' in configuration 
'windows-aarch64' (exit code 2)

=== Output from failing command(s) repeated here ===
* For target hotspot_variant-server_tools_adlc_objs_BUILD_ADLC_link:
LINK : fatal error LNK1104: cannot open file 'libcpmt.lib'

* All command lines available in 
/cygdrive/d/a/jdk/jdk/jdk/build/windows-aarch64/make-support/failure-logs.
=== End of repeated output ===

No indication of failed target found.
Hint: Try searching the build log for '] Error'.
Hint: See doc/building.html#troubleshooting for assistance.

make[1]: *** [/cygdrive/d/a/jdk/jdk/jdk/make/Init.gmk:315: main] Error 2
make: *** [/cygdrive/d/a/jdk/jdk/jdk/make/Init.gmk:186: default] Error 2
Error: Process completed with exit code 1.

Here's the link to particular run

https://github.com/stsypanov/jdk/runs/2792324849

What can I do with this?

Regards,
Sergey Tsypanov