Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v7]
> A fix to reduce ThreadsListHandle overhead in relation to handshakes and > we add sanity checks for ThreadsListHandles higher in the call stack. > > This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision: dholmes CR - change NULL to nullptr. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4677/files - new: https://git.openjdk.java.net/jdk/pull/4677/files/4e207e13..48416864 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4677&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4677&range=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4677.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4677/head:pull/4677 PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 15 Oct 2021 18:27:43 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/runtime/thread.cpp line 1771:
>>
>>> 1769: guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(this),
>>> 1770: "missing ThreadsListHandle in calling context.");
>>> 1771: if (is_exiting()) {
>>
>> This seems an unrelated change in behaviour ??
>
> So suspend_thread and resume thread's caller already takes a
> ThreadsListHandle so this is unnecessary and never happens.
> This seems an unrelated change in behaviour ??
Actually this is equivalent code. The baseline code does this:
ThreadsListHandle tlh;
if (!tlh.includes(this)) {
log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " not on
ThreadsList, no suspension", p2i(this));
return false;
}
What that code means is: if `this` thread does not appear in the newly created
ThreadsListHandle's list, then `this` thread has called `java_suspend()` after
`this` thread has exited. That's the only way that `this` thread could be
missing
from a newly created ThreadsListHandle's list.
All I've done here is get rid of the ThreadsListHandle, verify that the calling
context is protecting `this` and switch to an `is_exiting()` call.
> So suspend_thread and resume thread's caller already takes a ThreadsListHandle
> so this is unnecessary and never happens.
I presume @coleenp is saying that the `is_exiting()` check is not necessary.
That might be true, it might not be true. I was trying to create equivalent
code without creating a nested ThreadsListHandle here and I think I've
done that. I actually think it might be possible for a JVM/TI event handler
to fire after the thread has set is_exiting() and for that event handler to
call SuspendThread() which could get us to this point.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 15 Oct 2021 18:20:12 GMT, Coleen Phillimore wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8249004.cr1.patch
>
> src/hotspot/share/runtime/handshake.cpp line 358:
>
>> 356: bool target_is_dead = false;
>> 357: if (target == nullptr) {
>> 358: target_is_dead = true;
>
> Why would you pass a NULL target thread to Handshake::execute? Why would the
> caller not check if the target is dead?
The `NULL` target thread being passed in is actually handled by the baseline
code:
ThreadsListHandle tlh;
if (tlh.includes(target)) {
`tlh.includes(target)` returns `false` when `target` is `NULL/nullptr`.
I just made the already handled situation more explicit.
> Why would the caller not check if the target is dead?
Hmmm... It's hard for me to answer that question since I didn't write
the original code. The test code that calls `WB_HandshakeWalkStack()`
or `WB_AsyncHandshakeWalkStack()` can call those functions with
a `thread_handle` that translates into a `thread_oop` that returns a
`NULL` `JavaThread*`.
See the comment that I added to `WB_AsyncHandshakeWalkStack()` above.
> src/hotspot/share/runtime/thread.cpp line 497:
>
>> 495: // placement somewhere in the calling context.
>> 496: bool Thread::is_JavaThread_protected_by_my_ThreadsList(const
>> JavaThread* p) {
>> 497: Thread* current_thread = Thread::current();
>
> Shouldn't you call this on the current thread as "this" argument?
I modeled the new check after the existing:
bool Thread::is_JavaThread_protected(const JavaThread* p) {
which is also a static function.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 15 Oct 2021 06:34:42 GMT, David Holmes wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8249004.cr1.patch
>
> src/hotspot/share/prims/jvmtiEventController.cpp line 623:
>
>> 621: // If we have a JvmtiThreadState, then we've reached the point where
>> 622: // threads can exist so create a ThreadsListHandle to protect them.
>> 623: ThreadsListHandle tlh;
>
> Good catch on the missing TLH for this code.
It wasn't quite missing from the baseline code. This version of execute():
`Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target)`
used to always create a ThreadsListHandle. I added a `ThreadsListHandle*`
parameter to that version and created a wrapper with the existing signature
to pass `nullptr` to the execute() version with the `ThreadsListHandle*`
parameter. What that means is that all existing callers of:
`Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target)`
no longer had a ThreadsListHandle created for them. With the new sanity
check in place, I shook the trees to make sure that we had explicit
ThreadsListHandles in place for the locations that needed them.
`JvmtiEventControllerPrivate::recompute_enabled()` happened to be
one of the places where the ThreadsListHandle created by execute()
was hiding the fact that `recompute_enabled()` needed one.
> src/hotspot/share/prims/jvmtiEventController.cpp line 624:
>
>> 622: // threads can exist so create a ThreadsListHandle to protect them.
>> 623: ThreadsListHandle tlh;
>> 624: for (; state != NULL; state = state->next()) {
>
> s/NULL/nullptr/
Missed that one. Fixed.
> src/hotspot/share/runtime/handshake.cpp line 361:
>
>> 359: } else {
>> 360: if (tlh_p == nullptr) {
>> 361:
>> guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(target),
>
> This should be an assert once this has had some bake time.
Agreed. All of the
`guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(),...`
calls should be changed to asserts down the road.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 15 Oct 2021 06:45:02 GMT, David Holmes wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8249004.cr1.patch
>
> src/hotspot/share/runtime/thread.cpp line 1771:
>
>> 1769: guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(this),
>> 1770: "missing ThreadsListHandle in calling context.");
>> 1771: if (is_exiting()) {
>
> This seems an unrelated change in behaviour ??
So suspend_thread and resume thread's caller already takes a ThreadsListHandle
so this is unnecessary and never happens.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Thu, 14 Oct 2021 16:03:28 GMT, Daniel D. Daugherty
wrote:
>> A fix to reduce ThreadsListHandle overhead in relation to handshakes and
>> we add sanity checks for ThreadsListHandles higher in the call stack.
>>
>> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running.
>
> Daniel D. Daugherty has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8249004.cr1.patch
This has more moving pieces than the last version. I'm a bit uneasy about
passing NULL as a thread to Handshake::execute(). This shouldn't be something
that should happen.
src/hotspot/share/runtime/handshake.cpp line 358:
> 356: bool target_is_dead = false;
> 357: if (target == nullptr) {
> 358: target_is_dead = true;
Why would you pass a NULL target thread to Handshake::execute? Why would the
caller not check if the target is dead?
src/hotspot/share/runtime/thread.cpp line 497:
> 495: // placement somewhere in the calling context.
> 496: bool Thread::is_JavaThread_protected_by_my_ThreadsList(const JavaThread*
> p) {
> 497: Thread* current_thread = Thread::current();
Shouldn't you call this on the current thread as "this" argument?
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8266936: Add a finalization JFR event [v17]
On Fri, 15 Oct 2021 09:27:54 GMT, Markus Grönlund wrote: >> Greetings, >> >> Object.finalize() was deprecated in JDK9. There is an ongoing effort to >> replace and mitigate Object.finalize() uses in the JDK libraries; please see >> https://bugs.openjdk.java.net/browse/JDK-8253568 for more information. >> >> We also like to assist users in replacing and mitigating uses in non-JDK >> code. >> >> Hence, this changeset adds a periodic JFR event to help identify which >> classes are overriding Object.finalize(). >> >> Thanks >> Markus > > Markus Grönlund has updated the pull request incrementally with two > additional commits since the last revision: > > - renames > - spelling Thanks for doing the renames. Looks good! - Marked as reviewed by coleenp (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4731
Re: RFR: 8275322: Change nested classes in java.management to static nested classes
On Fri, 15 Oct 2021 06:43:13 GMT, Andrey Turbanov wrote: > Non-static classes hold a link to their parent classes, which can be avoided. Marked as reviewed by sspitsyn (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5962
Re: RFR: 8275322: Change nested classes in java.management to static nested classes
On Fri, 15 Oct 2021 06:43:13 GMT, Andrey Turbanov wrote: > Non-static classes hold a link to their parent classes, which can be avoided. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5962
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Fri, 15 Oct 2021 09:17:12 GMT, Richard Reingruber wrote: >> Ok. So you just need to reacquire the threadLock before the `findThread()` >> call and before exiting the while loop, and hold it until after the >> `trackAppResume()` call. I guess it ok then. But this exiting of the >> handlerLock here and in `blockOnDebuggerSuspend()` is always going to arouse >> suspicion for anyone that reads the code. The first question will be if the >> lock does not need to be held here, why grab it in the first place? We know >> the answer is this lock ordering issue that turns up when `trackAppResume()` >> is later called, but this will be far from obvious to the reader. It might >> be possible to make this a bit clearer with some code restructuring, like >> maybe combining `blockOnDebuggerSuspend()` and `trackAppResume()` into one >> function (I'm not sure that this will actually help, but maybe something to >> consider), but at the very least we need some comments calling out why >> handlerLock is held in the first place and why it is ok to release it. > > I've added the comments. Maybe I should really combine > `blockOnDebuggerSuspend()` and `trackAppResume()` into one function... Hm, I think this can be simplified by swaping blockOnDebuggerSuspend() and trackAppResume(). Can't try it today but will on Monday. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8275322: Change nested classes in java.management to static nested classes
On Fri, 15 Oct 2021 06:43:13 GMT, Andrey Turbanov wrote: > Non-static classes hold a link to their parent classes, which can be avoided. Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5962
Re: RFR: 8275259: Add support for Java level DCmd [v2]
> I proposed to extend DCmd to allow Java developers to customize their own
> diagnostic commands last week.
>
> At present, I have implemented a preliminary version.
>
> In the current implementation, I provided some simple APIs in the Java layer
> (under sun.management.cmd) for defining and registering commands.
>
> - Executable interface
> Java diagnostic commands need to implement this interface, the interface
> only contains a simple method:
>
> /**
> * @param output the output when this executable is running
> */
> void execute(PrintWriter output);
>
>
> - Add two annotations (@Command and @Parameter) to describe the command meta
> info
>
> - Use Factory API to register command, the following forms are supported
>
> @Command(name = "My.Echo", description = "Echo description")
> class Echo implements Executable {
>
> @Parameter(name = "text", ordinal=0, isMandatory = true)
> String text;
>
> @Parameter(name = "repeat", isMandatory = true, defaultValue = "1")
> int repeat;
>
> @Override
> public void execute(PrintWriter out) {
> for (int i = 0 ; i < repeat; i++) {
> out.println(text);
> }
> }
> }
>
> Factory.register(Echo.class);
>
>
>
> Factory.register("My.Date", output -> {
> output.println(new Date());
> });
>
>
> - When the command is running, the VM will call `Executor.executeCommand` to
> execute the command. In the implementation of Executor, I introduced a simple
> timeout mechanism to prevent the command channel from being blocked.
>
> At the VM layer, I extended the existing DCmd framework(implemented JavaDCmd
> and JavaDCmdFactoryImpl) to be compatible with existing functions (jmx, help,
> etc.).
>
> In terms of security, considering that the SecurityManager will be
> deprecated, there are not too many considerations for the time being.
>
> Any input is appreciated.
>
> Thanks,
> Denghui
Denghui Dong has updated the pull request incrementally with one additional
commit since the last revision:
print argument key if not found
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/5938/files
- new: https://git.openjdk.java.net/jdk/pull/5938/files/770b6aef..1f085c26
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5938&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5938&range=00-01
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/5938.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5938/head:pull/5938
PR: https://git.openjdk.java.net/jdk/pull/5938
Re: RFR: 8266936: Add a finalization JFR event [v16]
On Thu, 14 Oct 2021 22:36:30 GMT, Markus Grönlund wrote: >> src/hotspot/share/jfr/support/jfrSymbolTable.hpp line 68: >> >>> 66: template class, >>> typename, size_t> >>> 67: friend class HashTableHost; >>> 68: typedef HashTableHost>> JfrSymbolTable> SymbolTable; >> >> Oh here it is. Since it's an enclosed type, can you rename it something >> simpler like Symbols and the other Strings? > > Maybe :) Renamed. - PR: https://git.openjdk.java.net/jdk/pull/4731
Re: RFR: 8266936: Add a finalization JFR event [v16]
On Thu, 14 Oct 2021 21:43:27 GMT, Coleen Phillimore wrote: >> Markus Grönlund has refreshed the contents of this pull request, and >> previous commits have been removed. The incremental views will show >> differences compared to the previous content of the PR. > > src/hotspot/share/jfr/support/jfrSymbolTable.cpp line 75: > >> 73: >> 74: JfrSymbolTable::JfrSymbolTable() : >> 75: _symbol_table(new SymbolTable(this)), > > I'm confused about which symbol table this is. Is this the same SymbolTable > as classfile/symbolTable.cpp? that instance is assumed to be a singleton. Is > this a different SymbolTable and can it have a different name (thought it was > JfrSymbolTable). Renamed. - PR: https://git.openjdk.java.net/jdk/pull/4731
Re: RFR: 8266936: Add a finalization JFR event [v17]
> Greetings, > > Object.finalize() was deprecated in JDK9. There is an ongoing effort to > replace and mitigate Object.finalize() uses in the JDK libraries; please see > https://bugs.openjdk.java.net/browse/JDK-8253568 for more information. > > We also like to assist users in replacing and mitigating uses in non-JDK code. > > Hence, this changeset adds a periodic JFR event to help identify which > classes are overriding Object.finalize(). > > Thanks > Markus Markus Grönlund has updated the pull request incrementally with two additional commits since the last revision: - renames - spelling - Changes: - all: https://git.openjdk.java.net/jdk/pull/4731/files - new: https://git.openjdk.java.net/jdk/pull/4731/files/96a9d6bf..9b418149 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4731&range=16 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4731&range=15-16 Stats: 80 lines in 6 files changed: 0 ins; 0 del; 80 mod Patch: https://git.openjdk.java.net/jdk/pull/4731.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4731/head:pull/4731 PR: https://git.openjdk.java.net/jdk/pull/4731
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Thu, 14 Oct 2021 18:33:51 GMT, Chris Plummer wrote: >>> Ok, so you need to hold threadLock from the time `blockOnDebuggerSuspend()` >>> is done waiting on it until after `trackAppResume()` is done, and since >>> `trackAppResume()` needs to grab the handlerLock, and you need to grab the >>> handerLock before the threadLock, you need to jump through some lock >>> grabbing/release hoops. >> >> That's correct. >> >>> However, you are in fact releasing the threadLock for a short time in >>> `blockOnDebuggerSuspend()` after the wait completes. Doesn't this expose >>> the resumee to potential suspending after the wait has completed and before >>> `trackAppResume()` has been called? >> >> That's correct too. I wouldn't see an issue with it though. I think this is >> even a preexisting condition. The current thread can lose the race grabbing >> threadLock after it was notified to the debugger trying to suspend again (if >> there wasn't the deadlock of course) and consequently suspendCount can >> (again) be greater than 0 right after the wait. In that case we simply retry. > > Ok. So you just need to reacquire the threadLock before the `findThread()` > call and before exiting the while loop, and hold it until after the > `trackAppResume()` call. I guess it ok then. But this exiting of the > handlerLock here and in `blockOnDebuggerSuspend()` is always going to arouse > suspicion for anyone that reads the code. The first question will be if the > lock does not need to be held here, why grab it in the first place? We know > the answer is this lock ordering issue that turns up when `trackAppResume()` > is later called, but this will be far from obvious to the reader. It might be > possible to make this a bit clearer with some code restructuring, like maybe > combining `blockOnDebuggerSuspend()` and `trackAppResume()` into one function > (I'm not sure that this will actually help, but maybe something to consider), > but at the very least we need some comments calling out why handlerLock is > held in the first place and why it is ok to release it. I've added the comments. Maybe I should really combine `blockOnDebuggerSuspend()` and `trackAppResume()` into one function... - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v6]
> This change fixes deadlocks described in the JBS-bug by: > > * Releasing `handlerLock` before waiting on `threadLock` in > `blockOnDebuggerSuspend()` > > * Notifying on `threadLock` in `threadControl_reset()` > > Also the actions in handleAppResumeBreakpoint() are moved/deferred until > doPendingTasks() runs. This is necessary because an event handler must not > release handlerLock first and foremost because handlers are called while > iterating the handler chain for an event type which is protected by > handlerLock > (see https://github.com/openjdk/jdk/pull/5805) > > The first commit delays the cleanup actions after leaving the loop in > `debugLoop_run()`. It allows to reproduce the deadlock running the dispose003 > test with the command > > > make run-test > TEST=test/hotspot/jtreg/vmTestbase/nsk/jdi/VirtualMachine/dispose/dispose003 > > > The second commit adds a new test that reproduces the deadlock when calling > threadControl_resumeThread() while a thread is waiting in > blockOnDebuggerSuspend(). > > The third commit contains the fix described above. With it the deadlocks > cannot be reproduced anymore. > > The forth commit removes the diagnostic code introduced with the first commit > again. > > The fix passed > > test/hotspot/jtreg/serviceability/jdwp > test/jdk/com/sun/jdi > test/hotspot/jtreg/vmTestbase/nsk/jdwp > test/hotspot/jtreg/vmTestbase/nsk/jdi Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision: Better comments on lock usage. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/806bceb8..20f1f31b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=04-05 Stats: 26 lines in 1 file changed: 24 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5849.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5849/head:pull/5849 PR: https://git.openjdk.java.net/jdk/pull/5849
