Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v13]
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
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
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
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]
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]
> 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]
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]
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"
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
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"
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"
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
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]
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
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
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
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]
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
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]
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]
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]
> 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]
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]
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
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
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
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
