Re: RFR: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout [v3]
On Wed, 27 Mar 2024 05:12:53 GMT, Chris Plummer wrote:
>> Serguei Spitsyn 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 three additional
>> commits since the last revision:
>>
>> - “Merge”
>> - review: updated test with one more call to notifyAtBreakpoint to reset
>> the native state
>> - 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout
>
> test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/PopFrameTest.java
> line 147:
>
>> 145: }
>> 146: log("Main #B.2: got expected JVMTI_ERROR_NONE");
>> 147: notifyAtBreakpoint(); // needed to reset the native state
>
> This doesn't make much sense to me. The purpose of `notifyAtBreakpoint()` is
> to unblock the Breakpoint event handler, which is waiting for this notify.
> However, the event handler can only ever be entered once, yet we have a
> previous `noitfyBreakpoint()` above and a 3rd `notifyAtBreakpoint()` below.
> It seems that this call and the one below are no-ops. I don't see how
> bp_sync_reached ever gets set true again after the first
> `notifyAtBreakpoint()` call is made.
Thank you for the comment, Chris. You are right.
I've already figured my mistake/confusion (shared it on our team meeting) but
tested my update which has been just pushed. My analysis in the bug report is
incorrect, I'll update it soon. In fact, I don't really know what was the root
cause of this deadlock because I can't reproduce the issue. Also, it is not
easy to figure out by the code observation. The only idea I have is that it was
a spurious wakeup on the `wait()` at line 52 in the Breakpoint handler of the
native agent. I've added a couple of checks for robustness and added more
tracing to get more details if this happen again.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1540566744
Re: RFR: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout [v4]
> This PR fixes a synchronization issue in the test: > `test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest` > > The method `notifyAtBreakpoint()` can notify the `TestTask` thread when it > has not reached an expected breakpoint yet. > The fix is to add a call to the method `ensureAtBreakpoint()` one more time > in the `B2` sub-test. It is needed after the top-most frame was popped with > the JVMTI `PopFrame`, and the target thread needs to reach the breakpoint > again after its execution was resumed. > > The time is very intermittent. At least, I was not able to reproduce the > timeout failure in thousands of mach5 runs with the `-Xcomp` option. > > Testing: > - Run the test > `test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest` thousands > times in mach5 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: improve diagnostics and reliability - Changes: - all: https://git.openjdk.org/jdk/pull/18419/files - new: https://git.openjdk.org/jdk/pull/18419/files/1502492f..f4f92e92 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18419=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18419=02-03 Stats: 19 lines in 2 files changed: 9 ins; 4 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/18419.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18419/head:pull/18419 PR: https://git.openjdk.org/jdk/pull/18419
Re: RFR: JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set [v9]
On Tue, 26 Mar 2024 21:42:15 GMT, Chris Plummer wrote: > A docs CR will need to be filed to get it updated (and I see it is already > appears out-of-date w.r.t. HeapDumpGzipLevel) Sorry maybe it is maybe obvious for you but where should I open a "docs CR", would that be a separate JBS issue with Component name docs ? Should I include the HeapDumpGzipLevel in the same "docs CR" (but this might be bad for potential backporting) ? - PR Comment: https://git.openjdk.org/jdk/pull/18190#issuecomment-2022660444
Re: RFR: 8318026: jcmd should provide access to detailed JVM object information [v10]
> Introduce the jcmd "VM.inspect" to implement access to detailed JVM object > information. > > Not recommended for live production use. Requires UnlockDiagnosticVMOptions > and not included in jcmd help output, to remind us this is not a > general-purpose customer-facing tool. Kevin Walls has updated the pull request incrementally with two additional commits since the last revision: - Argument to be named address. - test nit - Changes: - all: https://git.openjdk.org/jdk/pull/17655/files - new: https://git.openjdk.org/jdk/pull/17655/files/bf5e4b26..837154c1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17655=09 - incr: https://webrevs.openjdk.org/?repo=jdk=17655=08-09 Stats: 28 lines in 3 files changed: 6 ins; 3 del; 19 mod Patch: https://git.openjdk.org/jdk/pull/17655.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17655/head:pull/17655 PR: https://git.openjdk.org/jdk/pull/17655
Re: RFR: JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set [v10]
> Currently jcmd command GC.heap_dump only works with an additionally provided > file name. > Syntax : GC.heap_dump [options] > > In case the JVM has the XX - flag HeapDumpPath set, we should support an > additional mode where the is optional. > In case the filename is NOT set, we take the HeapDumpPath (file or directory); > > new syntax : > GC.heap_dump [options] .. has precedence over second option > GC.heap_dump [options] …in case -XX: HeapDumpPath=p is set > > This would be a simplification e.g. for support cases where a filename or > directory is set at JVM startup with -XX: HeapDumpPath=p and writing to the > path is intended/recommended for usage also in the jcmd case. Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: adjust java.1 man page - Changes: - all: https://git.openjdk.org/jdk/pull/18190/files - new: https://git.openjdk.org/jdk/pull/18190/files/b414b1be..8039b9a4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18190=09 - incr: https://webrevs.openjdk.org/?repo=jdk=18190=08-09 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18190.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18190/head:pull/18190 PR: https://git.openjdk.org/jdk/pull/18190
Re: RFR: 8318026: jcmd should provide access to detailed JVM object information [v9]
On Tue, 26 Mar 2024 21:55:38 GMT, Kevin Walls wrote: >> Introduce the jcmd "VM.inspect" to implement access to detailed JVM object >> information. >> >> Not recommended for live production use. Requires UnlockDiagnosticVMOptions >> and not included in jcmd help output, to remind us this is not a >> general-purpose customer-facing tool. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Undo include I am still feeling uneasy about this new command. I can see its potential usefulness, but as stated before would prefer it being limited to debug-only JVMs. - jcmd can be exposed remotely, e.g., via jmx. I am not sure whether other solutions exist, but exposing jcmd beyond the immediate box the JVM runs on is something many folks want, and that is technically easy to do. So, customer-local solutions may exist for that, and the reach of jcmd may be larger than we think. - the underlying functionality for print_location was written with debugging and error analysis in mind. We keep adding functionality there. I don't think developers adding to that function have in mind that this functionality may be exposed, possibly remotely. In my opinion, UnlockDiagnosticVMOptions is not a good enough safeguard since it guards a whole swathe of switches that we may instruct the customer to enable. Once enabled, my experience is that UnlockDiagnosticVMOptions often lingers around. It is not unusual for customer scenarios to have set +UnlockDiagnosticVMOptions because of some years ago support cases. If others feel this command is safe enough, I'll shut up and be quiet, since I cannot think up a concrete attack scenario. - PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-2022310874
Re: RFR: 8318026: jcmd should provide access to detailed JVM object information [v10]
On Wed, 27 Mar 2024 12:23:50 GMT, Kevin Walls wrote: >> Introduce the jcmd "VM.inspect" to implement access to detailed JVM object >> information. >> >> Not recommended for live production use. Requires UnlockDiagnosticVMOptions >> and not included in jcmd help output, to remind us this is not a >> general-purpose customer-facing tool. > > Kevin Walls has updated the pull request incrementally with two additional > commits since the last revision: > > - Argument to be named address. > - test nit Thanks for the comments, addressed nits and will update further on the remaining points. Thomas thanks for your comments. Yes, you do see stale CompilerCommand directives etc... hanging around at times, we can only encourage good housekeeping! Remote access over JMX is not required for this command (hidden). - PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-2023297192
Re: RFR: 8329204: Diagnostic command for zeroing unused parts of the heap [v2]
> Diagnostic command for zeroing unused parts of the heap > > I propose to add a new diagnostic command `System.zero_unused_memory` which > zeros out all unused parts of the heap. The name of the command is > intentionally GC/heap agnostic because in the future it might be extended to > also zero unused parts of the Metaspace and/or CodeCache. > > Currently `System.zero_unused_memory` triggers a full GC and afterwards zeros > unused parts of the heap. Zeroing can help snapshotting technologies like > [CRIU][1] or [Firecracker][2] to shrink the snapshot size of VMs/containers > with running JVM processes because pages which only contain zero bytes can be > easily removed from the image by making the image *sparse* (e.g. with > [`fallocate -p`][3]). > > Notice that uncommitting unused heap parts in the JVM doesn't help in the > context of virtualization (e.g. KVM/Firecracker) because from the host > perspective they are still dirty and can't be easily removed from the > snapshot image because they usually contain some non-zero data. More details > can be found in my FOSDEM talk ["Zeroing and the semantic gap between host > and guest"][4]. > > Furthermore, removing pages which only contain zero bytes (i.e. "empty > pages") from a snapshot image not only decreases the image size but also > speeds up the restore process because empty pages don't have to be read from > the image file but will be populated by the kernel zero page first until they > are used for the first time. This also decreases the initial memory footprint > of a restored process. > > An additional argument for memory zeroing is security. By zeroing unused heap > parts, we can make sure that secrets contained in unreferenced Java objects > are deleted. Something that's currently impossibly to achieve from Java > because even if a Java program zeroes out arrays with sensitive data after > usage, it can never guarantee that the corresponding object hasn't already > been moved by the GC and an old, unreferenced copy of that data still exists > somewhere in the heap. > > A prototype implementation for this proposal for Serial, Parallel, G1 and > Shenandoah GC is available in the linked pull request. > > [1]: https://criu.org > [2]: > https://github.com/firecracker-microvm/firecracker/blob/main/docs/snapshotting/snapshot-support.md > [3]: https://man7.org/linux/man-pages/man1/fallocate.1.html > [4]: > https://fosdem.org/2024/schedule/event/fosdem-2024-3454-zeroing-and-the-semantic-gap-between-host-and-guest/ Volker Simonis has updated the pull request incrementally with one additional commit since the last revision: Fix build error on MacOs (with clang, if we use 'override' for a virtual method we have to use it for all methods to avoid '-Winconsistent-missing-override') - Changes: - all: https://git.openjdk.org/jdk/pull/18521/files - new: https://git.openjdk.org/jdk/pull/18521/files/b06aa327..4264e53d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18521=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18521=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18521.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18521/head:pull/18521 PR: https://git.openjdk.org/jdk/pull/18521
Re: RFR: 8318026: jcmd should provide access to detailed JVM object information [v9]
On Wed, 27 Mar 2024 09:33:43 GMT, Thomas Stuefe wrote: > In my opinion, UnlockDiagnosticVMOptions is not a good enough safeguard since > it guards a whole swathe of switches that we may instruct the customer to > enable. Once enabled, my experience is that UnlockDiagnosticVMOptions often > lingers around. It is not unusual for customer scenarios to have set > +UnlockDiagnosticVMOptions because of some years ago support cases. I think we also need to consider the flip side of this argument. Is this something that some customers might want to always enable, but don't want to always have UnlockDiagnosticVMOptions enabled. A new command line flag would be needed in that case. Also, isn't UnlockDiagnosticVMOptions meant for enabling the use of diagnostic command line flags? Do we have examples of it enabling a hotspot feature that does not also require setting a diagnostic command line flag? - PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-2023675637
RFR: 8329204: Diagnostic command for zeroing unused parts of the heap
Diagnostic command for zeroing unused parts of the heap I propose to add a new diagnostic command `System.zero_unused_memory` which zeros out all unused parts of the heap. The name of the command is intentionally GC/heap agnostic because in the future it might be extended to also zero unused parts of the Metaspace and/or CodeCache. Currently `System.zero_unused_memory` triggers a full GC and afterwards zeros unused parts of the heap. Zeroing can help snapshotting technologies like [CRIU][1] or [Firecracker][2] to shrink the snapshot size of VMs/containers with running JVM processes because pages which only contain zero bytes can be easily removed from the image by making the image *sparse* (e.g. with [`fallocate -p`][3]). Notice that uncommitting unused heap parts in the JVM doesn't help in the context of virtualization (e.g. KVM/Firecracker) because from the host perspective they are still dirty and can't be easily removed from the snapshot image because they usually contain some non-zero data. More details can be found in my FOSDEM talk ["Zeroing and the semantic gap between host and guest"][4]. Furthermore, removing pages which only contain zero bytes (i.e. "empty pages") from a snapshot image not only decreases the image size but also speeds up the restore process because empty pages don't have to be read from the image file but will be populated by the kernel zero page first until they are used for the first time. This also decreases the initial memory footprint of a restored process. An additional argument for memory zeroing is security. By zeroing unused heap parts, we can make sure that secrets contained in unreferenced Java objects are deleted. Something that's currently impossibly to achieve from Java because even if a Java program zeroes out arrays with sensitive data after usage, it can never guarantee that the corresponding object hasn't already been moved by the GC and an old, unreferenced copy of that data still exists somewhere in the heap. A prototype implementation for this proposal for Serial, Parallel, G1 and Shenandoah GC is available in the linked pull request. [1]: https://criu.org [2]: https://github.com/firecracker-microvm/firecracker/blob/main/docs/snapshotting/snapshot-support.md [3]: https://man7.org/linux/man-pages/man1/fallocate.1.html [4]: https://fosdem.org/2024/schedule/event/fosdem-2024-3454-zeroing-and-the-semantic-gap-between-host-and-guest/ - Commit messages: - Make VM_ZeroUnusedMemory a VM_GC_Sync_Operation - Implement unused memory zeroing for ShenadoahGC and move the zeroing part into a VM operation - Implement unused memory zeroing for G1GC - Implement unused memory zeroing for ParallelGC - 8329204: Diagnostic command for zeroing unused parts of the heap Changes: https://git.openjdk.org/jdk/pull/18521/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18521=00 Issue: https://bugs.openjdk.org/browse/JDK-8329204 Stats: 187 lines in 29 files changed: 187 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18521.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18521/head:pull/18521 PR: https://git.openjdk.org/jdk/pull/18521
Re: RFR: 8318026: jcmd should provide access to detailed JVM object information [v9]
On Wed, 27 Mar 2024 05:44:25 GMT, Chris Plummer wrote: > It looks like the test only inspects a thread and a java object. Perhaps you > could add tests for additional VM objects. Maybe grab a frame PC from a > thread stack trace. Yes - added a couple of metadata tests, and a compiled method test, which gets an address from Events info. We know that should resolve to a compiled method (if we have such an event, so this copes if there are none). Also the VM.info and Thread.print runs with the CommandExecutor are now silent. They are long, and max out the test log file and make it truncate. That output could mainly be useful if the regexes can't match items, but the output format should be reproducible in a new run. Also if we fail to find something we need, like a thread id, it will print the full output it searched. - PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-2023578758
Re: RFR: 8318026: jcmd should provide access to detailed JVM object information [v11]
> Introduce the jcmd "VM.inspect" to implement access to detailed JVM object > information. > > Not recommended for live production use. Requires UnlockDiagnosticVMOptions > and not included in jcmd help output, to remind us this is not a > general-purpose customer-facing tool. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Test more pointer types: compiled method and metadata. - Changes: - all: https://git.openjdk.org/jdk/pull/17655/files - new: https://git.openjdk.org/jdk/pull/17655/files/837154c1..9ed958f6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17655=10 - incr: https://webrevs.openjdk.org/?repo=jdk=17655=09-10 Stats: 58 lines in 1 file changed: 46 ins; 0 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/17655.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17655/head:pull/17655 PR: https://git.openjdk.org/jdk/pull/17655
Re: RFR: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout [v4]
On Wed, 27 Mar 2024 06:44:37 GMT, Serguei Spitsyn wrote:
>> This PR fixes a synchronization issue in the test:
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest`
>>
>> The method `notifyAtBreakpoint()` can notify the `TestTask` thread when it
>> has not reached an expected breakpoint yet.
>> The fix is to add a call to the method `ensureAtBreakpoint()` one more time
>> in the `B2` sub-test. It is needed after the top-most frame was popped with
>> the JVMTI `PopFrame`, and the target thread needs to reach the breakpoint
>> again after its execution was resumed.
>>
>> The time is very intermittent. At least, I was not able to reproduce the
>> timeout failure in thousands of mach5 runs with the `-Xcomp` option.
>>
>> Testing:
>> - Run the test
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest` thousands
>> times in mach5
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: improve diagnostics and reliability
test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/PopFrameTest.java
line 148:
> 146: log("Main #B.2: got expected JVMTI_ERROR_NONE");
> 147: resumeThread(testTaskThread);
> 148:
You probably don't need this minor edit or the copyright update any more.
However, it's still unclear to me how the ensureAtBreakpoint() below is suppose
to work if we already called notifyAtBreakpoint() between the 1st and 2nd
ensureAtBreakpoint(). From what I can tell, notifyAtBreakpoint() clears the
flag that ensureAtBreakpoint() checks for, and there is no 2nd breakpoint to
set it again. I would expect the ensureAtBreakpoint() below to always wait
indefinitely, but that doesn't appear to be the case. So how is this working? I
must be missing something.
test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/libPopFrameTest.cpp
line 58:
> 56: LOG("Breakpoint: In method TestTask.B(): after sync section\n");
> 57:
> 58: if (do_pop_frame != JNI_FALSE) {
Suggestion:
if (do_pop_frame) {
test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/libPopFrameTest.cpp
line 161:
> 159: int attempts = 0;
> 160: while (!bp_sync_reached) {
> 161: LOG("Main: ensureAtBreakpoint: waitig 5 millis\n");
Suggestion:
LOG("Main: ensureAtBreakpoint: waiting 10 millis\n");
I think this should be moved directly above the wait(10) call.
test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/libPopFrameTest.cpp
line 163:
> 161: LOG("Main: ensureAtBreakpoint: waitig 5 millis\n");
> 162: if (++attempts > 100) {
> 163: fatal(jni, "Main: ensureAtBreakpoint: waited 1 sec");
1 second isn't very long when you are talking about something that is relying
on debugger/debuggee communication. Maybe wait 100ms at a time for a total of
10 seconds.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1541957419
PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1541958086
PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1541947950
PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1541960905
Re: RFR: JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
On Wed, 27 Mar 2024 22:04:30 GMT, Jonathan Gibbons wrote:
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
src/jdk.javadoc/share/man/javadoc.1 line 111:
> 109: source code with the \f[V]javac\f[R] option \f[V]-Xlint\f[R], or more
> 110: specifically, \f[V]-Xlint:dangling-doc-comments\f[R].
> 111: Within a source file, you may use suppress any warnings generated by
Typo?
Suggestion:
Within a source file, you may suppress any warnings generated by
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18527#discussion_r1542131487
Re: RFR: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout [v4]
On Wed, 27 Mar 2024 20:08:19 GMT, Chris Plummer wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: improve diagnostics and reliability
>
> test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/libPopFrameTest.cpp
> line 163:
>
>> 161: LOG("Main: ensureAtBreakpoint: waitig 5 millis\n");
>> 162: if (++attempts > 100) {
>> 163: fatal(jni, "Main: ensureAtBreakpoint: waited 1 sec");
>
> 1 second isn't very long when you are talking about something that is relying
> on debugger/debuggee communication. Maybe wait 100ms at a time for a total of
> 10 seconds.
Caught this comment in passing. Delays like this should be scaled with
defaultTimeoutFactor so that test tasks that invoke tests with options
that can slow the test down, e.g., `-Xcomp`, can be accommodated.
I believe the defaultTimeoutFact for `-Xcomp` test tasks gets bumped
from 4 to 10.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1541986223
RFR: JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
Please review the updates to support a proposed new
`-Xlint:dangling-doc-comments` option.
The work can be thought of as in 3 parts:
1. An update to the `javac` internal class `DeferredLintHandler` so that it is
possible to specify the appropriately configured `Lint` object when it is time
to consider whether to generate the diagnostic or not.
2. Updates to the `javac` front end to record "dangling docs comments" found
near the beginning of a declaration, and to report them using an instance of
`DeferredLintHandler`. This allows the warnings to be enabled or disabled using
the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The procedure
for handling dangling doc comments is described in this comment in
`JavacParser`.
* Dangling documentation comments are handled as follows.
* 1. {@code Scanner} adds all doc comments to a queue of
* recent doc comments. The queue is flushed whenever
* it is known that the recent doc comments should be
* ignored and should not cause any warnings.
* 2. The primary documentation comment is the one obtained
* from the first token of any declaration.
* (using {@code token.getDocComment()}.
* 3. At the end of the "signature" of the declaration
* (that is, before any initialization or body for the
* declaration) any other "recent" comments are saved
* in a map using the primary comment as a key,
* using this method, {@code saveDanglingComments}.
* 4. When the tree node for the declaration is finally
* available, and the primary comment, if any,
* is "attached", (in {@link #attach}) any related
* dangling comments are also attached to the tree node
* by registering them using the {@link #deferredLintHandler}.
* 5. (Later) Warnings may be genereated for the dangling
* comments, subject to the {@code -Xlint} and
* {@code @SuppressWarnings}.
3. Updates to the make files to disable the warnings in modules for which the
warning is generated. This is often because of the confusing use of `/**` to
create box or other standout comments.
-
Commit messages:
- JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
Changes: https://git.openjdk.org/jdk/pull/18527/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=18527=00
Issue: https://bugs.openjdk.org/browse/JDK-8303689
Stats: 477 lines in 60 files changed: 368 ins; 5 del; 104 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
Re: RFR: JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
On Wed, 27 Mar 2024 22:04:30 GMT, Jonathan Gibbons wrote:
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Would this be the first lint -- not doclint -- warning related to comments, let
alone doc comments?
-
PR Comment: https://git.openjdk.org/jdk/pull/18527#issuecomment-2024106466
Re: RFR: JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
On Wed, 27 Mar 2024 22:04:30 GMT, Jonathan Gibbons wrote:
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Javadoc changes look trivially good. I only note that the javadoc man page diff
contains some unrelated changes.
-
PR Comment: https://git.openjdk.org/jdk/pull/18527#issuecomment-2024116236
Re: RFR: JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set [v10]
On Wed, 27 Mar 2024 13:44:42 GMT, Matthias Baesken wrote: >> Currently jcmd command GC.heap_dump only works with an additionally provided >> file name. >> Syntax : GC.heap_dump [options] >> >> In case the JVM has the XX - flag HeapDumpPath set, we should support an >> additional mode where the is optional. >> In case the filename is NOT set, we take the HeapDumpPath (file or >> directory); >> >> new syntax : >> GC.heap_dump [options] .. has precedence over second option >> GC.heap_dump [options] …in case -XX: HeapDumpPath=p is set >> >> This would be a simplification e.g. for support cases where a filename or >> directory is set at JVM startup with -XX: HeapDumpPath=p and writing to the >> path is intended/recommended for usage also in the jcmd case. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > adjust java.1 man page Notwithstanding that there may be cases where this change would be convenient, I really don't like this coupling between the jcmd behaviour and a -XX flag that is intended for something else. It doesn't completely mesh with the jcmd usage and other options, and the documentation story is quite complicated. > This change can help avoid the need for an additional copy and paste step, > which is prone to errors. To me that is not a sufficient benefit for the complexity this change would add. Further, it is not at all clear to me that a dynamic heap-dump _should_ be using the same destination as that set for the `HeapDumpOnOutOfMemory` case. - PR Comment: https://git.openjdk.org/jdk/pull/18190#issuecomment-2024127213
Re: RFR: JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set [v9]
On Wed, 27 Mar 2024 12:36:31 GMT, Matthias Baesken wrote: > Sorry maybe it is maybe obvious for you but where should I open a "docs CR", > would that be a separate JBS issue with Component name docs ? You would file a JBS issue with component "docs" and subcategory "hotspot". But let's hold off on that for now until if it is decided if the PR is going to be pushed. My main reason for mentioning it was to point out additional fallout of this change. > Should I include the HeapDumpGzipLevel in the same "docs CR" (but this might > be bad for potential backporting) ? Backporting also came to mind since the trouble shooting guide would need to be updated for each Oracle version this PR is backported to. And yes, HeapDumpGzipLevel docs need to match the actual implementation. I'm not sure if HeapDumpGzipLevel has been backported to 8, 11, and 17 (in the open for for the Oracle release). I'm inclined to say a separate jbs issue should track updating docs for HeapDumpGzipLevel, but then we also have the question of whether or not HeapDumpGzipLevel should impact the jcmd if this PR is to be pushed. - PR Comment: https://git.openjdk.org/jdk/pull/18190#issuecomment-2024147551
Re: RFR: JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set [v10]
On Wed, 27 Mar 2024 23:05:22 GMT, David Holmes wrote: > Notwithstanding that there may be cases where this change would be > convenient... I did get feedback from a couple of our support engineers. One felt it was of no real benefit as it was just as easy to provide the filename as an argument to the jcmd. The other thought it might be of some benefit, but also had a misunderstanding of how the jcmd currently works (thought that with no filename given, it would automatically dump into the current directory). There were no strong arguments for this PR, especially that overcome all the issues it creates. - PR Comment: https://git.openjdk.org/jdk/pull/18190#issuecomment-2024153420
Re: RFR: JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
On Wed, 27 Mar 2024 22:41:33 GMT, Pavel Rappo wrote: > Would this be the first lint -- not doclint -- warning related to comments, > let alone doc comments? No. `-Xlint:dep-ann` correlates `@Deprecated` annotations with `@deprecated` tags in doc comments. > src/jdk.javadoc/share/man/javadoc.1 line 111: > >> 109: source code with the \f[V]javac\f[R] option \f[V]-Xlint\f[R], or more >> 110: specifically, \f[V]-Xlint:dangling-doc-comments\f[R]. >> 111: Within a source file, you may use suppress any warnings generated by > > Typo? > Suggestion: > > Within a source file, you may suppress any warnings generated by Thanks; I'll check the underlying original. - PR Comment: https://git.openjdk.org/jdk/pull/18527#issuecomment-2024162355 PR Review Comment: https://git.openjdk.org/jdk/pull/18527#discussion_r1542157047
