Re: RFR: 8311630: [s390] Implementation of Foreign Function & Memory API (Preview) [v11]
> Implementation of "Foreign Function & Memory API" for s390x (Big Endian). sid8606 has updated the pull request incrementally with one additional commit since the last revision: Fix indentation Signed-off-by: Sidraya - Changes: - all: https://git.openjdk.org/jdk/pull/14801/files - new: https://git.openjdk.org/jdk/pull/14801/files/924b45bc..a28c92d6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14801=10 - incr: https://webrevs.openjdk.org/?repo=jdk=14801=09-10 Stats: 16 lines in 3 files changed: 0 ins; 4 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/14801.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14801/head:pull/14801 PR: https://git.openjdk.org/jdk/pull/14801
Re: RFR: 8311630: [s390] Implementation of Foreign Function & Memory API (Preview) [v10]
On Sun, 6 Aug 2023 09:32:08 GMT, Andrey Turbanov wrote:
>> sid8606 has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix typo
>>
>> Signed-off-by: Sidraya
>
> src/java.base/share/classes/jdk/internal/foreign/abi/s390/linux/LinuxS390CallArranger.java
> line 112:
>
>> 110: }
>> 111:
>> 112: public static MethodHandle arrangeDowncall(MethodType mt,
>> FunctionDescriptor cDesc, LinkerOptions options) {
>
> Suggestion:
>
> public static MethodHandle arrangeDowncall(MethodType mt,
> FunctionDescriptor cDesc, LinkerOptions options) {
Thank you @turbanoff for the review comment. I have Fixed this in new commit.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1286613222
Re: RFR: 8311630: [s390] Implementation of Foreign Function & Memory API (Preview) [v10]
On Mon, 7 Aug 2023 13:37:57 GMT, Martin Doerr wrote:
>> sid8606 has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix typo
>>
>> Signed-off-by: Sidraya
>
> src/hotspot/cpu/s390/foreignGlobals_s390.cpp line 154:
>
>> 152: } else {
>> 153: assert(to_reg.stack_size() == 4, "size should match");
>> 154: // s390 needs 4 Byte offset
>
> Seems like this comment should get removed.
Fixed
> src/java.base/share/classes/jdk/internal/foreign/abi/s390/linux/LinuxS390CallArranger.java
> line 221:
>
>> 219: Class type =
>> SharedUtils.primitiveCarrierForSize(layout.byteSize(), false);
>> 220: bindings.bufferLoad(0, type)
>> 221: .vmStore(storage, type);
>
> Maybe improve indentation?
Thank you @TheRealMDoerr. Fixed in new commit.
> src/java.base/share/classes/jdk/internal/foreign/abi/s390/linux/TypeClass.java
> line 44:
>
>> 42: FLOAT;
>> 43:
>> 44: private static final int MAX_AGGREGATE_REGS_SIZE = 1;
>
> Unused.
Fixed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1286613915
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1286613616
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1286613721
Re: RFR: 8313875: Use literals instead of static fields in java.util.Math: twoToTheDoubleScaleUp, twoToTheDoubleScaleDown [v2]
On Mon, 7 Aug 2023 19:44:48 GMT, Andrey Turbanov wrote: >> Couple of static fields in Math are used only once and can be replaced with >> literals `0x1p512`/`0x1p-512 ` > > Andrey Turbanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8313875: Make fields final in java.util.Math: twoToTheDoubleScaleUp, > twoToTheDoubleScaleDown > > completely remove the fields. Use literals instead. Marked as reviewed by bpb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14875#pullrequestreview-1566050946
Re: RFR: 8313875: Use literals instead of static fields final in java.util.Math: twoToTheDoubleScaleUp, twoToTheDoubleScaleDown [v2]
> Couple of static fields in Math are used only once and can be replaced with > literals `0x1p512`/`0x1p-512 ` Andrey Turbanov has updated the pull request incrementally with one additional commit since the last revision: 8313875: Make fields final in java.util.Math: twoToTheDoubleScaleUp, twoToTheDoubleScaleDown completely remove the fields. Use literals instead. - Changes: - all: https://git.openjdk.org/jdk/pull/14875/files - new: https://git.openjdk.org/jdk/pull/14875/files/759180e3..4e154831 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14875=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14875=00-01 Stats: 6 lines in 1 file changed: 0 ins; 4 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14875.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14875/head:pull/14875 PR: https://git.openjdk.org/jdk/pull/14875
Re: RFR: 8313875: Make fields final in java.util.Math: twoToTheDoubleScaleUp, twoToTheDoubleScaleDown
On Mon, 7 Aug 2023 17:35:34 GMT, Andrey Turbanov wrote:
>> src/java.base/share/classes/java/lang/Math.java line 3425:
>>
>>> 3423: // Constants used in scalb
>>> 3424: private static final double twoToTheDoubleScaleUp =
>>> powerOfTwoD(512);
>>> 3425: private static final double twoToTheDoubleScaleDown =
>>> powerOfTwoD(-512);
>>
>> Aren't these the _literals_ `0x1p512` and `0x1p-512`, respectively?
>
> Whoa. You are right.
>
> public static void main(String[] args) {
> System.out.println(twoToTheDoubleScaleUp);
> System.out.println(twoToTheDoubleScaleDown);
> System.out.println(0x1p512);
> System.out.println(0x1p-512);
>
> System.out.println(twoToTheDoubleScaleUp == 0x1p512);
> System.out.println(twoToTheDoubleScaleDown == 0x1p-512);
> }
>
>
>
> 1.3407807929942597E154
> 7.458340731200207E-155
> 1.3407807929942597E154
> 7.458340731200207E-155
> true
> true
These static fields seem to be used just once each, so we might as well just
replace the usages with the literals and get rid of the fields.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14875#discussion_r1286221538
Re: RFR: 8313875: Make fields final in java.util.Math: twoToTheDoubleScaleUp, twoToTheDoubleScaleDown
On Mon, 7 Aug 2023 16:43:49 GMT, Raffaello Giulietti
wrote:
>> Couple of static fields in Math could be declared `final`.
>
> src/java.base/share/classes/java/lang/Math.java line 3425:
>
>> 3423: // Constants used in scalb
>> 3424: private static final double twoToTheDoubleScaleUp =
>> powerOfTwoD(512);
>> 3425: private static final double twoToTheDoubleScaleDown =
>> powerOfTwoD(-512);
>
> Aren't these the _literals_ `0x1p512` and `0x1p-512`, respectively?
Whoa. You are right.
public static void main(String[] args) {
System.out.println(twoToTheDoubleScaleUp);
System.out.println(twoToTheDoubleScaleDown);
System.out.println(0x1p512);
System.out.println(0x1p-512);
System.out.println(twoToTheDoubleScaleUp == 0x1p512);
System.out.println(twoToTheDoubleScaleDown == 0x1p-512);
}
1.3407807929942597E154
7.458340731200207E-155
1.3407807929942597E154
7.458340731200207E-155
true
true
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14875#discussion_r1286196598
Integrated: 8313702: Update IANA Language Subtag Registry to Version 2023-08-02
On Thu, 3 Aug 2023 16:31:01 GMT, Justin Lu wrote: > Please review this PR which updates the IANA data from version 05-11-2023 to > version > [08-02-2023](https://mm.icann.org/pipermail/ietf-languages-announcements/2023-August/88.html). > > The contents of the update added variant subtag “blasl” to the language > subtag registry. This pull request has now been integrated. Changeset: 1da82a34 Author:Justin Lu URL: https://git.openjdk.org/jdk/commit/1da82a34b14189814e45a93c68620ccb51427111 Stats: 12 lines in 2 files changed: 9 ins; 0 del; 3 mod 8313702: Update IANA Language Subtag Registry to Version 2023-08-02 Reviewed-by: naoto, iris - PR: https://git.openjdk.org/jdk/pull/15142
Re: RFR: 8313875: Make fields final in java.util.Math: twoToTheDoubleScaleUp, twoToTheDoubleScaleDown
On Thu, 13 Jul 2023 17:57:16 GMT, Andrey Turbanov wrote: > Couple of static fields in Math could be declared `final`. src/java.base/share/classes/java/lang/Math.java line 3425: > 3423: // Constants used in scalb > 3424: private static final double twoToTheDoubleScaleUp = > powerOfTwoD(512); > 3425: private static final double twoToTheDoubleScaleDown = > powerOfTwoD(-512); Aren't these the _literals_ `0x1p512` and `0x1p-512`, respectively? - PR Review Comment: https://git.openjdk.org/jdk/pull/14875#discussion_r1286144980
Re: RFR: 8313875: Make fields final in java.util.Math: twoToTheDoubleScaleUp, twoToTheDoubleScaleDown
On Thu, 13 Jul 2023 17:57:16 GMT, Andrey Turbanov wrote: > Couple of static fields in Math could be declared `final`. +1 - Marked as reviewed by bpb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14875#pullrequestreview-1565684801
RFR: 8313889: Fix -Wconversion warnings in foreign benchmarks
Fix these -Wconversion warnings in the foreign benchmarks:
./test/micro/org/openjdk/bench/java/lang/foreign/libQSortJNI.c: In function
‘Java_org_openjdk_bench_java_lang_foreign_QSort_jni_1qsort_1optimized’:
./test/micro/org/openjdk/bench/java/lang/foreign/libQSortJNI.c:59:17: warning:
conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘jsize’ {aka ‘int’} may
change the sign of the result [-Wsign-conversion]
59 | qsort(ints, length, sizeof(jint), );
| ^~
./test/micro/org/openjdk/bench/java/lang/foreign/libQSortJNI.c: In function
‘Java_org_openjdk_bench_java_lang_foreign_QSort_jni_1qsort_1naive’:
./test/micro/org/openjdk/bench/java/lang/foreign/libQSortJNI.c:87:17: warning:
conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘jsize’ {aka ‘int’} may
change the sign of the result [-Wsign-conversion]
87 | qsort(carr, length, sizeof(jint), java_cmp);
|
In this case the issue is that we're converting from a signed type to an
unsigned type. So, if the source value is negative, it will be positive after
conversion.
Since the source value is a Java array length in both cases, the value can not
be negative, so we simply fix this by casting to `size_t` explicitly.
-
Commit messages:
- fix -Wconversion warnings in foreign benchmarks
Changes: https://git.openjdk.org/jdk/pull/15179/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=15179=00
Issue: https://bugs.openjdk.org/browse/JDK-8313889
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/15179.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/15179/head:pull/15179
PR: https://git.openjdk.org/jdk/pull/15179
Re: RFR: 8313875: Make fields final in java.util.Math: twoToTheDoubleScaleUp, twoToTheDoubleScaleDown
On Thu, 13 Jul 2023 17:57:16 GMT, Andrey Turbanov wrote: > Couple of static fields in Math could be declared `final`. Marked as reviewed by darcy (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14875#pullrequestreview-1565476822
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present
On Thu, 6 Jul 2023 17:34:10 GMT, Severin Gehwolf wrote: > Please review this patch which adds a "jmodless" jlink mode to the JDK. > Fundamentally this patch adds an option to use `jlink` even though your JDK > install might not come with the packaged modules (directory `jmods`). This is > particularly useful to further reduce the size of a jlinked runtime. After > the removal of the concept of a JRE, a common distribution mechanism is still > the full JDK with all modules and packaged modules. However, packaged modules > can incur an additional size tax. For example in a container scenario it > could be useful to have a base JDK container including all modules, but > without also delivering the packaged modules. This comes at a size advantage > of `~25%`. Such a base JDK container could then be used to `jlink` > application specific runtimes, further reducing the size of the application > runtime image (App + JDK runtime; as a single image *or* separate bundles, > depending on the app being modularized). > > The basic design of this approach is to add a jlink plugin for tracking > non-class and non-resource files of a JDK install. I.e. files which aren't > present in the jimage (`lib/modules`). This enables producing a > `JmodLessArchive` class which has all the info of what constitutes the final > jlinked runtime. > > Basic usage example: > > $ diff -u <(./bin/java --list-modules --limit-modules java.se) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules java.se) > $ diff -u <(./bin/java --list-modules --limit-modules java.se) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules jdk.jlink) > $ ls ../linux-x86_64-server-release/images/jdk/jmods > java.base.jmodjava.net.http.jmod java.sql.rowset.jmod > jdk.crypto.ec.jmod jdk.internal.opt.jmod > jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod > java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod > jdk.dynalink.jmod jdk.internal.vm.ci.jmod > jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod > java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod > jdk.editpad.jmod jdk.internal.vm.compiler.jmod > jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod > java.desktop.jmod java.scripting.jmod java.xml.jmod > jdk.hotspot.agent.jmod jdk.internal.vm.compiler.management.jmod > jdk.jlink.jmod jdk.naming.dns.jmodjdk.unsupported... Please keep open, bot. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-1667923083
Re: RFR: 8311630: [s390] Implementation of Foreign Function & Memory API (Preview) [v10]
On Tue, 1 Aug 2023 06:29:28 GMT, sid8606 wrote: >> Implementation of "Foreign Function & Memory API" for s390x (Big Endian). > > sid8606 has updated the pull request incrementally with one additional commit > since the last revision: > > Fix typo > > Signed-off-by: Sidraya src/java.base/share/classes/jdk/internal/foreign/abi/s390/linux/LinuxS390CallArranger.java line 221: > 219: Class type = > SharedUtils.primitiveCarrierForSize(layout.byteSize(), false); > 220: bindings.bufferLoad(0, type) > 221: .vmStore(storage, type); Maybe improve indentation? src/java.base/share/classes/jdk/internal/foreign/abi/s390/linux/TypeClass.java line 44: > 42: FLOAT; > 43: > 44: private static final int MAX_AGGREGATE_REGS_SIZE = 1; Unused. - PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1285885012 PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1285886685
Re: RFR: 8311630: [s390] Implementation of Foreign Function & Memory API (Preview) [v10]
On Tue, 1 Aug 2023 06:29:28 GMT, sid8606 wrote:
>> Implementation of "Foreign Function & Memory API" for s390x (Big Endian).
>
> sid8606 has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Fix typo
>
> Signed-off-by: Sidraya
src/hotspot/cpu/s390/foreignGlobals_s390.cpp line 154:
> 152: } else {
> 153: assert(to_reg.stack_size() == 4, "size should match");
> 154: // s390 needs 4 Byte offset
Seems like this comment should get removed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14801#discussion_r1285880721
Integrated: 8313880: Incorrect copyright header in jdk/java/foreign/TestFree.java after JDK-8310643
This PR fixes a formatting error in a copyright header. - Commit messages: - Restore enablePreview - Fix copyright formatting in TestFree Changes: https://git.openjdk.org/jdk/pull/15175/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15175=00 Issue: https://bugs.openjdk.org/browse/JDK-8313880 Stats: 6 lines in 1 file changed: 2 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/15175.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15175/head:pull/15175 PR: https://git.openjdk.org/jdk/pull/15175
Re: Integrated: 8313880: Incorrect copyright header in jdk/java/foreign/TestFree.java after JDK-8310643
On Mon, 7 Aug 2023 12:27:39 GMT, Per Minborg wrote: > This PR fixes a formatting error in a copyright header. Looks good and trivial to me. - Marked as reviewed by thartmann (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15175#pullrequestreview-1565217211
Integrated: 8313880: Incorrect copyright header in jdk/java/foreign/TestFree.java after JDK-8310643
On Mon, 7 Aug 2023 12:27:39 GMT, Per Minborg wrote: > This PR fixes a formatting error in a copyright header. This pull request has now been integrated. Changeset: bbbfa217 Author:Per Minborg URL: https://git.openjdk.org/jdk/commit/bbbfa217a030e90e41c036203f85b764927f4848 Stats: 6 lines in 1 file changed: 2 ins; 2 del; 2 mod 8313880: Incorrect copyright header in jdk/java/foreign/TestFree.java after JDK-8310643 Reviewed-by: thartmann - PR: https://git.openjdk.org/jdk/pull/15175
Re: RFR: 8288899: java/util/concurrent/ExecutorService/CloseTest.java failed with "InterruptedException: sleep interrupted" [v2]
> Addresses Jdk 8288899 : java/util/concurrent/ExecutorService/CloseTest.java > failed with "InterruptedException: sleep interrupted" and related issues. > > This is a major ForkJoin update (and hard to review -- sorry) that finally > addresses incompatibilities between ExecutorService and ForkJoinPool (which > claims to implement it), with the goal of avoiding continuing bug reports and > incompatibilities. Doing this required reworking internal control to use > phaser/seqlock-style versioning schemes (affecting nearly every method) that > ensure consistent data structures and actions without requiring global > synchronization or locking on every task execution that would massively > degrade performance. The previous lack of a solution to this was the main > reason for these incompatibilities. Doug Lea has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 40 additional commits since the last revision: - Merge branch 'openjdk:master' into JDK-8288899 - Rework versioning - Merge branch 'openjdk:master' into JDK-8288899 - Merge branch 'openjdk:master' into JDK-8288899 - Merge branch 'openjdk:master' into JDK-8288899 - Merge branch 'openjdk:master' into JDK-8288899 - Merge branch 'openjdk:master' into JDK-8288899 - Reduce contention - minor improvements and renamings - Merge branch 'openjdk:master' into JDK-8288899 - ... and 30 more: https://git.openjdk.org/jdk/compare/8891d660...e29bbca3 - Changes: - all: https://git.openjdk.org/jdk/pull/14301/files - new: https://git.openjdk.org/jdk/pull/14301/files/6a116f50..e29bbca3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14301=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14301=00-01 Stats: 207739 lines in 3515 files changed: 99360 ins; 90418 del; 17961 mod Patch: https://git.openjdk.org/jdk/pull/14301.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14301/head:pull/14301 PR: https://git.openjdk.org/jdk/pull/14301
Re: RFR: 8313875: Make fields final in java.util.Math: twoToTheDoubleScaleUp, twoToTheDoubleScaleDown
On Thu, 13 Jul 2023 17:57:16 GMT, Andrey Turbanov wrote: > Couple of static fields in Math could be declared `final`. Looks good and trivial. - Marked as reviewed by redestad (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14875#pullrequestreview-1565168404
Re: RFR: 8310643: Misformatted copyright messages in FFM [v3]
> This PR suggests updating some of the ill-formed copyright headers in the FFM > API and the implementation and test thereof. > > Some of the test files will have now the "classpath" exception. Is this > correct? Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Restore removed test definition - Changes: - all: https://git.openjdk.org/jdk/pull/15042/files - new: https://git.openjdk.org/jdk/pull/15042/files/d81d0c4e..c0a50714 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15042=02 - incr: https://webrevs.openjdk.org/?repo=jdk=15042=01-02 Stats: 8 lines in 1 file changed: 8 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15042.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15042/head:pull/15042 PR: https://git.openjdk.org/jdk/pull/15042
Integrated: 8310643: Misformatted copyright messages in FFM
On Wed, 26 Jul 2023 15:43:12 GMT, Per Minborg wrote: > This PR suggests updating some of the ill-formed copyright headers in the FFM > API and the implementation and test thereof. > > Some of the test files will have now the "classpath" exception. Is this > correct? This pull request has now been integrated. Changeset: 0b4387e3 Author:Per Minborg URL: https://git.openjdk.org/jdk/commit/0b4387e3a33dd360efc5856126394739256505f8 Stats: 1012 lines in 58 files changed: 102 ins; 113 del; 797 mod 8310643: Misformatted copyright messages in FFM Reviewed-by: jvernee - PR: https://git.openjdk.org/jdk/pull/15042
Re: RFR: 8310643: Misformatted copyright messages in FFM [v3]
On Mon, 7 Aug 2023 10:53:20 GMT, Per Minborg wrote: >> This PR suggests updating some of the ill-formed copyright headers in the >> FFM API and the implementation and test thereof. >> >> Some of the test files will have now the "classpath" exception. Is this >> correct? > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Restore removed test definition Marked as reviewed by jvernee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15042#pullrequestreview-1565069902
RFR: 8313875: Make fields final in java.util.Math: twoToTheDoubleScaleUp, twoToTheDoubleScaleDown
Couple of static fields in Math could be declared `final`. - Commit messages: - [PATCH] Make fields final in java.util.Math: twoToTheDoubleScaleUp, twoToTheDoubleScaleDown Changes: https://git.openjdk.org/jdk/pull/14875/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14875=00 Issue: https://bugs.openjdk.org/browse/JDK-8313875 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14875.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14875/head:pull/14875 PR: https://git.openjdk.org/jdk/pull/14875
Re: RFR: 8313657 : com.sun.jndi.ldap.Connection.cleanup does not close connections on SocketTimeoutErrors [v3]
On Fri, 4 Aug 2023 19:09:58 GMT, Weibing Xiao wrote:
>> com.sun.jndi.ldap.Connection::leanup does not close the underlying socket if
>> the is an IOException generation when the output stream was flushing the
>> buffer.
>>
>> Please refer to the bug https://bugs.openjdk.org/browse/JDK-8313657.
>
> Weibing Xiao has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update error message
test/jdk/com/sun/jndi/ldap/SocketCloseTest.java line 62:
> 60: props.put(Context.PROVIDER_URL,
> "ldap://localhost:1389/o=example;);
> 61: props.put("java.naming.ldap.factory.socket",
> CustomSocketFactory.class.getName());
> 62: try {
this will execute twice: once in the agentvm main and also, as desired, in the
test JVM main.
The agentvm main sees a ClassNotFoundException being thrown and then the output
of "The socket was not closed. " in the log c.f. other comments and bug
comments for further details to avoid execution in agentvm or use of othervm
test mode execution
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15143#discussion_r1285675453
Re: RFR: 8313657 : com.sun.jndi.ldap.Connection.cleanup does not close connections on SocketTimeoutErrors [v3]
On Fri, 4 Aug 2023 19:09:58 GMT, Weibing Xiao wrote:
>> com.sun.jndi.ldap.Connection::leanup does not close the underlying socket if
>> the is an IOException generation when the output stream was flushing the
>> buffer.
>>
>> Please refer to the bug https://bugs.openjdk.org/browse/JDK-8313657.
>
> Weibing Xiao has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update error message
I ran the test multiple times in mach5 and it executes fine. However, while
examining the log output I noticed the "The socket was not closed. " output.
This is a little confusing. While looking at the test and exploring the origins
of this outout, we see that the test is essentially an othervm execution. As
such, I restructured the test to othervm and it executes with some informative
output and eliminates the discombobulating output.
I have left a comment in the JBS bug item with some further details and changes
for othervm. This gives some imformative output as the test executes.
The origins of the output is the main try block executing in the agentvm and
the throwing of a ClassNotFoundException.
If you don't wish to do otherrvm and to avoid the "The socket was not closed.
" output, and if wish to retain the same structure, then consider adding a run
command to the test with an arg, and then a conditional on the core test logic
e.g.
* @run main SocketCloseTest launcher
and in the main method
public static void main(String[] args) throws Exception {
Hashtable props = new Hashtable<>();
if (args.length == 0) { // only executed in the launched JVM
props.put(Context.INITIAL_CONTEXT_FACTORY,
"com.sun.jndi.ldap.LdapCtxFactory");
props.put(Context.PROVIDER_URL, "ldap://localhost:1389/o=example;);
props.put("java.naming.ldap.factory.socket",
CustomSocketFactory.class.getName());
try {
final DirContext ctx = new InitialDirContext(props);
} catch (Exception e) {
if (CustomSocketFactory.customSocket.closeMethodCalledCount() > 0) {
System.out.println(SOCKET_CLOSED_MSG);
} else {
System.out.println(SOCKET_NOT_CLOSED_MSG);
}
}
}
OutputAnalyzer outputAnalyzer =
ProcessTools.executeTestJvm("SocketCloseTest_II");
outputAnalyzer.stdoutShouldContain(SOCKET_CLOSED_MSG);
outputAnalyzer.stdoutShouldNotContain(SOCKET_NOT_CLOSED_MSG);
outputAnalyzer.stdoutShouldContain(BAD_FLUSH);
}
-
PR Comment: https://git.openjdk.org/jdk/pull/15143#issuecomment-1667550730
Re: RFR: 8310643: Misformatted copyright messages in FFM [v2]
On Mon, 7 Aug 2023 08:57:56 GMT, Per Minborg wrote: >> This PR suggests updating some of the ill-formed copyright headers in the >> FFM API and the implementation and test thereof. >> >> Some of the test files will have now the "classpath" exception. Is this >> correct? > > Per Minborg has updated the pull request incrementally with two additional > commits since the last revision: > > - Remove classpath exception from test files > - Add classpath exception to some internal files test/jdk/java/foreign/TestIllegalLink.java line 30: > 28: * @requires jdk.foreign.linker != "UNSUPPORTED" > 29: * @modules java.base/jdk.internal.foreign > 30: * @run testng/othervm --enable-native-access=ALL-UNNAMED TestIllegalLink This is removing the jtreg test. - PR Review Comment: https://git.openjdk.org/jdk/pull/15042#discussion_r1285595093
Re: RFR: 8310643: Misformatted copyright messages in FFM [v2]
> This PR suggests updating some of the ill-formed copyright headers in the FFM > API and the implementation and test thereof. > > Some of the test files will have now the "classpath" exception. Is this > correct? Per Minborg has updated the pull request incrementally with two additional commits since the last revision: - Remove classpath exception from test files - Add classpath exception to some internal files - Changes: - all: https://git.openjdk.org/jdk/pull/15042/files - new: https://git.openjdk.org/jdk/pull/15042/files/3389537c..d81d0c4e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15042=01 - incr: https://webrevs.openjdk.org/?repo=jdk=15042=00-01 Stats: 135 lines in 45 files changed: 8 ins; 82 del; 45 mod Patch: https://git.openjdk.org/jdk/pull/15042.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15042/head:pull/15042 PR: https://git.openjdk.org/jdk/pull/15042
Re: RFR: 8313657 : com.sun.jndi.ldap.Connection.cleanup does not close connections on SocketTimeoutErrors [v3]
On Mon, 7 Aug 2023 06:42:54 GMT, Vyom Tewari wrote:
>> Weibing Xiao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update error message
>
> src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 702:
>
>> 700:
>> 701: // close socket
>> 702: private void closeConnectionSocket() {
>
> 'closeSocketConnection' will be more meaningful.
the suggested name was to convey the closing of the Connection's socket, not a
socket connection per se.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15143#discussion_r1285577687
Re: RFR: 8310643: Misformatted copyright messages in FFM
On Sat, 29 Jul 2023 12:29:13 GMT, David Holmes wrote: >> This PR suggests updating some of the ill-formed copyright headers in the >> FFM API and the implementation and test thereof. >> >> Some of the test files will have now the "classpath" exception. Is this >> correct? > > src/java.base/share/classes/java/lang/foreign/GroupLayout.java line 25: > >> 23: * questions. >> 24: */ >> 25: > > Is there some style guideline that says there needs to be a blank line here? This is common for many JDK files. - PR Review Comment: https://git.openjdk.org/jdk/pull/15042#discussion_r1285558048
Re: RFR: 8313657 : com.sun.jndi.ldap.Connection.cleanup does not close connections on SocketTimeoutErrors [v3]
On Fri, 4 Aug 2023 19:09:58 GMT, Weibing Xiao wrote:
>> com.sun.jndi.ldap.Connection::leanup does not close the underlying socket if
>> the is an IOException generation when the output stream was flushing the
>> buffer.
>>
>> Please refer to the bug https://bugs.openjdk.org/browse/JDK-8313657.
>
> Weibing Xiao has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update error message
src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 686:
> 684:
> 685: // flush and close output stream
> 686: private void flushCloseOutputStream() {
'flushAndCloseOutputStream' will be more meaningful name.
src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 702:
> 700:
> 701: // close socket
> 702: private void closeConnectionSocket() {
'closeSocketConnection' will be more meaningful.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15143#discussion_r1285437029
PR Review Comment: https://git.openjdk.org/jdk/pull/15143#discussion_r1285438547
Re: RFR: JDK-8313670: Simplify shared lib name handling code in some tests
On Fri, 4 Aug 2023 20:55:25 GMT, Serguei Spitsyn wrote: >> test/lib/jdk/test/lib/Platform.java line 376: >> >>> 374: } >>> 375: } >>> 376: >> >> The following tests could leverage this new API. Just look for calls to >> `Platform.sharedLibraryExt()`: >> >> test/hotspot/jtreg/runtime/signal/SigTestDriver.java >> test/hotspot/jtreg/vmTestbase/nsk/jvmti/NativeLibraryCopier.java >> test/jdk/com/sun/tools/attach/warnings/DynamicLoadWarningTest.java >> >> Perhaps a `Platform.buildSharedLibraryName()` API is worth considering. > > The fix looks good in general. > But I like the suggestion from Chris above Agreed, I'd like to see `Platform.buildSharedLibraryName()` in addition to the other methods (the latter are needed if you want to decompose a filename to get the library name (though we could also provide a function just to do that). - PR Review Comment: https://git.openjdk.org/jdk/pull/15151#discussion_r1285438334
