Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v11]
On Wed, 4 May 2022 12:12:48 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview). >> >> 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 >> can add additional labels, if required, as the PR progresses. >> >> 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 Extent 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 with a new target base due to a > merge or a rebase. The pull request now contains 17 commits: > > - Refresh d77f7a49af75bcc5b20686805ff82a93a20dde05 > - Merge with 4b2c82200fdc01de868cf414e40a4d891e753f89 > - Refresh 6091080db743ece5f1b2111fcc35a5f2179a403a > - Merge with cfcba1fccc8e3e6a68e1cb1826b70e076d5d83c4 > - Refresh ee9fa8ed05ec22de7a13383052d68aa8aa7832ec > - Merge with jdk-19+20 > - Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e > - Refresh 8d8f0a2fd646e57fe6b4e8ab669f836dc46dda69 > - Refresh cf561073f48fad58e931a5285b92629aa83c9bd1 > - Merge with jdk-19+19 > - ... and 7 more: > https://git.openjdk.java.net/jdk/compare/4b2c8220...f06aff75 I've completed review of new vthread-related tests in the folder serviceability/jvmti. It includes sub-folders: serviceability/jvmti/events serviceability/jvmti/negative serviceability/jvmti/thread Thank you, Leonid, for resolving my comments! - 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 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: 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: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 19:00:52 GMT, Coleen Phillimore wrote: >> Alan Bateman has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e > > src/hotspot/share/prims/jvmtiEnvBase.cpp line 2388: > >> 2386: } >> 2387: >> 2388: void > > @sspitsyn We should clean up this aberrant style of putting the return type > on the line before the rest of the function declaration after this > integration. It looks so strange. I noticed that it's pre-existing for > other functions in this file. I'm okay to follow any style. We can do it after the integration. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v7]
On Tue, 26 Apr 2022 17:27:35 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 8d8f0a2fd646e57fe6b4e8ab669f836dc46dda69 I've reviewed JDI, JDB and JDWP changes. All look good. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 11:22:48 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on modules owned by the serviceability team > (`java.instrument java.management.rmi java.management jdk.attach > jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi > jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and accepted > those changes where it indeed discovered real typos. > > > I will update copyright years using a script before pushing (otherwise like > every second change would be a copyright update, making reviewing much > harder). > > The long term goal here is to make tooling support for running `codespell`. > The trouble with automating this is of course all false positives. But before > even trying to solve that issue, all true positives must be fixed. Hence this > PR. Hi Magnus, Great job, thank you for doing this! It looks good to me. These findings are pretty interesting. Some are funny and some seems to be typical. Good examples of typical typos are: - double letter instead of single - single or even triple letter instead of double Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8281585: Remove unused imports under test/lib and jtreg/gc [v2]
On Fri, 11 Feb 2022 08:54:51 GMT, Leo Korinth wrote: >> Remove unused imports under test/lib and jtreg/gc. They create lots of >> warnings if editing using an IDE. Tests in hotspot_gc passed. > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > updating copyright Looks good. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7426
Re: RFR: 8280889: java/lang/instrument/GetObjectSizeIntrinsicsTest.java fails with -XX:-UseCompressedOops
On Fri, 28 Jan 2022 15:37:59 GMT, Aleksey Shipilev wrote: > Recent test regression after adding new cases in the test. Without compressed > oops, ~1G elements `Object[]` array takes >8G of memory, which fails the > test. The fix cuts it down to 512M when reference size is 8 bytes. > Additionally, `ObjectAlignmentInBytes=32` is excessive for new test configs. > > Additional testing: > - [x] Linux x86_64 fastdebug, default, affected test still passes > - [x] Linux x86_32 fastdebug, default, affected test still passes > - [x] Linux x86_64 fastdebug, `-XX:-UseCompressedOops`, affected test now > passes Looks good to me. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7269
Re: RFR: 8280166: Extend java/lang/instrument/GetObjectSizeIntrinsicsTest.java test cases
On Tue, 18 Jan 2022 18:33:29 GMT, Aleksey Shipilev wrote: > While working on JDK-8280003, I noticed that > java/lang/instrument/GetObjectSizeIntrinsicsTest.java does not test arrays > with more than 1-byte size elements, and no large arrays (past 4G limit) are > tested either. It would be better to add those test cases. > > Additional testing: > - [x] Linux x86_64 fastdebug, affected test still passes > - [x] Linux x86_32 fastdebug, affected test still passes > - [x] Linux AArch64 fastdebug, affected test still passes > - [x] Linux PPC64 fastdebug, affected test still passes Looks okay to me. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7132
Re: RFR: 8280010: Remove double buffering of InputStream for Properties.load
On Mon, 10 Jan 2022 20:46:36 GMT, Andrey Turbanov wrote: > `Properties.load` uses `java.util.Properties.LineReader`. LineReader already > buffers input stream. Hence wrapping InputStream in BufferedInputStream is > redundant. Marked as reviewed by sspitsyn (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7021
Re: RFR: 8279918: Fix various doc typos [v2]
On Thu, 13 Jan 2022 14:01:04 GMT, Pavel Rappo wrote: >> - Most of the typos are of a trivial kind: missing whitespace. >> - If any of the typos should be fixed in the upstream projects instead, >> please say so; I will drop those typos from the patch. >> - As I understand it, ` ` in ImageInputStream and DataInput is an irrelevant >> formatting artefact and thus can be removed. >> - `` is an apostrophe, which does not require to be encoded. > > Pavel Rappo has updated the pull request incrementally with one additional > commit since the last revision: > > Additional typos Hi Pavel, Looks good. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: JDK-8274930: sun/tools/jps/TestJps.java can fail with long VM arguments string
On Thu, 7 Oct 2021 21:46:47 GMT, Alex Menkov wrote: > The fix adds "-XX:PerfMaxStringConstLength" argument running target app > (default is 1024, 8K should be enough for any environments) LGTM. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5858
Re: RFR: 8273187: jtools tests fail with missing markerName check
On Thu, 16 Sep 2021 01:08:45 GMT, Naoto Sato wrote: > Fixing failing regression tests caused by the JEP 400: UTF-8 by Default. > > `JcmdOutputEncodingTest` test case uses `file.encoding=UTF-8` in `C` locale. > The output from the agent library is in `UTF-8` so it succeeded before the > JEP has been implemented, as System.out used `UTF-8` converter. After the > JEP, it started failing because System.out is using `US-ASCII` which > generates series of '?', ending up with assertion failure in the test. > > The proposed fix is to pass `sun.stdout.encoding=UTF-8` as well as > `file.encoding` so that tool process' System.out is in `UTF-8` as well. Marked as reviewed by sspitsyn (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5539
Re: RFR: 8264148: Update spec for exceptions retrofitted for exception chaining
On Wed, 24 Mar 2021 23:17:46 GMT, Joe Darcy wrote: > 8264148: Update spec for exceptions retrofitted for exception chaining Joe, The Serviceability part looks good. Thanks, Serguei - PR: https://git.openjdk.java.net/jdk/pull/3182
Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v13]
On Fri, 29 Jan 2021 05:14:31 GMT, Lin Zang wrote:
>> Hi Lin,
>>
>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java
>>
>> Thank you for the update.
>>
>> The list of possible commands must include one more variant with no
>> arguments:
>> + * Possible command:
>> + * dumpheap gz=1 file;
>> + * dumpheap gz=1;
>> + * dumpheap file;
>> + * dumpheap
>>
>> The argument processing is still not right.
>> Why are there no checks for gzlevel < 0 and gzlevel > 9 ?
>>
>> I'd suggest the following refactoring below:
>>
>>/*
>> * Possible commands:
>> * dumpheap gz=1 file
>> * dumpheap gz=1
>> * dumpheap file
>> * dumpheap
>> *
>> * Use default filename if cntTokens == 0.
>> * Handle cases with cntTokens == 1 or 2.
>> */
>>
>> if (cntTokens > 2) {
>> err.println("Too big number of options: " +
>> cntTokens);
>> usage();
>> return;
>> }
>> if (cntTokens >= 1) { // parse first argument which is
>> "gz=" option
>> String option = t.nextToken();
>> gzlevel = parseHeapDumpCompressionLevel(option);
>> if (gzlevel <= 0 || gzlevel > 9) {
>> err.println("Invalid "gz=" option: " + option);
>> usage();
>> return;
>> }
>> filename = "heap.bin.gz";
>> }
>> if (cntTokens == 2) { // parse second argument which is
>> filename
>> filename = t.nextToken();
>> if (filename.startsWith("gz=")) {
>> err.println("Filename should not start with
>> "gz=": " + filename);
>> usage();
>> return;
>> }
>> }
>
> Hi @sspitsyn,
> Thanks for your suggestion, the parseHeapDumpCompresslevel() inside tested
> the gzlevel, but I think your code is much reasonable. I will make it in next
> update.
> BRs,
> Lin
You are right.
So, in my suggestion just replace this block:
if (gzlevel <= 0 || gzlevel > 9) {
err.println("Invalid "gz=" option: " + option);
usage();
return;
}
with:
if (gzlevel == 0) {
usage();
return;
}
-
PR: https://git.openjdk.java.net/jdk/pull/1712
RFR: 8165276: Spec states that invoke the premain method in an agent cl…
This change have been already reviewed by Mandy, Alan and David. Now, the PR approval is needed. The push was postponed because the CSR was not approved at that time (it is now): https://bugs.openjdk.java.net/browse/JDK-8248189 Investigation of existing popular java agents was requested by Joe. -- The java.lang.instrument spec: https://docs.oracle.com/en/java/javase/15/docs/api/java.instrument/java/lang/instrument/package-summary.html Summary: The java.lang.instrument spec clearly states: "The agent class must implement a public static premain method similar in principle to the main application entry point." Current implementation of sun/instrument/InstrumentationImpl.java allows the premain method be non-public which violates the spec. This fix aligns the implementation with the spec. Testing: A mach5 run of jdk_instrument tests is in progress. - Commit messages: - JDK-8165276 Spec states that invoke the premain method in an agent class if it's public but implementation differs Changes: https://git.openjdk.java.net/jdk/pull/1694/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1694=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8165276 Stats: 356 lines in 26 files changed: 251 ins; 56 del; 49 mod Patch: https://git.openjdk.java.net/jdk/pull/1694.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1694/head:pull/1694 PR: https://git.openjdk.java.net/jdk/pull/1694
Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`
On Thu, 3 Dec 2020 22:54:54 GMT, Mandy Chung wrote: > This patch replaces some uses of `Reference::get` to `Reference::refersTo` to > avoid keeping a referent strongly reachable that could cause unnecessary > delay in collecting such object. I only made change in some but not all > classes in core libraries when working with Kim on `Reference::refersTo`. > The remaining uses are left for the component owners to convert at > appropriate time. Hi Mandy, Looks good. I guess, you are going to update the copyright comments before the push. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1609
Re: RFR: 8255787: Tag container tests that use cGroups with cgroups keyword
On Tue, 10 Nov 2020 21:24:25 GMT, Harold Seigel wrote: > Please review this small change to add a cgroups keyword to tests that use > cgroups. The fix was tested by running Mach5 container tests. Hi Harold, The fix looks good. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1148
Re: RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
On Tue, 22 Sep 2020 00:38:43 GMT, Serguei Spitsyn wrote:
>> Reviewed in the good old hg times :)
>> See also
>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-August/041453.html
>
> Hi Richard,
>
> Could you consider to place the classes EscapeBarrier and
> JvmtiDeferredUpdates into theyr own .hpp/.cpp files? The
> class JvmtiDeferredUpdates would be better to put into the folder 'prims'
> then.
> src/hotspot/share/opto/macro.cpp:
> @@ -1091,11 +1091,11 @@
> bool PhaseMacroExpand::eliminate_allocate_node(AllocateNode *alloc) {
>// Don't do scalar replacement if the frame can be popped by JVMTI:
>// if reallocation fails during deoptimization we'll pop all
>// interpreter frames for this compiled frame and that won't play
>// nice with JVMTI popframe.
> - if (!EliminateAllocations || JvmtiExport::can_pop_frame() ||
> !alloc->_is_non_escaping) {
> + if (!EliminateAllocations || !alloc->_is_non_escaping) {
> return false;
>}
>
> I wonder if the comment is still correct after you removed the check for
> JvmtiExport::can_pop_frame().
>
> src/hotspot/share/runtime/deoptimization.hpp:
>
> + EscapeBarrier(JavaThread* calling_thread, JavaThread* deoptee_thread, bool
> barrier_active)
> +: _calling_thread(calling_thread), _deoptee_thread(deoptee_thread),
> + _barrier_active(barrier_active && (JVMCI_ONLY(UseJVMCICompiler)
> NOT_JVMCI(false)
> + COMPILER2_PRESENT(|| DoEscapeAnalysis)))
> . . . . . . . . .
> +
> + // Revert ea based optimizations for all java threads
> + EscapeBarrier(JavaThread* calling_thread, bool barrier_active)
> +: _calling_thread(calling_thread), _deoptee_thread(NULL),
>
> Nit: would better to make the parameter deoptee_thread to be the 3rd to
> better mach the seconf constructor.
>
> + bool all_threads()const { return _deoptee_thread == NULL; }
> // Should revert optimizations for all
> threads. + bool self_deopt() const { return _calling_thread ==
> _deoptee_thread; } // Current thread deoptimizes
> its own objects. + bool barrier_active() const { return _barrier_active; }
> // Inactive barriers are
> created if no local objects can escape.
> I'd suggest to put comments in a line before function definitions as it is
> done for other declarations/definitions.
src/hotspot/share/runtime/deoptimization.cpp:
@@ -349,12 +408,12 @@
// Now that the vframeArray has been created if we have any deferred local
writes
// added by jvmti then we can free up that structure as the data is now in
the
// vframeArray
- if (thread->deferred_locals() != NULL) {
-GrowableArray* list =
thread->deferred_locals();
+ if (JvmtiDeferredUpdates::deferred_locals(thread) != NULL) {
+GrowableArray* list =
JvmtiDeferredUpdates::deferred_locals(thread);
int i = 0;
do {
// Because of inlining we could have multiple vframes for a single frame
// and several of the vframes could have deferred writes. Find them all.
if (list->at(i)->id() == array->original().id()) {
@@ -365,13 +424,14 @@
} else {
i++;
}
} while ( i < list->length() );
if (list->length() == 0) {
- thread->set_deferred_locals(NULL);
- // free the list and elements back to C heap.
- delete list;
+ JvmtiDeferredUpdates* updates = thread->deferred_updates();
+ thread->set_deferred_updates(NULL);
+ // free deferred updates.
+ delete updates;
}
It is not clear why the 'list' is not deleted anymore. If it is intentional
then could you, please, add a comment with
an explanation?
-
PR: https://git.openjdk.java.net/jdk/pull/119
Re: RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
On Thu, 17 Sep 2020 07:45:52 GMT, Goetz Lindenmaier wrote:
>> Marked as reviewed by goetz (Reviewer).
>
> Reviewed in the good old hg times :)
> See also
> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-August/041453.html
src/hotspot/share/opto/macro.cpp:
@@ -1091,11 +1091,11 @@
bool PhaseMacroExpand::eliminate_allocate_node(AllocateNode *alloc) {
// Don't do scalar replacement if the frame can be popped by JVMTI:
// if reallocation fails during deoptimization we'll pop all
// interpreter frames for this compiled frame and that won't play
// nice with JVMTI popframe.
- if (!EliminateAllocations || JvmtiExport::can_pop_frame() ||
!alloc->_is_non_escaping) {
+ if (!EliminateAllocations || !alloc->_is_non_escaping) {
return false;
}
I wonder if the comment is still correct after you removed the check for
JvmtiExport::can_pop_frame().
-
PR: https://git.openjdk.java.net/jdk/pull/119
Re: RFR: 8252537: Updated @exception with @throws [v3]
On Sat, 19 Sep 2020 20:24:14 GMT, Vipin Sharma wrote: >> Updated @exception with @throws for core-libs, it fixes all open sub-tasks >> of JDK-8252536. >> >> Open Subtasks part of this fix are: >> 1. JDK-8252537 >> 2. JDK-8252539 >> 3. JDK-8252540 >> 4. JDK-8252541 >> >> Previous conversation on this: >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068540.html > > Vipin Sharma has updated the pull request with a new target base due to a > merge or a rebase. The pull request now > contains one commit: > JDK-8252537 Updated @exception with @throws, implemented review comments I've reviewed the management files only. They look good. - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/95
Re: RFR: 8241390: 'Deadlock' with VM_RedefineClasses::lock_classes()
On Tue, 15 Sep 2020 20:02:38 GMT, Coleen Phillimore wrote: >> The change fixes a 'deadlock' situation in >> VM_RedefineClasses::lock_classes() when the current thread is in the middle >> of redefining the same class. The change is based on the fix [1] suggested >> in the Jira issue [2] comment. >> [1] http://cr.openjdk.java.net/~jiangli/8241390/webrev.00/ >> [2] https://bugs.openjdk.java.net/browse/JDK-8241390 >> >> Testing: :jdk_instrument, tier1-tier3, and tier5 tests pass. > > Changes requested by coleenp (Reviewer). The identifier 'cls' does not reflect the nature of the object and needs to be replaces with something like: classes, redef_classes or classes_list This impacts the files: jvmtiThreadState.hpp and jvmtiRedefineClasses.cpp. - PR: https://git.openjdk.java.net/jdk/pull/190
Re: RFR: 8241390: 'Deadlock' with VM_RedefineClasses::lock_classes()
On Tue, 15 Sep 2020 20:02:33 GMT, Coleen Phillimore wrote:
>> The change fixes a 'deadlock' situation in
>> VM_RedefineClasses::lock_classes() when the current thread is in the middle
>> of redefining the same class. The change is based on the fix [1] suggested
>> in the Jira issue [2] comment.
>> [1] http://cr.openjdk.java.net/~jiangli/8241390/webrev.00/
>> [2] https://bugs.openjdk.java.net/browse/JDK-8241390
>>
>> Testing: :jdk_instrument, tier1-tier3, and tier5 tests pass.
>
> src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 159:
>
>> 157: if (!cls->contains(def_ik)) {
>> 158: def_ik->set_is_being_redefined(false);
>> 159: }
>
> Ok, so adding the Klass to the thread local list for each recursion works
> like ref counting. Can't think of a simpler
> way to do this. Looks good.
Yes, the same class can be pushed to the list multiple times (not more than
once by each recursive redefinition). It'd
make sense to add a comment about it as it is not obvious.
> test/jdk/java/lang/instrument/MakeAgentJAR.sh line 1:
>
>> 1: #!/bin/sh
>
> There are tests in test/hotspot/jtreg/serviceability/jvmti/RedefineClasses
> that don't use shell scripts that are much
> better. Can you add this test using that framework instead?
I'm second for this suggestion from Coleen to get rid of the shell script in
the test.
-
PR: https://git.openjdk.java.net/jdk/pull/190
Re: RFR: 8252537: Updated @exception with @throws
On Wed, 9 Sep 2020 19:29:30 GMT, Vipin Sharma wrote:
> Updated @exception with @throws for core-libs, it fixes all open sub-tasks of
> JDK-8252536.
>
> Open Subtasks part of this fix are:
> 1. JDK-8252537
> 2. JDK-8252539
> 3. JDK-8252540
> 4. JDK-8252541
>
> Previous conversation on this:
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068540.html
I've only looked at the management files. They look good in general.
src/java.management/share/classes/java/lang/management/ClassLoadingMXBean.java
108 * @throws java.lang.SecurityException if a security manager
109 * exists and the caller does not have
110 * ManagementPermission("control").
src/java.management/share/classes/java/lang/management/MemoryMXBean.java
286 * @throws java.lang.SecurityException if a security manager
287 * exists and the caller does not have
288 * ManagementPermission("control").
Could you, please, fix the indentation?
-
Changes requested by sspitsyn (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/95
Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination
Hi David,
The update looks good.
Thanks,
Serguei
On 10/29/19 9:28 PM, David Holmes wrote:
Hi Doug,
Your proposed patch wasn't quite right. I made some adjustments but
I'm still having issues with the test,
HotSpotMethodSubstitutionTest.testThreadSubstitutions, as I don't see
how to make the test execution conditional on the VM config.
Updated webrev:
http://cr.openjdk.java.net/~dholmes/8229516/webrev.v2/
Thanks,
David
-
On 29/10/2019 7:14 pm, Doug Simon wrote:
On 29 Oct 2019, at 10:12, David Holmes wrote:
Hi Doug,
Thanks for taking a look so quickly! :)
On 29/10/2019 6:55 pm, Doug Simon wrote:
Hi David,
Since Graal needs to work against multiple JVMCI versions (and VM
versions!), the Graal changes should only disable the intrinsic
when the relevant VM config is missing:
So to be clear I should revert all the Graal file changes I made and
just apply the patch below?
Yes please.
-Doug
diff --git
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
index 0752a621215..baa2136a6ba 100644
---
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
+++
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
@@ -315,7 +315,7 @@ public class GraalHotSpotVMConfig extends
GraalHotSpotVMConfigBase {
public final int javaThreadAnchorOffset =
getFieldOffset("JavaThread::_anchor", Integer.class,
"JavaFrameAnchor");
public final int javaThreadShouldPostOnExceptionsFlagOffset =
getFieldOffset("JavaThread::_should_post_on_exceptions_flag",
Integer.class, "int", Integer.MIN_VALUE);
public final int threadObjectOffset =
getFieldOffset("JavaThread::_threadObj", Integer.class, "oop");
- public final int osThreadOffset =
getFieldOffset("JavaThread::_osthread", Integer.class, "OSThread*");
+ public final int osThreadOffset =
getFieldOffset("JavaThread::_osthread", Integer.class, "OSThread*",
Integer.MAX_VALUE);
public final int threadIsMethodHandleReturnOffset =
getFieldOffset("JavaThread::_is_method_handle_return",
Integer.class, "int");
public final int threadObjectResultOffset =
getFieldOffset("JavaThread::_vm_result", Integer.class, "oop");
public final int jvmciCountersThreadOffset =
getFieldOffset("JavaThread::_jvmci_counters", Integer.class,
"jlong*");
diff --git
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
index 6b37fff083d..ffc8032d2b0 100644
---
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
+++
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
@@ -441,7 +441,9 @@ public class HotSpotGraphBuilderPlugins {
}
});
- r.registerMethodSubstitution(ThreadSubstitutions.class,
"isInterrupted", Receiver.class, boolean.class);
+ if (config.osThreadOffset != Integer.MAX_VALUE) {
+ r.registerMethodSubstitution(ThreadSubstitutions.class,
"isInterrupted", Receiver.class, boolean.class);
+ }
}
public static final String reflectionClass;
diff --git
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
index 1d694fae2a4..8500c4de115 100644
---
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
+++
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
@@ -308,6 +308,7 @@ public class HotSpotReplacementsUtil {
@Fold
public static int osThreadOffset(@InjectedParameter
GraalHotSpotVMConfig config) {
+ assert config.instanceKlassInitThreadOffset !=
Integer.MAX_VALUE;
return config.osThreadOffset;
}
All the other JVMCI changes look good to me.
-Doug
On 29 Oct 2019, at 08:42, David Holmes
wrote:
Bug: https://bugs.openjdk.java.net/browse/JDK-8229516
CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already
approved)
webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/
This cross-cuts core-libs, hotspot-runtime and the JIT compilers,
but only in small pieces each. There is also a small touch to
serviceability code.
This change is "simply" moving the "interrupted" field out of the
osThread and into the java.lang.Thread so that it can be set
independently of whether the thread is alive (and to make things
easier for loom in the near future). It is
Re: RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class
Hi Brent,
Some quick reply below.
On 9/5/19 5:09 PM, Brent Christian wrote:
Hi, David
On 9/5/19 12:52 AM, David Holmes wrote:
Good to see this all come together :)
:)
So to clarify for others any current caller to
find_class_from_class_loader that passes true for "init" was already
implicitly asking for a linked class (as you must be linked before
you can be initialized). With that in mind I would suggest that in
find_class_from_class_loader you add an assert:
! jclass find_class_from_class_loader(JNIEnv* env, Symbol* name,
jboolean init, jboolean link,
Handle loader, Handle
protection_domain,
jboolean throwError, TRAPS) {
+assert((init && link) || !init, "incorrect use of init/link
arguments");
just to ensure the callers understand this.
I'm good with adding an assert to check for coherent arguments.
(Another interpretation is that if 'init' is set, then the 'link'
argument is ignored, since linking is implied).
Aside: in looking at this I've spotted another existing bug. JNI
FindClass is not specified to perform class initialization, but the
implementation passes init=true!
OK, thanks. I've noted this in JDK-8226540.
src/java.base/share/classes/java/lang/invoke/MethodHandles.java
I don't see the need for the new note given it already has
* @throws LinkageError if the linkage fails
(Discussed in the CSR)
src/java.base/share/classes/sun/launcher/LauncherHelper.java
... > Is AccessControlException the only exception, that is not a
LinkageError, that might be thrown when linking? I would think a
number of others are possible - in particular IllegalAccessError
might occur during verification. Other than the fact a test obviously
triggered this, it's not clear why ACE should be singled out here. ??
Also discussed in the CSR[1].
test/hotspot/jtreg/serviceability/jvmti/ClassStatus/ClassStatus.java
45 // public static void foo(Foo f) { }
Unclear why this exists. Also the test refers initially to class Foo
but then uses Foo2 and Foo3. ??
I'm guessing it's just a leftover from an earlier version of the test.
I've removed the comment, and updated the test @summary.
Yes, it is just a left over from earlier version of the test.
Serguei, anything to add?
I'm happy this test got used and included into the webrev, thanks!
There is also a CSR[2] in need of review.
I've added a couple of comments and will add myself as a reviewer
once things are near finalized.
Thanks for taking a look.
Updated webrev at:
http://cr.openjdk.java.net/~bchristi/8212117/webrev10/
Will look at the webrev soon.
Thanks,
serguei
-Brent
1.
https://bugs.openjdk.java.net/browse/JDK-8222071?focusedCommentId=14288303=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14288303
The spec for the 2-arg and 3-arg Class.forName() methods states they
will "locate, load, and link" the class, however the linking part is
not ensured to happen.
Although the VM spec allows flexibility WRT when classes are linked,
this is a corner where the Class.forName() spec is not being upheld.
While this is not an issue for most usages, 8181144 [3] demonstrates
how this can be a problem (with the debugging interface, in this case).
This fix ensures that linking happens during the course of
Class.forName(). Class.forName() already @throws LinkageError, so
no spec change is needed there. MethodHandles.Lookup.findClass()
needs a small spec update, due to calling Class.forName with
init=false,
Of course Errors are not required to be caught. It is therefore
possible that the new behavior could introduce previously unseen,
possibly unhandled LinkageErrors. A new VM flag
(-XX:+ClassForNameDeferLinking) is introduced to restore the
previous behavior (to keep such code running until it can be updated).
This change surfaced a couple new "A JNI error has occurred"
situations (see 8181033[5]) in the Launcher, by way of
test/jdk/tools/launcher/MainClassCantBeLoadedTest.java
(using the 3-arg Class::forName, detailed in the bug report[4]),
and
test/jdk/tools/launcher/modules/basic/LauncherErrors.java
(using the 2-arg Class::forName).
The Launcher is updated to maintain non-confusing error messages :).
The new test included with this fix ensures that 8181144[3] is
addressed. Thanks go to Serguei Spitsyn for writing the test.
Automated corelibs and hotspot tests pass cleanly.
Thanks,
-Brent
--
1. https://bugs.openjdk.java.net/browse/JDK-8212117
2. https://bugs.openjdk.java.net/browse/JDK-8222071
3. https://bugs.openjdk.java.net/browse/JDK-8181144
4.
https://bugs.openjdk.java.net/browse/JDK-8212117?focusedCommentId=14215986=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14215986
5. https://bugs.openjdk.java.net/browse/JDK-8181033
Re: RFR XS,docs JDK-8225309 HTML issues in jdk.jdi module
Hi Jon,
Looks good to me.
Thanks,
Serguei
On 6/4/19 2:42 PM, Jonathan Gibbons wrote:
Please review another small patch for the jdk.jdi module, to fix a
missing heading in one file and to adjust a heading in another.
No webrev; patch below.
That being said, the file
src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html barely
warrants being its own file, but even what content there is deserves
to be fixed. The table caption is oversized, the table has no borders
and not not use any CSS to "stripe" the rows. However, that cleanup is
out of scope for this accessibility fix for the headings.
-- Jon
JBS: https://bugs.openjdk.java.net/browse/JDK-8225309
$ hg diff -R open
diff -r 4158e6a864d4
src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html
--- a/src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html
Tue Jun 04 13:47:59 2019 -0700
+++ b/src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html
Tue Jun 04 14:35:34 2019 -0700
@@ -41,6 +41,7 @@
+JDI Type Signatures
JDI Type Signatures
diff -r 4158e6a864d4 src/jdk.jdi/share/classes/module-info.java
--- a/src/jdk.jdi/share/classes/module-info.java Tue Jun 04 13:47:59
2019 -0700
+++ b/src/jdk.jdi/share/classes/module-info.java Tue Jun 04 14:35:34
2019 -0700
@@ -46,7 +46,7 @@
* This module includes a simple command-line debugger,
* {@index jdb jdb tool}.
*
- * Global Exceptions
+ * Global Exceptions
*
* This section documents exceptions which apply to the entire API
and are thus
* not documented on individual methods.
