Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which >> JDK version to target. >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e I'm sorry for the noise with my comments. I'll continue to discuss it privately unless there is something really important. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e
test/hotspot/jtreg/serviceability/jvmti/events/ClassPrepare/classprep01/libclassprep01.cpp
line 59:
> 57: static jvmtiEventCallbacks callbacks;
> 58: static jint result = PASSED;
> 59: static volatile size_t eventsCount = 0; // TODO these 2 vars mofified
> from different threads in getReady/check. What to DO???
I'd suggest to use RawMonitorLocker with event_lock or counter_lock to protect
updates of these counters.
You can remove this comment then.
test/hotspot/jtreg/serviceability/jvmti/events/ClassPrepare/classprep01/libclassprep01.cpp
line 201:
> 199: LOG("\n");
> 200:
> 201:
Too many empty lines. It might make sense to do a common cleanup with all edits
like this.
test/hotspot/jtreg/serviceability/jvmti/events/ClassPrepare/classprep01/libclassprep01.cpp
line 258:
> 256: return JNI_VERSION_1_8;
> 257: }
> 258: #endif
Empty line is missed => common cleanup.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which >> JDK version to target. >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e test/hotspot/jtreg/serviceability/jvmti/events/Exception/exception01/libexception01.cpp line 178: > 176:ex.c_cls, ex.c_name, ex.c_sig, > 177:(jint)(ex.c_loc >> 32), (jint)ex.c_loc); > 178: result = STATUS_FAILED; Unaligned lines in the range: 172-177. There are some non-uinified unneeded spaces at lines 172,175. test/hotspot/jtreg/serviceability/jvmti/events/Exception/exception01/libexception01.cpp line 287: > 285: } > 286: > 287: eventsCount = 0; Counters are not protected with locks. I'm not sure it is a real problem here but would be better to double-check. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator [v4]
> Hi all, > > The Current Vector API doc for `LSHR` is > > Produce a>>>(n&(ESIZE*8-1)). Integral only. > > > This is misleading which may lead to bugs for Java developers. > This is because for negative byte/short elements, the results computed by > `LSHR` will be different from that of `>>>`. > For more details, please see > https://github.com/openjdk/jdk/pull/8276#issue-1206391831 . > > After the patch, the doc for `LSHR` is > > Produce zero-extended right shift of a by (n&(ESIZE*8-1)) bits. Integral only. > > > Thanks. > Best regards, > Jie Jie Fu 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 eight additional commits since the last revision: - Address CSR review comments - Merge branch 'master' into JDK-8284992 - Address review comments - Merge branch 'master' into JDK-8284992 - Merge branch 'master' into JDK-8284992 - Address review comments - Merge branch 'master' into JDK-8284992 - 8284992: Fix misleading Vector API doc for LSHR operator - Changes: - all: https://git.openjdk.java.net/jdk/pull/8291/files - new: https://git.openjdk.java.net/jdk/pull/8291/files/7e82e721..0161571b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8291&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8291&range=02-03 Stats: 6657 lines in 233 files changed: 5591 ins; 490 del; 576 mod Patch: https://git.openjdk.java.net/jdk/pull/8291.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8291/head:pull/8291 PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator [v3]
On Thu, 28 Apr 2022 19:48:18 GMT, Paul Sandoz wrote: >> Jie Fu 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 six additional commits since >> the last revision: >> >> - Address review comments >> - Merge branch 'master' into JDK-8284992 >> - Merge branch 'master' into JDK-8284992 >> - Address review comments >> - Merge branch 'master' into JDK-8284992 >> - 8284992: Fix misleading Vector API doc for LSHR operator > > It should be possible for you finalize now. Hi @PaulSandoz , the CSR had been approved and I pushed one more commit to address the CSR review comments. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which >> JDK version to target. >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e test/hotspot/jtreg/serviceability/jvmti/events/ClassLoad/classload01/libclassload01.cpp line 139: > 137: primClsEvents[i]++; > 138: LOG( > 139: "TEST FAILED: JVMTI_EVENT_CLASS_LOAD event received for\n" Combine 138+138. I won't comment this more in hope the same will be fixed in all tests. test/hotspot/jtreg/serviceability/jvmti/events/ClassLoad/classload01/libclassload01.cpp line 177: > 175: return JNI_VERSION_1_8; > 176: } > 177: #endif One empty line would be nice to add after 177. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e
test/hotspot/jtreg/serviceability/jvmti/events/Breakpoint/breakpoint01/libbreakpoint01.cpp
line 139:
> 137:thr_info.name,(jni->IsVirtualThread(thread) ==
> JNI_TRUE) ? "virtual" : "kernel",
> 138: (thr_info.is_daemon == JNI_TRUE) ? "deamon" : "user");
> 139: }
I'd suggest to add locals (their names are up to you):
const char* thr_virtual_tag = jni->IsVirtualThread(thread) == JNI_TRUE ?
"virtual" : "kernel";
const char* thr_daemon_tag == JNI_TRUE) ? "deamon" : "user";
It is better to place togeter lines: 129+130.
Line 137 is not aligned, and there are many unneeded spaces.
The '()' around conditions are not needed. At least, I do not see them
increasing readability.
test/hotspot/jtreg/serviceability/jvmti/events/Breakpoint/breakpoint01/libbreakpoint01.cpp
line 167:
> 165: result = checkStatus = STATUS_FAILED;
> 166: LOG(
> 167: "TEST FAILED: Breakpoint event with unexpected class
> signature:\n"
Combine lines 166+167.
test/hotspot/jtreg/serviceability/jvmti/events/Breakpoint/breakpoint01/libbreakpoint01.cpp
line 187:
> 185: jboolean isVirtual = jni->IsVirtualThread(thread);
> 186: if (isVirtual != METHODS_ATTRS[i]) {
> 187: LOG("TEST FAILED: IsVirtualThread check failed with unexpected
> result %d when expected is %d\n", isVirtual, METHODS_ATTRS[i]);
Line 187 is too long and can be splitted.
test/hotspot/jtreg/serviceability/jvmti/events/Breakpoint/breakpoint01/libbreakpoint01.cpp
line 231:
> 229: result = STATUS_FAILED;
> 230: LOG(
> 231: "TEST FAILED: wrong number of Breakpoint events\n"
Combine 230+231.
test/hotspot/jtreg/serviceability/jvmti/events/Breakpoint/breakpoint01/libbreakpoint01.cpp
line 307:
> 305: if (agent_lock == NULL) {
> 306: return JNI_ERR;
> 307: }
Better to also use same style with curly brackets in fragments: 293, 296, 299.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which >> JDK version to target. >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e test/hotspot/jtreg/serviceability/jvmti/events/Breakpoint/breakpoint01/libbreakpoint01.cpp line 66: > 64: for (i = 0; i < METH_NUM; i++) > 65: bpEvents[i] = 0; > 66: } As I understand all new tests are C++ based. So, I'd suggest to always use C++ style of loops: for (int i = 0; i < METH_NUM; i++) - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: JDK-8285730: unify _WIN32_WINNT settings
On Wed, 27 Apr 2022 14:57:41 GMT, Matthias Baesken wrote: > Currently we set _WIN32_WINNT at various places in the codebase; this is used > to target a minimum Windows version we want to support. See also for more > detailled information : > https://docs.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers?redirectedfrom=MSDN#setting-winver-or-_win32_winnt > Macros for Conditional Declarations > "Certain functions that depend on a particular version of Windows are > declared using conditional code. This enables you to use the compiler to > detect whether your application uses functions that are not supported on its > target version(s) of Windows." > > However currently we have quite a lot of differing settings of _WIN32_WINNT > in the codebase ; setting _WIN32_WINNT to 0x0601 (Windows 7) where possible > would make sense because we have this setting already at java_props_md.c > (so targeting older Windows versions at other places is most likely not > useful). That only seems to be half of the issue though. If we are defining _WIN32_WINNT=0x0601 because the minimum required OS API support level is Windows 7, then don't we need a check that the build platform is also at least Windows 7? - PR: https://git.openjdk.java.net/jdk/pull/8428
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which >> JDK version to target. >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e Marked as reviewed by will-mol...@github.com (no known OpenJDK username). - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v5]
On Fri, 29 Apr 2022 02:54:06 GMT, Joe Darcy wrote: > > Is there intent to backport this to the RI for JSR 337 MR 4? > > I think that would be helpful, if not strictly necessary. PS And if backported, the spec could be updated for earlier release trains to start at a different value like "4". - PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
On Fri, 29 Apr 2022 03:00:40 GMT, Stuart Marks wrote: >> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch >> in the bug report that breaks `IdentityHashMap` now causes several cases in >> this new test to fail. There's more that could be done, but the new tests >> cover most of the core functions of `IdentityHashMap`. Unfortunately it >> seems difficult to merge this with the existing, comprehensive Collections >> tests (e.g., MOAT.java) because those tests implicity rely on >> `equals()`-based contract instead of the special-purpose `==`-based contract >> used by `IdentityHashMap`. > > Stuart Marks has updated the pull request incrementally with two additional > commits since the last revision: > > - Assertions over return values. Some refinement of equals() testing. > - Add comment about Map.Entry identity not guaranteed. I've added checking of return values for I think everything that has a significant return value. I've elected to store the return value in a local variable and add a separate assert line. For example, instead of assertNull(map.put(newKey, newVal)); I've done Box r = map.put(newKey, newVal); assertNull(r); The reason is that I think it separates the test setup/action from the test assertions. I tried it the first way, but it felt like the lack of this separation made things messy. I've also added a couple more tests over `equals` and added more asserts over `equals` to the `keySet` and `entrySet` view sets. (Note, the `values` collection is just a `Collection` and thus doesn't have a defined notion of `equals`.) The testing of the view collections could probably be made more comprehensive, but I think what's here is a good start. Please take a look. - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch > in the bug report that breaks `IdentityHashMap` now causes several cases in > this new test to fail. There's more that could be done, but the new tests > cover most of the core functions of `IdentityHashMap`. Unfortunately it seems > difficult to merge this with the existing, comprehensive Collections tests > (e.g., MOAT.java) because those tests implicity rely on `equals()`-based > contract instead of the special-purpose `==`-based contract used by > `IdentityHashMap`. Stuart Marks has updated the pull request incrementally with two additional commits since the last revision: - Assertions over return values. Some refinement of equals() testing. - Add comment about Map.Entry identity not guaranteed. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8354/files - new: https://git.openjdk.java.net/jdk/pull/8354/files/5857fe6f..d66ad0b8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=00-01 Stats: 117 lines in 1 file changed: 84 ins; 2 del; 31 mod Patch: https://git.openjdk.java.net/jdk/pull/8354.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8354/head:pull/8354 PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v5]
On Fri, 29 Apr 2022 02:12:19 GMT, Iris Clark wrote:
>> Joe Darcy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Change punctuation from review feedback.
>
> src/java.base/share/classes/java/lang/System.java line 743:
>
>> 741: * have the value {@code "1"}; after a second maintenance
>> 742: * release, this property will have the value {@code "2"},
>> 743: * and so on.
>
> There should be no requirement that values be allocated sequentially. In
> other words, if JCP MR does not require an RI, then it should not be
> surprising if there is no JDK build with maintenance version . As an
> example, JSR 337 MR 1 and MR 2 both used the same RI. If this system
> property had existed during development of MR 1, it would return "1". Since
> no RI was submitted for MR 2, a build returning "2" should never exist. MR 3
> did update the RI, so it would return "3".
> Is there intent to backport this to the RI for JSR 337 MR 4?
I think that would be helpful, if not strictly necessary.
-
PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v5]
On Fri, 29 Apr 2022 02:00:08 GMT, Iris Clark wrote:
>> Joe Darcy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Change punctuation from review feedback.
>
> src/java.base/share/classes/java/lang/VersionProps.java.template line 113:
>
>> 111: props.put("java.specification.version", VERSION_SPECIFICATION);
>> 112:
>> 113: // Uncomment next props.put call after the first maintenance
>> release for a
>
> Is "after" correct? I think this value should be set to the target MR at the
> beginning of RI development for the release, similar to what is done for
> other version identification system properties.
Well, sure -- this versioning info should be updated in the same way we update
versions for all the early access builds of a release.
-
PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v5]
On Fri, 29 Apr 2022 01:20:17 GMT, Joe Darcy wrote:
>> Add a new system property, java.specification.maintenance.version, to return
>> the maintenance release number of the Java SE specification being
>> implemented. The property is unset, optional in the terminology of
>> System.getProperties, for an initial release of a specification.
>>
>> Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8285764
>>
>> I'll update copyright years before an integration.
>
> Joe Darcy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Change punctuation from review feedback.
Is there intent to backport this to the RI for JSR 337 MR 4?
src/java.base/share/classes/java/lang/System.java line 743:
> 741: * have the value {@code "1"}; after a second maintenance
> 742: * release, this property will have the value {@code "2"},
> 743: * and so on.
There should be no requirement that values be allocated sequentially. In other
words, if JCP MR does not require an RI, then it should not be surprising
if there is no JDK build with maintenance version . As an example, JSR 337
MR 1 and MR 2 both used the same RI. If this system property had existed
during development of MR 1, it would return "1". Since no RI was submitted for
MR 2, a build returning "2" should never exist. MR 3 did update the RI, so it
would return "3".
src/java.base/share/classes/java/lang/VersionProps.java.template line 113:
> 111: props.put("java.specification.version", VERSION_SPECIFICATION);
> 112:
> 113: // Uncomment next props.put call after the first maintenance
> release for a
Is "after" correct? I think this value should be set to the target MR at the
beginning of RI development for the release, similar to what is done for other
version identification system properties.
-
Changes requested by iris (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8437
Re: Improve `finally` block exception handling
On 21/04/2022 7:30 am,
some-java-user-99206970363698485...@vodafonemail.de wrote:
Thanks a lot for your feedback! My original message was a bit vague in parts,
sorry for that.
The proposal consists of the following:
1. Forbidding usage of `break`, `continue`, `yield` and `return` in `finally`
blocks
2. Adding exceptions as suppressed exceptions:
If exception E1 led to execution of the `finally` block and the block is
left due to
another exception E2, then either E1 or E2 should be thrown with the other
one added
as suppressed exception. For consistency with try-with-resources and
because E1 is
most likely more relevant ideally E1 would be thrown and E2 should be
suppressed. But
if you think the impact on backward compatibility would be too large, then
E2 should
be thrown (the current behavior), but E1 should at least be added as
suppressed exception.
The following replies to your comments hopefully make the rationale for these
proposed
changes clearer.
The behaviour of try/catch/finally is not "obvious at all", you have to read
what it means
and when you do that you clearly see what happens regarding exceptions.
For try-catch you see that the code catches some specific exception and then
handles it in a
certain way, whether that is by rethrowing, logging or itentionally ignoring it.
The issue with `finally` is that in its current form, exceptions which occurred
in the `try`
block might just silently disappear. Consider this simple example:
```
try {
throw new RuntimeException();
} finally {
return true;
}
```
Here it is not clear at all, unless you have read the JLS in detail, that the
thrown exception
just vanishes. There is no explicit indication that the `finally` has any
effect on the
thrown exception. Of course this is a contrived example, but consider the same
situation where
the `try` block calls a method which might throw an exception (or the general
case that a
VirtualMachineError occurs), and that the `return` (or any of the other
affected statements)
is not the only statement in the `finally` block.
Similar confusing code can be written where the `try` or `catch` block returns
a value
(or continues or breaks loop iteration), but the `finally` block overwrites
that action:
```
try {
return 1;
} finally {
return 2;
}
```
Again, contrived, but consider the same code with additional statements in the
`try` and
`finally` blocks. This breaks fundamental assumptions one has about the
behavior of the
statements, such as `return`.
I still maintain this is simply a matter of user education. You have no
business using a finally block if you have never bothered to even learn
how it works. Sorry no sympathy from me on that one.
Are there any plans to forbid usage of `break`, `continue`, `yield` and
`return` in
`finally` blocks?
Why would we do that? What would that gain?
It would prevent code such as the one shown above, where code, most likely
unintentionally,
discards exceptions. Now also consider that inside the `try` block a
VirtualMachineError
(or a subclass of it) may occur which you really should not catch. Yet the code
in the
`finally` block silently discards it without you ever noticing the error,
before it occurs
later in another part of the application again and has possibly already
corrupted the state
of the application.
Again this is primarily user education. And there are times when you
definitely do want to use those statements and you would have to jump
through hoops to get the same effect if they were banned.
But in terms of "are there any plans ..." the answer is a clear no as
the compatibility issues would make this a non-starter.
Similarly for `throw` and any implicitly thrown exceptions, is there a plan to
at least
add the original exception as suppressed one to the newly thrown?
That suggestion is not completely without merit, but nor is it a "slam-dunk" obvious thing to do.
The semantic implications of the exceptions matter, and semantics come from the programmers intent. There's
no reasonable way to automagically determine that when an exception is created that another exception (that
led to the finally block) should be inserted as a "suppressed exception". That would actually be
completely wrong to do in many circumstances you would instead need to act when the exception would terminate
the finally block and then add the original exception as the "suppressed" exception.
To clarify my suggestion: If a `finally` block is entered due to an exception
E1, and is
exited due to an exception E2 (regardless of whether explicitly thrown by a
`throw` statement),
and E1 != E2, then both exceptions should be preserved, with one being added as
suppressed
exception to the other one.
That is not unreasonable: E2 suppresses E1.
But really the programmer is in the best position to decide how exceptions need
to be handled.
Except that in a `finally` block they don't have access
Re: RFR: 8285295: Need better testing for IdentityHashMap
On Fri, 29 Apr 2022 00:06:32 GMT, Stuart Marks wrote: >> test/jdk/java/util/IdentityHashMap/Basic.java line 257: >> >>> 255: checkEntries(map.entrySet(), entry(k1b, v1b), >>> 256: entry(k2, v2)); >>> 257: } >> >> Would an additional check `assertFalse(map.equals(map2));` be useful here >> (and other similar tests where we do "remove"). > > I don't know if you noticed that previous versions checked the map's contents > with `map.equals(map2)` and either asserted true or false depending on the > situation. I pulled most of those out when I added `checkEntries`. The reason > is that `checkEntries` and `checkElements` are supposed to check the exact > contents of the map or the collection, and they fail if there is a missing, > different, or extra entry or element. If that's true we don't need to test > `map.equals`. I don't think it calling `map.equals` adds any value or does > any checking that the `checkX` methods don't already do. > > Of course this relies on `checkEntries` and `checkElements` to do their jobs > properly, so we should make sure they do! > > And also we need to test that the `equals` method is doing its job as well. > There are a couple tests for it already, and they test at least the basic > cases. But it's possible I might have missed something. > > In any case, if we believe the `checkX` methods are sufficient, and if we > believe that the tests for `equals` are also sufficient, then I don't think > we need to add assertions about `equals` in any tests other than for `equals` > itself. Hello Stuart, Thank you for the explanation. > In any case, if we believe the checkX methods are sufficient, and if we > believe that the tests for equals are also sufficient, then I don't think we > need to add assertions about equals in any tests other than for equals itself. That makes sense. - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v5]
On Fri, 29 Apr 2022 01:20:17 GMT, Joe Darcy wrote: >> Add a new system property, java.specification.maintenance.version, to return >> the maintenance release number of the Java SE specification being >> implemented. The property is unset, optional in the terminology of >> System.getProperties, for an initial release of a specification. >> >> Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8285764 >> >> I'll update copyright years before an integration. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Change punctuation from review feedback. Thank you for the updates, Joe. Looks good to me. - Marked as reviewed by jpai (Committer). PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v4]
On Thu, 28 Apr 2022 23:56:27 GMT, Mark Reinhold wrote:
>> Joe Darcy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Respond to review feedback; make sequence of values explicit.
>
> src/java.base/share/classes/java/lang/System.java line 740:
>
>> 738: * > href="https://jcp.org/en/procedures/jcp2#3.6.4";>maintenance
>> 739: * release (optional).
>> 740: * After a first maintenance release, this property will
>
> Separate independent clauses with a semicolon: `After a first maintenance
> release this property will have the value {@code "1"}; after a second
> maintenance release it will have the value {@code "2"}, and so on.`
PR and CSR updated.
-
PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v5]
> Add a new system property, java.specification.maintenance.version, to return > the maintenance release number of the Java SE specification being > implemented. The property is unset, optional in the terminology of > System.getProperties, for an initial release of a specification. > > Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8285764 > > I'll update copyright years before an integration. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Change punctuation from review feedback. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8437/files - new: https://git.openjdk.java.net/jdk/pull/8437/files/461407aa..2048aaac Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8437&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8437&range=03-04 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8437.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8437/head:pull/8437 PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which >> JDK version to target. >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e Remind: please use the command `/jep JEP-425` [1] to mark this PR. [1] https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/jep - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v34]
On Thu, 28 Apr 2022 18:10:57 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-424 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.java.net/jeps/424 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Downcall and upcall tests use wrong layout which is missing some trailing > padding Remind: please use the command `/jep JEP-424` [1] to mark this PR. [1] https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/jep - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8285295: Need better testing for IdentityHashMap
On Thu, 28 Apr 2022 13:17:59 GMT, Jaikiran Pai wrote: >> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch >> in the bug report that breaks `IdentityHashMap` now causes several cases in >> this new test to fail. There's more that could be done, but the new tests >> cover most of the core functions of `IdentityHashMap`. Unfortunately it >> seems difficult to merge this with the existing, comprehensive Collections >> tests (e.g., MOAT.java) because those tests implicity rely on >> `equals()`-based contract instead of the special-purpose `==`-based contract >> used by `IdentityHashMap`. > > test/jdk/java/util/IdentityHashMap/Basic.java line 257: > >> 255: checkEntries(map.entrySet(), entry(k1b, v1b), >> 256: entry(k2, v2)); >> 257: } > > Would an additional check `assertFalse(map.equals(map2));` be useful here > (and other similar tests where we do "remove"). I don't know if you noticed that previous versions checked the map's contents with `map.equals(map2)` and either asserted true or false depending on the situation. I pulled most of those out when I added `checkEntries`. The reason is that `checkEntries` and `checkElements` are supposed to check the exact contents of the map or the collection, and they fail if there is a missing, different, or extra entry or element. If that's true we don't need to test `map.equals`. I don't think it calling `map.equals` adds any value or does any checking that the `checkX` methods don't already do. Of course this relies on `checkEntries` and `checkElements` to do their jobs properly, so we should make sure they do! And also we need to test that the `equals` method is doing its job as well. There are a couple tests for it already, and they test at least the basic cases. But it's possible I might have missed something. In any case, if we believe the `checkX` methods are sufficient, and if we believe that the tests for `equals` are also sufficient, then I don't think we need to add assertions about `equals` in any tests other than for `equals` itself. - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap
On Thu, 28 Apr 2022 13:22:34 GMT, Jaikiran Pai wrote:
>> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch
>> in the bug report that breaks `IdentityHashMap` now causes several cases in
>> this new test to fail. There's more that could be done, but the new tests
>> cover most of the core functions of `IdentityHashMap`. Unfortunately it
>> seems difficult to merge this with the existing, comprehensive Collections
>> tests (e.g., MOAT.java) because those tests implicity rely on
>> `equals()`-based contract instead of the special-purpose `==`-based contract
>> used by `IdentityHashMap`.
>
> test/jdk/java/util/IdentityHashMap/Basic.java line 337:
>
>> 335: public void testPutOverwrite() {
>> 336: Box newVal = new Box(v1a);
>> 337: map.put(k1a, newVal);
>
> Should we capture the return value and assert that it's not null and is
> identity equal to `v1a`?
>
> Perhaps, similarly at a few other places where we do put and putIfAbsent?
Yes, good point, I'll add checks of the return values in the appropriate
places. There are several, including `remove`, `computeX`, `put`, etc.
-
PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v4]
On Thu, 28 Apr 2022 20:54:29 GMT, Joe Darcy wrote:
>> Add a new system property, java.specification.maintenance.version, to return
>> the maintenance release number of the Java SE specification being
>> implemented. The property is unset, optional in the terminology of
>> System.getProperties, for an initial release of a specification.
>>
>> Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8285764
>>
>> I'll update copyright years before an integration.
>
> Joe Darcy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Respond to review feedback; make sequence of values explicit.
src/java.base/share/classes/java/lang/System.java line 740:
> 738: * href="https://jcp.org/en/procedures/jcp2#3.6.4";>maintenance
> 739: * release (optional).
> 740: * After a first maintenance release, this property will
Separate independent clauses with a semicolon: `After a first maintenance
release this property will have the value {@code "1"}; after a second
maintenance release it will have the value {@code "2"}, and so on.`
-
PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator [v3]
On Thu, 28 Apr 2022 19:48:18 GMT, Paul Sandoz wrote: > It should be possible for you finalize now. Done. Thanks @PaulSandoz . - PR: https://git.openjdk.java.net/jdk/pull/8291
java.net.http.HttpClient do NOT support Digest authentication while HttpURLConnection do
Hi,
Even though many tutorial said that the new java.net.http.HttpClient
support Digest authentication it's not true:
https://github.com/openjdk/jdk/blob/master/src/java.net.http/share/classes/jdk/internal/net/http/AuthenticationFilter.java#L278
But at the same time HttpURLConnection support it through the simple:
---
Authenticator.setDefault(new Authenticator() {
@Override
protected PasswordAuthentication getPasswordAuthentication() {
return new PasswordAuthentication (
"username",
"password".toCharArray()
);
}
});
---
Is it a bug or you don't even plan to support it with HttpClient?
Regards.
--
Levente "Si vis pacem para bellum!"
Re: RFR: 8285523: Improve test java/io/FileOutputStream/OpenNUL.java
On Thu, 28 Apr 2022 15:43:09 GMT, Andrew John Hughes wrote: >> The new test added as part of the >> [JDK-8285445](https://bugs.openjdk.java.net/browse/JDK-8285445) cannot >> trigger that bug and pass w/ and w/o fix. >> >> An updated test validates the "default" case when the >> `jdk.io.File.enableADS` property is not set, in this case the ADS should be >> [accepted](https://github.com/openjdk/jdk/blob/9d9f4e502f6ddc3116ed9b80f7168a1edfce839e/src/java.base/windows/classes/java/io/WinNTFileSystem.java#L59). > > test/jdk/java/io/FileOutputStream/OpenNUL.java line 31: > >> 29: * @run main/othervm OpenNUL >> 30: * @run main/othervm -Djdk.io.File.enableADS OpenNUL >> 31: * @run main/othervm -Djdk.io.File.enableADS=FalsE OpenNUL > > Is there a reason for the mixed case? Just to check equalsIgnoreCase is being > used? Yes only for equalsIgnoreCase - PR: https://git.openjdk.java.net/jdk/pull/8379
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v4]
On Thu, 28 Apr 2022 20:54:29 GMT, Joe Darcy wrote: >> Add a new system property, java.specification.maintenance.version, to return >> the maintenance release number of the Java SE specification being >> implemented. The property is unset, optional in the terminology of >> System.getProperties, for an initial release of a specification. >> >> Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8285764 >> >> I'll update copyright years before an integration. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to review feedback; make sequence of values explicit. Marked as reviewed by mullan (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v2]
On Thu, 28 Apr 2022 20:15:44 GMT, Joe Darcy wrote: > > Also, don't forget to update the CSR with the new specification text, for > > the record. Otherwise, looks good! > > Will do; I'll update the CSR to reflect that change as well as to be more > explicit about the sequence of values, once that is determined. CSR now updated accordingly. - PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v4]
> Add a new system property, java.specification.maintenance.version, to return > the maintenance release number of the Java SE specification being > implemented. The property is unset, optional in the terminology of > System.getProperties, for an initial release of a specification. > > Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8285764 > > I'll update copyright years before an integration. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Respond to review feedback; make sequence of values explicit. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8437/files - new: https://git.openjdk.java.net/jdk/pull/8437/files/fc7730d6..461407aa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8437&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8437&range=02-03 Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8437.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8437/head:pull/8437 PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available [v2]
On Thu, 28 Apr 2022 20:13:48 GMT, Uwe Schindler wrote: > By the way: FileOutputStream has exactly the same problem with > `write(byte[])`. I see no test for it, but I assume this is now also fixed. > That's a longstanding issue in Lucene (this is why we use a > ChunkedOutputStream when writing files. Yes, `write(byte[])` is also fixed: [io_util.c#L164](https://github.com/bplb/jdk/blob/40d46f8f4463dbd7dbe10651910826e90ca4dbdd/src/java.base/share/native/libjava/io_util.c#L164). - PR: https://git.openjdk.java.net/jdk/pull/8235
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v3]
On Thu, 28 Apr 2022 20:15:36 GMT, Joe Darcy wrote: >> Add a new system property, java.specification.maintenance.version, to return >> the maintenance release number of the Java SE specification being >> implemented. The property is unset, optional in the terminology of >> System.getProperties, for an initial release of a specification. >> >> Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8285764 >> >> I'll update copyright years before an integration. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to review feedback. Marked as reviewed by mr (Lead). - PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v2]
On Thu, 28 Apr 2022 20:07:41 GMT, Joe Darcy wrote: >> src/java.base/share/conf/security/java.policy line 34: >> >>> 32:"java.specification.version", "read"; >>> 33: permission java.util.PropertyPermission >>> 34:"java.specification.maintenance.version", "read"; >> >> For consistency, you should probably also add a hard-coded permission to the >> `sun.security.provider.PolicyFile.initStaticPolicy()` method which is used >> as a fallback if there is a problem parsing the `java.policy` file. I doubt >> this will cause any issues as I don't suspect many/any applications will >> suddenly start reading this property and running with an SM but again it is >> mainly for consistency as all the other default permissions in this file are >> also granted in that method. > > Will do; thanks. Just to double-check, under the current proposal, this > property will be undefined most of the time. I assume it is fine for the > permissions to grant the ability to read a property that is not actually > there. Yes. - PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v2]
On Thu, 28 Apr 2022 18:53:11 GMT, Mark Reinhold wrote: > Also, don't forget to update the CSR with the new specification text, for the > record. Otherwise, looks good! Will do; I'll update the CSR to reflect that change as well as to be more explicit about the sequence of values, once that is determined. - PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version
On Thu, 28 Apr 2022 10:24:46 GMT, Jaikiran Pai wrote: > Hello Joe, should the property description have a note stating what kind of a > value this property holds, if at all present? Would it be free form text or > an integer value? > There is some discussion about the sequence of values in comments on the JBS issue. I think it would be reasonable to have the sequence: undefined -> 1 -> 2 -> 3 ... where 1, 2, 3 are strings. I'll update the text to be more explicit about the eventual sequence once the discussion is concluded; thanks. - PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v2]
On Thu, 28 Apr 2022 19:37:00 GMT, Sean Mullan wrote: >> Joe Darcy has updated the pull request with a new target base due to a merge >> or a rebase. The incremental webrev excludes the unrelated changes brought >> in by the merge/rebase. The pull request contains five additional commits >> since the last revision: >> >> - Respond to review feedback. >> - Respond to CSR feedback. >> - Merge branch 'master' into JDK-8285497 >> - Update comment in template. >> - JDK-8285497: Add system property for Java SE specification maintenance >> version > > src/java.base/share/conf/security/java.policy line 34: > >> 32:"java.specification.version", "read"; >> 33: permission java.util.PropertyPermission >> 34:"java.specification.maintenance.version", "read"; > > For consistency, you should probably also add a hard-coded permission to the > `sun.security.provider.PolicyFile.initStaticPolicy()` method which is used as > a fallback if there is a problem parsing the `java.policy` file. I doubt this > will cause any issues as I don't suspect many/any applications will suddenly > start reading this property and running with an SM but again it is mainly for > consistency as all the other default permissions in this file are also > granted in that method. Will do; thanks. Just to double-check, under the current proposal, this property will be undefined most of the time. I assume it is fine for the permissions to grant the ability to read a property that is not actually there. - PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available [v2]
On Thu, 28 Apr 2022 20:02:36 GMT, Brian Burkhalter wrote: >> Modify native multi-byte read-write code used by the `java.io` classes to >> limit the size of the allocated native buffer thereby decreasing off-heap >> memory footprint and increasing throughput. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 6478546: Decrease malloc'ed buffer maximum size to 64 kB By the way: FileOutputStream has exactly the same problem with `write(byte[])`. I see no test for it, but I assume this is now also fixed. That's a longstanding issue in Lucene (this is why we use a ChunkedOutputStream when writing files. - PR: https://git.openjdk.java.net/jdk/pull/8235
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v3]
> Add a new system property, java.specification.maintenance.version, to return > the maintenance release number of the Java SE specification being > implemented. The property is unset, optional in the terminology of > System.getProperties, for an initial release of a specification. > > Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8285764 > > I'll update copyright years before an integration. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Respond to review feedback. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8437/files - new: https://git.openjdk.java.net/jdk/pull/8437/files/101ad872..fc7730d6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8437&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8437&range=01-02 Stats: 4 lines in 2 files changed: 3 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8437.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8437/head:pull/8437 PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available [v2]
> Modify native multi-byte read-write code used by the `java.io` classes to > limit the size of the allocated native buffer thereby decreasing off-heap > memory footprint and increasing throughput. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 6478546: Decrease malloc'ed buffer maximum size to 64 kB - Changes: - all: https://git.openjdk.java.net/jdk/pull/8235/files - new: https://git.openjdk.java.net/jdk/pull/8235/files/8bb1e14f..40d46f8f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8235&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8235&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8235.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8235/head:pull/8235 PR: https://git.openjdk.java.net/jdk/pull/8235
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator [v3]
On Wed, 27 Apr 2022 09:06:12 GMT, Jie Fu wrote: >> Hi all, >> >> The Current Vector API doc for `LSHR` is >> >> Produce a>>>(n&(ESIZE*8-1)). Integral only. >> >> >> This is misleading which may lead to bugs for Java developers. >> This is because for negative byte/short elements, the results computed by >> `LSHR` will be different from that of `>>>`. >> For more details, please see >> https://github.com/openjdk/jdk/pull/8276#issue-1206391831 . >> >> After the patch, the doc for `LSHR` is >> >> Produce zero-extended right shift of a by (n&(ESIZE*8-1)) bits. Integral >> only. >> >> >> Thanks. >> Best regards, >> Jie > > Jie Fu 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 six additional commits since the > last revision: > > - Address review comments > - Merge branch 'master' into JDK-8284992 > - Merge branch 'master' into JDK-8284992 > - Address review comments > - Merge branch 'master' into JDK-8284992 > - 8284992: Fix misleading Vector API doc for LSHR operator It should be possible for you finalize now. - PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: JDK-8285764: Add system property for Java SE specification maintenance version [v2]
On Thu, 28 Apr 2022 17:36:32 GMT, Joe Darcy wrote: >> Add a new system property, java.specification.maintenance.version, to return >> the maintenance release number of the Java SE specification being >> implemented. The property is unset, optional in the terminology of >> System.getProperties, for an initial release of a specification. >> >> Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8285764 >> >> I'll update copyright years before an integration. > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains five additional commits since > the last revision: > > - Respond to review feedback. > - Respond to CSR feedback. > - Merge branch 'master' into JDK-8285497 > - Update comment in template. > - JDK-8285497: Add system property for Java SE specification maintenance > version src/java.base/share/conf/security/java.policy line 34: > 32:"java.specification.version", "read"; > 33: permission java.util.PropertyPermission > 34:"java.specification.maintenance.version", "read"; For consistency, you should probably also add a hard-coded permission to the `sun.security.provider.PolicyFile.initStaticPolicy()` method which is used as a fallback if there is a problem parsing the `java.policy` file. I doubt this will cause any issues as I don't suspect many/any applications will suddenly start reading this property and running with an SM but again it is mainly for consistency as all the other default permissions in this file are also granted in that method. - PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which >> JDK version to target. >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e Marked as reviewed by egahlin (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v5]
On Thu, 28 Apr 2022 18:24:33 GMT, Andrey Turbanov wrote:
>> Joe Darcy has updated the pull request with a new target base due to a merge
>> or a rebase. The incremental webrev excludes the unrelated changes brought
>> in by the merge/rebase. The pull request contains seven additional commits
>> since the last revision:
>>
>> - Update copyright years.
>> - Merge branch 'master' into JDK-8285676
>> - Respond to more review feedback.
>> - Respond to more review feedback.
>> - Respond to review feedback.
>> - Merge branch 'master' into JDK-8285676
>> - JDK-8285676: Add missing @param tags for type parameters on classes and
>> interfaces
>
> src/java.base/share/classes/java/lang/ref/PhantomReference.java line 48:
>
>> 46: * The {@link #refersTo(Object) refersTo} method can be used to test
>> 47: * whether some object is the referent of a phantom reference.
>> 48: * @param the type of the referent
>
> Shouldn't there be a space after `@param` ?
Good catch. Sorry I missed it. This occurs in all `java/lang/ref` files.
-
PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: JDK-8285764: Add system property for Java SE specification maintenance version [v2]
On Thu, 28 Apr 2022 17:36:32 GMT, Joe Darcy wrote:
>> Add a new system property, java.specification.maintenance.version, to return
>> the maintenance release number of the Java SE specification being
>> implemented. The property is unset, optional in the terminology of
>> System.getProperties, for an initial release of a specification.
>>
>> Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8285764
>>
>> I'll update copyright years before an integration.
>
> Joe Darcy has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains five additional commits since
> the last revision:
>
> - Respond to review feedback.
> - Respond to CSR feedback.
> - Merge branch 'master' into JDK-8285497
> - Update comment in template.
> - JDK-8285497: Add system property for Java SE specification maintenance
> version
Also, don't forget to update the CSR with the new specification text, for the
record.
Otherwise, looks good!
src/java.base/share/classes/java/lang/VersionProps.java.template line 113:
> 111: props.put("java.specification.version", VERSION_SPECIFICATION);
> 112:
> 113: // Uncomment next props.put call after the first maintenance
> review for a
s/review/release/
-
Changes requested by mr (Lead).
PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v5]
On Thu, 28 Apr 2022 18:05:39 GMT, Joe Darcy wrote:
>> To enable more complete doclint checking (courtesy @jonathan-gibbons),
>> please review this PR to add type-level @param tags where they are missing.
>>
>> To the maintainers of java.util.concurrent, those changes could be separated
>> out in another bug if that would ease maintenance of that code.
>>
>> Making these library fixes is a blocker for correcting and expanding the
>> doclint checks (JDK-8285496).
>>
>> I'll update copyright years before pushing.
>
> Joe Darcy has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains seven additional commits since
> the last revision:
>
> - Update copyright years.
> - Merge branch 'master' into JDK-8285676
> - Respond to more review feedback.
> - Respond to more review feedback.
> - Respond to review feedback.
> - Merge branch 'master' into JDK-8285676
> - JDK-8285676: Add missing @param tags for type parameters on classes and
> interfaces
src/java.base/share/classes/java/lang/ref/PhantomReference.java line 48:
> 46: * The {@link #refersTo(Object) refersTo} method can be used to test
> 47: * whether some object is the referent of a phantom reference.
> 48: * @param the type of the referent
Shouldn't there be a space after `@param` ?
-
PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v34]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Downcall and upcall tests use wrong layout which is missing some trailing padding - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/5866bbb5..945317ec Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=33 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=32-33 Stats: 9 lines in 2 files changed: 7 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Integrated: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces
On Tue, 26 Apr 2022 22:24:26 GMT, Joe Darcy wrote: > To enable more complete doclint checking (courtesy @jonathan-gibbons), please > review this PR to add type-level @param tags where they are missing. > > To the maintainers of java.util.concurrent, those changes could be separated > out in another bug if that would ease maintenance of that code. > > Making these library fixes is a blocker for correcting and expanding the > doclint checks (JDK-8285496). > > I'll update copyright years before pushing. This pull request has now been integrated. Changeset: bba456a8 Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/bba456a8dbf9027e4b015567c17a79fc7441aa08 Stats: 102 lines in 40 files changed: 76 ins; 0 del; 26 mod 8285676: Add missing @param tags for type parameters on classes and interfaces Reviewed-by: wetmore, smarks, dfuchs, prr, alanb, mchung - PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v5]
> To enable more complete doclint checking (courtesy @jonathan-gibbons), please > review this PR to add type-level @param tags where they are missing. > > To the maintainers of java.util.concurrent, those changes could be separated > out in another bug if that would ease maintenance of that code. > > Making these library fixes is a blocker for correcting and expanding the > doclint checks (JDK-8285496). > > I'll update copyright years before pushing. Joe Darcy has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision: - Update copyright years. - Merge branch 'master' into JDK-8285676 - Respond to more review feedback. - Respond to more review feedback. - Respond to review feedback. - Merge branch 'master' into JDK-8285676 - JDK-8285676: Add missing @param tags for type parameters on classes and interfaces - Changes: - all: https://git.openjdk.java.net/jdk/pull/8410/files - new: https://git.openjdk.java.net/jdk/pull/8410/files/aaa8f828..cb1fe1c2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8410&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8410&range=03-04 Stats: 687 lines in 59 files changed: 610 ins; 8 del; 69 mod Patch: https://git.openjdk.java.net/jdk/pull/8410.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8410/head:pull/8410 PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: 8283667: [vectorapi] Vectorization for masked load with IOOBE with predicate feature [v2]
On Fri, 22 Apr 2022 07:08:24 GMT, Xiaohong Gong wrote:
>> Currently the vector load with mask when the given index happens out of the
>> array boundary is implemented with pure java scalar code to avoid the IOOBE
>> (IndexOutOfBoundaryException). This is necessary for architectures that do
>> not support the predicate feature. Because the masked load is implemented
>> with a full vector load and a vector blend applied on it. And a full vector
>> load will definitely cause the IOOBE which is not valid. However, for
>> architectures that support the predicate feature like SVE/AVX-512/RVV, it
>> can be vectorized with the predicated load instruction as long as the
>> indexes of the masked lanes are within the bounds of the array. For these
>> architectures, loading with unmasked lanes does not raise exception.
>>
>> This patch adds the vectorization support for the masked load with IOOBE
>> part. Please see the original java implementation (FIXME: optimize):
>>
>>
>> @ForceInline
>> public static
>> ByteVector fromArray(VectorSpecies species,
>>byte[] a, int offset,
>>VectorMask m) {
>> ByteSpecies vsp = (ByteSpecies) species;
>> if (offset >= 0 && offset <= (a.length - species.length())) {
>> return vsp.dummyVector().fromArray0(a, offset, m);
>> }
>>
>> // FIXME: optimize
>> checkMaskFromIndexSize(offset, vsp, m, 1, a.length);
>> return vsp.vOp(m, i -> a[offset + i]);
>> }
>>
>> Since it can only be vectorized with the predicate load, the hotspot must
>> check whether the current backend supports it and falls back to the java
>> scalar version if not. This is different from the normal masked vector load
>> that the compiler will generate a full vector load and a vector blend if the
>> predicate load is not supported. So to let the compiler make the expected
>> action, an additional flag (i.e. `usePred`) is added to the existing
>> "loadMasked" intrinsic, with the value "true" for the IOOBE part while
>> "false" for the normal load. And the compiler will fail to intrinsify if the
>> flag is "true" and the predicate load is not supported by the backend, which
>> means that normal java path will be executed.
>>
>> Also adds the same vectorization support for masked:
>> - fromByteArray/fromByteBuffer
>> - fromBooleanArray
>> - fromCharArray
>>
>> The performance for the new added benchmarks improve about `1.88x ~ 30.26x`
>> on the x86 AVX-512 system:
>>
>> Benchmark before After Units
>> LoadMaskedIOOBEBenchmark.byteLoadArrayMaskIOOBE 737.542 1387.069 ops/ms
>> LoadMaskedIOOBEBenchmark.doubleLoadArrayMaskIOOBE 118.366 330.776 ops/ms
>> LoadMaskedIOOBEBenchmark.floatLoadArrayMaskIOOBE 233.832 6125.026 ops/ms
>> LoadMaskedIOOBEBenchmark.intLoadArrayMaskIOOBE233.816 7075.923 ops/ms
>> LoadMaskedIOOBEBenchmark.longLoadArrayMaskIOOBE 119.771 330.587 ops/ms
>> LoadMaskedIOOBEBenchmark.shortLoadArrayMaskIOOBE 431.961 939.301 ops/ms
>>
>> Similar performance gain can also be observed on 512-bit SVE system.
>
> Xiaohong Gong has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Rename the "usePred" to "offsetInRange"
@PaulSandoz Could you please take a look at the Java changes when you find
time. This PR from @XiaohongGong is a very good step towards long standing
Vector API wish list for better tail loop handling.
-
PR: https://git.openjdk.java.net/jdk/pull/8035
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v4]
On Thu, 28 Apr 2022 16:58:40 GMT, Joe Darcy wrote: >> To enable more complete doclint checking (courtesy @jonathan-gibbons), >> please review this PR to add type-level @param tags where they are missing. >> >> To the maintainers of java.util.concurrent, those changes could be separated >> out in another bug if that would ease maintenance of that code. >> >> Making these library fixes is a blocker for correcting and expanding the >> doclint checks (JDK-8285496). >> >> I'll update copyright years before pushing. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to more review feedback. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: JDK-8285764: Add system property for Java SE specification maintenance version
On Thu, 28 Apr 2022 12:31:31 GMT, Sean Mullan wrote: > Should this be added to the default permissions in the > `conf/security/java.policy` file along with other similar properties? Seems reasonable; good catch. - PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: JDK-8285764: Add system property for Java SE specification maintenance version [v2]
> Add a new system property, java.specification.maintenance.version, to return > the maintenance release number of the Java SE specification being > implemented. The property is unset, optional in the terminology of > System.getProperties, for an initial release of a specification. > > Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8285764 > > I'll update copyright years before an integration. Joe Darcy has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Respond to review feedback. - Respond to CSR feedback. - Merge branch 'master' into JDK-8285497 - Update comment in template. - JDK-8285497: Add system property for Java SE specification maintenance version - Changes: - all: https://git.openjdk.java.net/jdk/pull/8437/files - new: https://git.openjdk.java.net/jdk/pull/8437/files/4ea4708b..101ad872 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8437&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8437&range=00-01 Stats: 670 lines in 38 files changed: 614 ins; 8 del; 48 mod Patch: https://git.openjdk.java.net/jdk/pull/8437.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8437/head:pull/8437 PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v4]
On Thu, 28 Apr 2022 16:58:40 GMT, Joe Darcy wrote: >> To enable more complete doclint checking (courtesy @jonathan-gibbons), >> please review this PR to add type-level @param tags where they are missing. >> >> To the maintainers of java.util.concurrent, those changes could be separated >> out in another bug if that would ease maintenance of that code. >> >> Making these library fixes is a blocker for correcting and expanding the >> doclint checks (JDK-8285496). >> >> I'll update copyright years before pushing. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to more review feedback. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v3]
On Thu, 28 Apr 2022 08:10:38 GMT, Alan Bateman wrote: >> Joe Darcy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Respond to more review feedback. > > src/java.base/share/classes/java/nio/file/WatchEvent.java line 51: > >> 49: /** >> 50: * An event kind, for the purposes of identification. >> 51: * @param the type of the context value > > This is okay but the it differs slightly to the type parameters specified > further up. I think the issue here is that it was just wasn't copied down to > WatchEvent.Kind. Okay -- I'll for the T type parameter of the Kind interface I'll reuse the wording of the T type parameter of the enclosing WatchEvent interface. (The type variable on Kind could be renamed to show that the two type parameters are distinct.) - PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v4]
> To enable more complete doclint checking (courtesy @jonathan-gibbons), please > review this PR to add type-level @param tags where they are missing. > > To the maintainers of java.util.concurrent, those changes could be separated > out in another bug if that would ease maintenance of that code. > > Making these library fixes is a blocker for correcting and expanding the > doclint checks (JDK-8285496). > > I'll update copyright years before pushing. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Respond to more review feedback. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8410/files - new: https://git.openjdk.java.net/jdk/pull/8410/files/db4919a9..aaa8f828 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8410&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8410&range=02-03 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8410.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8410/head:pull/8410 PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v3]
On Thu, 28 Apr 2022 08:08:37 GMT, Alan Bateman wrote: >> Joe Darcy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Respond to more review feedback. > > src/java.base/share/classes/java/nio/file/SecureDirectoryStream.java line 55: > >> 53: * against the original path of the directory (irrespective of if >> the >> 54: * directory is moved since it was opened). >> 55: * @param the type of path > > It may not be a path. The type parameter is specified in the super > interfaces, can you copy that down instead? Will change in the next push. - PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: 8285523: Improve test java/io/FileOutputStream/OpenNUL.java
On Mon, 25 Apr 2022 04:35:13 GMT, Sergey Bylokhov wrote: > The new test added as part of the > [JDK-8285445](https://bugs.openjdk.java.net/browse/JDK-8285445) cannot > trigger that bug and pass w/ and w/o fix. > > An updated test validates the "default" case when the `jdk.io.File.enableADS` > property is not set, in this case the ADS should be > [accepted](https://github.com/openjdk/jdk/blob/9d9f4e502f6ddc3116ed9b80f7168a1edfce839e/src/java.base/windows/classes/java/io/WinNTFileSystem.java#L59). Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8379
Re: RFR: 8285523: Improve test java/io/FileOutputStream/OpenNUL.java
On Mon, 25 Apr 2022 04:35:13 GMT, Sergey Bylokhov wrote: > The new test added as part of the > [JDK-8285445](https://bugs.openjdk.java.net/browse/JDK-8285445) cannot > trigger that bug and pass w/ and w/o fix. > > An updated test validates the "default" case when the `jdk.io.File.enableADS` > property is not set, in this case the ADS should be > [accepted](https://github.com/openjdk/jdk/blob/9d9f4e502f6ddc3116ed9b80f7168a1edfce839e/src/java.base/windows/classes/java/io/WinNTFileSystem.java#L59). Changes look right to me. The test seems to have been based on the situation pre-patch, where setting the property to `true` worked around the bug. With the patch, only the `false` case actually exhibits the bug. test/jdk/java/io/FileOutputStream/OpenNUL.java line 31: > 29: * @run main/othervm OpenNUL > 30: * @run main/othervm -Djdk.io.File.enableADS OpenNUL > 31: * @run main/othervm -Djdk.io.File.enableADS=FalsE OpenNUL Is there a reason for the mixed case? Just to check equalsIgnoreCase is being used? - Marked as reviewed by andrew (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8379
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Thu, 28 Apr 2022 15:10:18 GMT, Markus Grönlund wrote:
>> src/jdk.jfr/share/classes/jdk/jfr/events/VirtualThreadSubmitFailedEvent.java
>> line 35:
>>
>>> 33:
>>> 34: @Category({"Java Runtime"})
>>> 35: @Label("Virtual thread submit task failed")
>>
>> The label is a bit a long, would "Virtual Thread Submit Failed" work?
>
> It works. Thanks.
Yes, this is okay.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Thu, 28 Apr 2022 14:44:05 GMT, Erik Gahlin wrote:
>> Alan Bateman has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e
>
> src/hotspot/share/jfr/leakprofiler/sampling/objectSampler.cpp line 168:
>
>> 166: assert(!tl->is_excluded(), "invariant");
>> 167: if (virtual_thread) {
>> 168: // TODO: blob cache for virtual threads
>
> Fix this now or after integration?
Well spotted. It is an optimization and can happen after integration. I will
create an enhancement for tracking; thank you.
> src/hotspot/share/jfr/metadata/metadata.xml line 121:
>
>> 119: thread="true" stackTrace="true">
>> 120: > description="Thread enlisted as a carrier" />
>> 121: > description="Class of the continuation" />
>
> "Continuation class" = >" Continuation Class"
> numFrames = frames
> numRefs = references
> "Number of interpreted frames" => "Interpreted Frames"
> "Number of references" => "References"
> "Stack size in bytes" => "Stack Size" contentType"bytes"
Will fix.
> src/hotspot/share/jfr/metadata/metadata.xml line 130:
>
>> 128: thread="true" stackTrace="true">
>> 129: > description="Thread enlisted as a carrier" />
>> 130: > description="Class of the continuation" />
>
> contClass => continuationClass
> "Continuation class" => "Continuation Class"
> "Class of the continuation" Remove (not needed)
> "Number of interpreted frames" => "Interpreted Frames"
> numFrames => frames
> "Number of references" => "References"
> numRefs => references
> "Stack size in bytes" => "Stack Size" contentType="bytes"
Will fix.
> src/hotspot/share/jfr/metadata/metadata.xml line 138:
>
>> 136: > category="Java Virtual Machine" label="Continuation Freeze Young"
>> thread="true" stackTrace="false" startTime="false">
>> 137:
>> 138:
>
> "Allocated new" => "Allocated New"
Will fix, thanks.
> src/jdk.jfr/share/classes/jdk/jfr/events/VirtualThreadEndEvent.java line 35:
>
>> 33:
>> 34: @Category({"Java Runtime"})
>> 35: @Label("Virtual thread end")
>
> "Virtual thread end" => "Virtual Thread End"
> Remove description.
Will fix.
> src/jdk.jfr/share/classes/jdk/jfr/events/VirtualThreadPinnedEvent.java line
> 35:
>
>> 33:
>> 34: @Category({"Java Runtime"})
>> 35: @Label("Virtual thread pinned")
>
> "Virtual thread pinned" => "Virtual Thread Pinned"
> Remove description
Will fix.
> src/jdk.jfr/share/classes/jdk/jfr/events/VirtualThreadStartEvent.java line 35:
>
>> 33:
>> 34: @Category({"Java Runtime"})
>> 35: @Label("Virtual thread start")
>
> "virtual thread start" => "Virtual Thread Start"
> Remove description
Will fix, thanks.
> src/jdk.jfr/share/classes/jdk/jfr/events/VirtualThreadSubmitFailedEvent.java
> line 35:
>
>> 33:
>> 34: @Category({"Java Runtime"})
>> 35: @Label("Virtual thread submit task failed")
>
> The label is a bit a long, would "Virtual Thread Submit Failed" work?
It works. Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8285690: CloneableReference subtest should not throw CloneNotSupportedException [v2]
On Wed, 27 Apr 2022 15:32:35 GMT, Roger Riggs wrote: >> Kim Barrett has updated the pull request incrementally with one additional >> commit since the last revision: >> >> update copyright, @bug list > > LGTM Thanks for reviews @RogerRiggs and @mlchung - PR: https://git.openjdk.java.net/jdk/pull/8418
Integrated: 8285690: CloneableReference subtest should not throw CloneNotSupportedException
On Wed, 27 Apr 2022 09:24:24 GMT, Kim Barrett wrote: > Please review this fix to test/jdk/java/lang/ref/ReferenceClone.java. It was > passing if CloneableReference::clone were to throw CloneNotSupportedException. > That should be a failure. > > Testing: > Locally (linux-x64) verified test still passes. Temporarily changed > CloneableReference::clone to throw and verified the expected failure gets > reported, unlike before. This pull request has now been integrated. Changeset: 2d8d1402 Author:Kim Barrett URL: https://git.openjdk.java.net/jdk/commit/2d8d1402147f6ddd15732ce7098a8438317a2681 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod 8285690: CloneableReference subtest should not throw CloneNotSupportedException Reviewed-by: rriggs, mchung - PR: https://git.openjdk.java.net/jdk/pull/8418
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Thu, 28 Apr 2022 14:41:04 GMT, Erik Gahlin wrote: >> Alan Bateman has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e > > src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.cpp line 94: > >> 92: static const size_t global_buffer_size = 512 * K; >> 93: >> 94: static const size_t thread_local_buffer_prealloc_count = 32; > > Why is more memory needed? Moore's law or something specific to virtual > threads? The carrier thread will write the metadata for the mounted virtual threads lazily with virtual threads as part of the event write. They write this thread locally, and the memory increase accommodates fewer roundtrips to fetch new buffers. The previous size was small because it would only hold at most one entry. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Thu, 28 Apr 2022 14:37:22 GMT, Erik Gahlin wrote: >> Alan Bateman has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e > > src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointWriter.cpp line 81: > >> 79: be_writer.write(size); >> 80: be_writer.write(time); >> 81: be_writer.write(JfrTicks::now().value() - time); > > Why is this changed? It was a small optimization attempted before the current system of writing virtual thread checkpoints in bulk. Will restore it, thank you. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v3]
On Fri, 22 Apr 2022 14:35:14 GMT, Sibabrata Sahoo wrote: >> A new API to support replacing selective lines with desired content. > > Sibabrata Sahoo has updated the pull request incrementally with one > additional commit since the last revision: > > Update FileUtils.java I will do a small change in method signature patch(Path path, int fromLine, int toLine, List from, String to) where the argument "List from" to be "String from". Also a new open Test will be added into this change-set. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e
src/hotspot/share/jfr/leakprofiler/sampling/objectSampler.cpp line 168:
> 166: assert(!tl->is_excluded(), "invariant");
> 167: if (virtual_thread) {
> 168: // TODO: blob cache for virtual threads
Fix this now or after integration?
src/hotspot/share/jfr/metadata/metadata.xml line 121:
> 119: thread="true" stackTrace="true">
> 120: description="Thread enlisted as a carrier" />
> 121: description="Class of the continuation" />
"Continuation class" = >" Continuation Class"
numFrames = frames
numRefs = references
"Number of interpreted frames" => "Interpreted Frames"
"Number of references" => "References"
"Stack size in bytes" => "Stack Size" contentType"bytes"
src/hotspot/share/jfr/metadata/metadata.xml line 130:
> 128: thread="true" stackTrace="true">
> 129: description="Thread enlisted as a carrier" />
> 130: description="Class of the continuation" />
contClass => continuationClass
"Continuation class" => "Continuation Class"
"Class of the continuation" Remove (not needed)
"Number of interpreted frames" => "Interpreted Frames"
numFrames => frames
"Number of references" => "References"
numRefs => references
"Stack size in bytes" => "Stack Size" contentType="bytes"
src/hotspot/share/jfr/metadata/metadata.xml line 138:
> 136:category="Java Virtual Machine" label="Continuation Freeze Young"
> thread="true" stackTrace="false" startTime="false">
> 137:
> 138:
"Allocated new" => "Allocated New"
src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.cpp line 94:
> 92: static const size_t global_buffer_size = 512 * K;
> 93:
> 94: static const size_t thread_local_buffer_prealloc_count = 32;
Why is more memory needed? Moore's law or something specific to virtual threads?
src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointWriter.cpp line 81:
> 79: be_writer.write(size);
> 80: be_writer.write(time);
> 81: be_writer.write(JfrTicks::now().value() - time);
Why is this changed?
src/jdk.jfr/share/classes/jdk/jfr/events/VirtualThreadEndEvent.java line 35:
> 33:
> 34: @Category({"Java Runtime"})
> 35: @Label("Virtual thread end")
"Virtual thread end" => "Virtual Thread End"
Remove description.
src/jdk.jfr/share/classes/jdk/jfr/events/VirtualThreadPinnedEvent.java line 35:
> 33:
> 34: @Category({"Java Runtime"})
> 35: @Label("Virtual thread pinned")
"Virtual thread pinned" => "Virtual Thread Pinned"
Remove description
src/jdk.jfr/share/classes/jdk/jfr/events/VirtualThreadStartEvent.java line 35:
> 33:
> 34: @Category({"Java Runtime"})
> 35: @Label("Virtual thread start")
"virtual thread start" => "Virtual Thread Start"
Remove description
src/jdk.jfr/share/classes/jdk/jfr/events/VirtualThreadSubmitFailedEvent.java
line 35:
> 33:
> 34: @Category({"Java Runtime"})
> 35: @Label("Virtual thread submit task failed")
The label is a bit a long, would "Virtual Thread Submit Failed" work?
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8282274: Compiler implementation for Pattern Matching for switch (Third Preview) [v8]
> This is a (preliminary) patch for javac implementation for the third preview > of pattern matching for switch (type patterns in switches). > > Draft JLS: > http://cr.openjdk.java.net/~gbierman/PatternSwitchPlusRecordPatterns/PatternSwitchPlusRecordPatterns-20220407/specs/patterns-switch-jls.html > > The changes are: > -there are no guarded patterns anymore, guards are not bound to the > CaseElement (JLS 15.28) > -a new contextual keyword `when` is used to add a guard, instead of `&&` > -`null` selector value is handled on switch level (if a switch has `case > null`, it is used, otherwise a NPE is thrown), rather than on pattern > matching level. > -total patterns are allowed in `instanceof` > -`java.lang.MatchException` is added for the case where a switch is > exhaustive (due to sealed types) at compile-time, but not at runtime. > > Feedback is welcome! > > Thanks! Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 20 commits: - Reducing MatchException constructors. - Merge branch 'master' into type-patterns-third - Reference-type pattern is not applicable at a selector of a primitive type - fixing. - Merge branch 'master' into type-patterns-third - Cleanup, fixing tests. - Cleanup - more total -> unconditional pattern renaming. - Adjusting to review feedback. - Cleanup. - Fixing tests. - Adding a test for a NPE from guards. - ... and 10 more: https://git.openjdk.java.net/jdk/compare/4919525d...115605ae - Changes: https://git.openjdk.java.net/jdk/pull/8182/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8182&range=07 Stats: 860 lines in 52 files changed: 424 ins; 244 del; 192 mod Patch: https://git.openjdk.java.net/jdk/pull/8182.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8182/head:pull/8182 PR: https://git.openjdk.java.net/jdk/pull/8182
Re: RFR: 8285295: Need better testing for IdentityHashMap
On Fri, 22 Apr 2022 03:37:27 GMT, Stuart Marks wrote:
> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch
> in the bug report that breaks `IdentityHashMap` now causes several cases in
> this new test to fail. There's more that could be done, but the new tests
> cover most of the core functions of `IdentityHashMap`. Unfortunately it seems
> difficult to merge this with the existing, comprehensive Collections tests
> (e.g., MOAT.java) because those tests implicity rely on `equals()`-based
> contract instead of the special-purpose `==`-based contract used by
> `IdentityHashMap`.
test/jdk/java/util/IdentityHashMap/Basic.java line 337:
> 335: public void testPutOverwrite() {
> 336: Box newVal = new Box(v1a);
> 337: map.put(k1a, newVal);
Should we capture the return value and assert that it's not null and is
identity equal to `v1a`?
Perhaps, similarly at a few other places where we do put and putIfAbsent?
-
PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap
On Fri, 22 Apr 2022 03:37:27 GMT, Stuart Marks wrote:
> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch
> in the bug report that breaks `IdentityHashMap` now causes several cases in
> this new test to fail. There's more that could be done, but the new tests
> cover most of the core functions of `IdentityHashMap`. Unfortunately it seems
> difficult to merge this with the existing, comprehensive Collections tests
> (e.g., MOAT.java) because those tests implicity rely on `equals()`-based
> contract instead of the special-purpose `==`-based contract used by
> `IdentityHashMap`.
test/jdk/java/util/IdentityHashMap/Basic.java line 244:
> 242: public void testKeySetNoRemove() {
> 243: Set keySet = map.keySet();
> 244: keySet.remove(new Box(k1a));
Should we assert here that this returns `false`?
test/jdk/java/util/IdentityHashMap/Basic.java line 253:
> 251: public void testKeySetRemove() {
> 252: Set keySet = map.keySet();
> 253: keySet.remove(k1a);
Similarly should we assert `true` here and few other places in this PR which
does the remove?
test/jdk/java/util/IdentityHashMap/Basic.java line 257:
> 255: checkEntries(map.entrySet(), entry(k1b, v1b),
> 256: entry(k2, v2));
> 257: }
Would an additional check `assertFalse(map.equals(map2));` be useful here (and
other similar tests where we do "remove").
test/jdk/java/util/IdentityHashMap/Basic.java line 325:
> 323: Box newKey = new Box(k1a);
> 324: Box newVal = new Box(v1a);
> 325: map.put(newKey, newVal);
Should we capture the return value and assert that it is `null`?
-
PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: JDK-8285730: unify _WIN32_WINNT settings
On Thu, 28 Apr 2022 07:16:30 GMT, Matthias Baesken wrote: > Hi Erik/David/Phil, we already have a good central place where we set the > definition of WIN32_LEAN_AND_MEAN > > autoconf/flags-cflags.m4:460: ALWAYS_DEFINES_JDK="-DWIN32_LEAN_AND_MEAN > -D_CRT_SECURE_NO_DEPRECATE autoconf/flags-cflags.m4:462: > ALWAYS_DEFINES_JVM="-DNOMINMAX -DWIN32_LEAN_AND_MEAN" > > should we add there the _WIN32_WINNT=0x0601 define (and remove the differing > defines instead at the other places) ? (defines of _WIN32_WINNT in 3rd party > code would stay like wepoll.c and harfbuzz). Seems reasonable to me at least. - PR: https://git.openjdk.java.net/jdk/pull/8428
Re: RFR: JDK-8285764: Add system property for Java SE specification maintenance version
On Wed, 27 Apr 2022 22:27:34 GMT, Joe Darcy wrote: > Add a new system property, java.specification.maintenance.version, to return > the maintenance release number of the Java SE specification being > implemented. The property is unset, optional in the terminology of > System.getProperties, for an initial release of a specification. > > Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8285764 > > I'll update copyright years before an integration. Should this be added to the default permissions in the `lib/security/java.policy` file along with other similar properties? - PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: 8285295: Need better testing for IdentityHashMap
On Wed, 27 Apr 2022 03:28:09 GMT, Stuart Marks wrote: >> test/jdk/java/util/IdentityHashMap/Basic.java line 77: >> >>> 75: E[] contents = (E[]) c.toArray(); >>> 76: >>> 77: assertEquals(c.size(), given.length); >> >> I believe testng has the expected values in front in the `assertEquals` >> methods, as embodied in the exception messages, so this should be >> `assertEquals(given.length, c.size());`. Applies to other places. > > No, TestNG is `assertEquals(actual, expected)` which is irritatingly the > opposite of JUnit. > > https://github.com/cbeust/testng/blob/master/testng-asserts/src/main/java/org/testng/asserts/Assertion.java#L151 > > This will make things quite tedious when/if we convert to JUnit. There’s a reason why I prefer using [AssertJ], where the calls are: Assertions.assertThat(actual) .isEqualTo(expected); [AssertJ]: https://assertj.github.io/doc - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: JDK-8285764: Add system property for Java SE specification maintenance version
On Wed, 27 Apr 2022 22:27:34 GMT, Joe Darcy wrote: > Add a new system property, java.specification.maintenance.version, to return > the maintenance release number of the Java SE specification being > implemented. The property is unset, optional in the terminology of > System.getProperties, for an initial release of a specification. > > Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8285764 > > I'll update copyright years before an integration. Hello Joe, should the property description have a note stating what kind of a value this property holds, if at all present? Would it be free form text or an integer value? - PR: https://git.openjdk.java.net/jdk/pull/8437
Integrated: 8283994: Make Xerces DatatypeException stackless
On Wed, 30 Mar 2022 11:38:59 GMT, Aleksey Shipilev wrote: > See bug report for more details. This change improves > SPECjvm2008:xml.validation for about +3%: > > > baseline: 298.353 ± 1.008 ops/min > patched: 309.912 ± 1.347 ops/min > > Of course, the real improvements might be even higher, as exception might be > thrown from much deeper call hierarchy. > > Additional testing: > - [x] Linux x86_64 fastdebug, `jaxp_all` > - [x] Linux x86_64 fastdebug, `javax/xml` This pull request has now been integrated. Changeset: 85f8d14e Author:Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/85f8d14edf0128e94bfc8102619a6ddbc37ead70 Stats: 8 lines in 1 file changed: 6 ins; 0 del; 2 mod 8283994: Make Xerces DatatypeException stackless Reviewed-by: joehw, jpai - PR: https://git.openjdk.java.net/jdk/pull/8036
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v3]
On Thu, 28 Apr 2022 01:34:19 GMT, Joe Darcy wrote: >> To enable more complete doclint checking (courtesy @jonathan-gibbons), >> please review this PR to add type-level @param tags where they are missing. >> >> To the maintainers of java.util.concurrent, those changes could be separated >> out in another bug if that would ease maintenance of that code. >> >> Making these library fixes is a blocker for correcting and expanding the >> doclint checks (JDK-8285496). >> >> I'll update copyright years before pushing. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to more review feedback. src/java.base/share/classes/java/nio/file/SecureDirectoryStream.java line 55: > 53: * against the original path of the directory (irrespective of if > the > 54: * directory is moved since it was opened). > 55: * @param the type of path It may not be a path. The type parameter is specified in the super interfaces, can you copy that down instead? src/java.base/share/classes/java/nio/file/WatchEvent.java line 51: > 49: /** > 50: * An event kind, for the purposes of identification. > 51: * @param the type of the context value This is okay but the it differs slightly to the type parameters specified further up. I think the issue here is that it was just wasn't copied down to WatchEvent.Kind. - PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v3]
On Thu, 28 Apr 2022 01:34:19 GMT, Joe Darcy wrote: >> To enable more complete doclint checking (courtesy @jonathan-gibbons), >> please review this PR to add type-level @param tags where they are missing. >> >> To the maintainers of java.util.concurrent, those changes could be separated >> out in another bug if that would ease maintenance of that code. >> >> Making these library fixes is a blocker for correcting and expanding the >> doclint checks (JDK-8285496). >> >> I'll update copyright years before pushing. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to more review feedback. Thanks for the updates Joe. Your new wording looks good to me. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 19:43:22 GMT, Paul Sandoz wrote:
>> Alan Bateman has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e
>
> src/java.base/share/classes/java/util/concurrent/ThreadPerTaskExecutor.java
> line 45:
>
>> 43: * threads is unbounded.
>> 44: */
>> 45: class ThreadPerTaskExecutor implements ExecutorService {
>
> This class manages the set of per-task threads which arguably should be the
> job of the thread container, and it awkwardly overrides the container's set
> of threads by setting a field on `SharedThreadContainer.threadsSupplier`.
>
> `SharedThreadContainer` supports a number of different shared container
> policies that could be made clearer.
>
> Architecturally i think this could be better layered but it can be iterated
> on later.
Indeed, the layering is a bit awkward and I plan to replace it. It came about
as a short term solution to avoid double bookkeeping of virtual threads.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 19:21:58 GMT, Alan Bateman wrote: >> src/java.base/share/classes/jdk/internal/misc/UnsafeConstants.java line 122: >> >>> 120: */ >>> 121: >>> 122: public static final int SCOPED_CACHE_SHIFT; >> >> I can't find this constant being used. If added for future, maybe keep >> `UnsafeConstants` class and this static field package-private for now. > > It originally configured the number of lines in extent local cache but the > implementation has changed. @theRealAph would be best to comment on this, it > may be possible to delete it. Yes, it's dead. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: JDK-8285730: unify _WIN32_WINNT settings
On Wed, 27 Apr 2022 14:57:41 GMT, Matthias Baesken wrote: > Currently we set _WIN32_WINNT at various places in the codebase; this is used > to target a minimum Windows version we want to support. See also for more > detailled information : > https://docs.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers?redirectedfrom=MSDN#setting-winver-or-_win32_winnt > Macros for Conditional Declarations > "Certain functions that depend on a particular version of Windows are > declared using conditional code. This enables you to use the compiler to > detect whether your application uses functions that are not supported on its > target version(s) of Windows." > > However currently we have quite a lot of differing settings of _WIN32_WINNT > in the codebase ; setting _WIN32_WINNT to 0x0601 (Windows 7) where possible > would make sense because we have this setting already at java_props_md.c > (so targeting older Windows versions at other places is most likely not > useful). Hi Erik/David/Phil, we already have a good central place where we set the definition of WIN32_LEAN_AND_MEAN autoconf/flags-cflags.m4:460:ALWAYS_DEFINES_JDK="-DWIN32_LEAN_AND_MEAN -D_CRT_SECURE_NO_DEPRECATE \ autoconf/flags-cflags.m4:462:ALWAYS_DEFINES_JVM="-DNOMINMAX -DWIN32_LEAN_AND_MEAN" should we add there the _WIN32_WINNT=0x0601 define (and remove the differing defines instead at the other places) ? (defines of _WIN32_WINNT in 3rd party code would stay like wepoll.c and harfbuzz). - PR: https://git.openjdk.java.net/jdk/pull/8428
Re: RFR: JDK-8285730: unify _WIN32_WINNT settings
On Wed, 27 Apr 2022 15:10:51 GMT, Alan Bateman wrote: >> Currently we set _WIN32_WINNT at various places in the codebase; this is >> used to target a minimum Windows version we want to support. See also for >> more detailled information : >> https://docs.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers?redirectedfrom=MSDN#setting-winver-or-_win32_winnt >> Macros for Conditional Declarations >> "Certain functions that depend on a particular version of Windows are >> declared using conditional code. This enables you to use the compiler to >> detect whether your application uses functions that are not supported on its >> target version(s) of Windows." >> >> However currently we have quite a lot of differing settings of _WIN32_WINNT >> in the codebase ; setting _WIN32_WINNT to 0x0601 (Windows 7) where possible >> would make sense because we have this setting already at java_props_md.c >> (so targeting older Windows versions at other places is most likely not >> useful). > > src/java.base/windows/native/libnio/ch/wepoll.c line 159: > >> 157: >> 158: #undef _WIN32_WINNT >> 159: #define _WIN32_WINNT 0x0601 > > This is 3rd party code and would prefer not change it if possible. Hi Alan, I agree (thats why I did not change the define in harfbuzz, but I missed that wepoll.c is 3rd party code as well). - PR: https://git.openjdk.java.net/jdk/pull/8428
