Re: RFR: 8285739: disable EA when both JVMTI and Preview are enabled [v2]
On Sat, 21 May 2022 16:09:17 GMT, Leonid Mesnik wrote: > > Hi Leonid, if EscapeAnalysis is not disabled, then local objects cannot be > > read per JVMTI if they are scalarized in compiled frames on the heap, > > right? This would be a problem I'd think. Thanks, Richard. > > Yes, the fix restores correct behavior for platform threads if > --enable-preview is on. The > [JDK-8285739](https://bugs.openjdk.java.net/browse/JDK-8285739) should fix > this for virtual threads. Could it be that you mean [JDK-8264699](https://bugs.openjdk.java.net/browse/JDK-8264699)? I'm ok with this version of your fix. I'd suggest to change title/synopsis of the bug report to better match it though. - PR: https://git.openjdk.java.net/jdk/pull/8589
Re: RFR: 8285739: disable EA when both JVMTI and Preview are enabled [v2]
On Fri, 20 May 2022 20:09:59 GMT, Leonid Mesnik wrote: >> The fix disables EscapeBarrier and EscapeAnalysis when certain JVMTI >> capabilities are enabled and --enable-preview. >> >> It restores the same behavior as it was before >> https://bugs.openjdk.java.net/browse/JDK-8227745 "Enable Escape Analysis for >> Better Performance in the Presence of JVMTI Agents" is implemented when >> Continuations are enabled. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > 2nd v Hi Leonid, if EscapeAnalysis is not disabled, then local objects cannot be read per JVMTI if they are scalarized in compiled frames on the heap, right? This would be a problem I'd think. Thanks, Richard. - PR: https://git.openjdk.java.net/jdk/pull/8589
Re: RFR: 8285739: disable EA when both JVMTI and Preview are enabled
On Mon, 9 May 2022 01:36:39 GMT, Leonid Mesnik wrote: > The fix disables EscapeBarrier and EscapeAnalysis when certain JVMTI > capabilities are enabled and --enable-preview. > > It restores the same behavior as it was before > https://bugs.openjdk.java.net/browse/JDK-8227745 "Enable Escape Analysis for > Better Performance in the Presence of JVMTI Agents" is implemented when > Continuations are enabled. Hi Leonid, your changes look good to me. Thanks, Richard. Ok, thanks for letting me know. - Marked as reviewed by rrich (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8589
Re: RFR: 8285739: disable EA when both JVMTI and Preview are enabled
On Mon, 9 May 2022 01:36:39 GMT, Leonid Mesnik wrote: > The fix disables EscapeBarrier and EscapeAnalysis when certain JVMTI > capabilities are enabled and --enable-preview. > > It restores the same behavior as it was before > https://bugs.openjdk.java.net/browse/JDK-8227745 "Enable Escape Analysis for > Better Performance in the Presence of JVMTI Agents" is implemented when > Continuations are enabled. Hi Leonid, have you done some testing? [JDK-8227745](https://bugs.openjdk.java.net/browse/JDK-8227745) came with a bunch of tests. I wonder how they behave with your change. Thanks, Richard. - PR: https://git.openjdk.java.net/jdk/pull/8589
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v10]
On Fri, 22 Oct 2021 17:48:36 GMT, Chris Plummer wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Improve comment as suggested by Chris. > > Marked as reviewed by cjplummer (Reviewer). Thanks for the reviews @plummercj, @sspitsyn, @schmelter-sap! - PR: https://git.openjdk.java.net/jdk/pull/5849
Integrated: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend
On Thu, 7 Oct 2021 13:50:49 GMT, Richard Reingruber wrote: > 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 This pull request has now been integrated. Changeset: ca2efb73 Author:Richard Reingruber URL: https://git.openjdk.java.net/jdk/commit/ca2efb73f59112d9be2ec29db405deb4c58dd435 Stats: 333 lines in 2 files changed: 314 ins; 14 del; 5 mod 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend Reviewed-by: cjplummer, sspitsyn, rschmelter - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v12]
> 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: Changes based on Ralf's feedback. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/85dfaef1..1da43f79 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5849=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5849=10-11 Stats: 28 lines in 1 file changed: 9 ins; 10 del; 9 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
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v10]
On Mon, 8 Nov 2021 19:10:23 GMT, Ralf Schmelter wrote:
>> Richard Reingruber has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Improve comment as suggested by Chris.
>
> src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 812:
>
>> 810: {
>> 811: jthread resumer = evinfo->thread;
>> 812: ThreadNode *node;
>
> You could move the declaration into the if() block below, since it is not
> needed elsewhere.
Done.
> src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 2192:
>
>> 2190: * ordering handlerLock has to be acquired before threadLock.
>> 2191: */
>> 2192: debugMonitorExit(threadLock);
>
> You could move this to the if (resumer != NULL) block, since otherwise all
> the locking and unlocking is not needed anyways as far as I can see.
Done. Thanks for reviewing!
-
PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v10]
On Wed, 10 Nov 2021 01:24:58 GMT, Serguei Spitsyn wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Improve comment as suggested by Chris. > > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 2189: > >> 2187: >> 2188: /* >> 2189: * trackAppResume() (indirectly) aquires handlerLock. For >> proper lock > > I'd suggest to get rid of '()' brackets in this comment line, so it will be: >`* trackAppResume indirectly aquires handlerLock. For proper lock` Sure. Done. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v11]
> 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: Improve comment as suggested by Serguei. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/3383a236..85dfaef1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5849=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5849=09-10 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v10]
On Wed, 10 Nov 2021 01:27:40 GMT, Serguei Spitsyn wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Improve comment as suggested by Chris. > > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 2194: > >> 2192: debugMonitorExit(threadLock); >> 2193: eventHandler_lock(); >> 2194: debugMonitorEnter(threadLock); > > The lines 2192-2194 look like a hack to me. > But I see you have filed an enhancement to fix this. > I'm okay to address it later. Thanks for looking at this Serguei. I agree that this part looks a little odd. It is still correct, though, and the changes are limited to the hardly used code responsible for handling j.l.Thread.resume() calls which is a good thing IMHO. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v9]
On Fri, 22 Oct 2021 04:02:04 GMT, Chris Plummer wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Hoist locking from trackAppResume() up to it its caller doPendingTasks() > > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 2212: > >> 2210: * handlerLock is not needed anymore. We release it before >> calling >> 2211: * blockOnDebuggerSuspend() because it is required for resumes >> by the >> 2212: * debugger so we cannot wait for that holding handlerLock. > > * handlerLock is not needed anymore. We must release it before calling > * blockOnDebuggerSuspend() because it is required for resumes done by > * the debugger. If resumee is currently suspended by the debugger, > then > * blockOnDebuggerSuspend() will block until a debugger resume is > done. > * If it blocks while holding the handlerLock, then the resume will > deadlock. Done. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v10]
> 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: Improve comment as suggested by Chris. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/c884a76e..3383a236 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5849=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5849=08-09 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 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
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v7]
On Fri, 22 Oct 2021 04:04:13 GMT, Chris Plummer wrote: >> I have hoisted the locking from `trackAppResume()` up to `doPendingTasks()` >> where it is more visible. The change in that form is sufficient to fix the >> deadlock issues. Would it be ok for you to do further enhancements in a >> follow-up RFE? > > Yes, an RFE is fine. The changes look good. I just added one suggestion to > improve a comment. Thanks. I've created https://bugs.openjdk.java.net/browse/JDK-8275764. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v7]
On Thu, 21 Oct 2021 15:05:19 GMT, Chris Plummer wrote: >>> move the locking and unlocking into doPendingTasks(). At the start of the >>> node->handlingAppResume block, grab handlerLock and threadLock, and explain >>> that handlerLock is being grabbed because trackAppResume() will (indirectly) >>> try to grab it, and grabbing it before threadLock is necessary for lock >>> ordering. Then grab threadLock and hold onto it until the end of the block. >> >> So threadLock would be released at the end of the new block that depends on >> node->handlingAppResume. After that block follow accesses to >> `pendingInterrupt` >> and `pendingStop` that would be unsynchronized then. I think they do need to >> be >> synchronized through threadLock though. > > If you are worried about another thread changing those fields after they have > already been checked, then you can use the threadLock around them also. So > you can grab threadLock before the `if (node->handlingAppResume)` and release > after the `pendingInterrupt` and `pendingStop` references. I don't think it's > really needed, because this is the only place where the flags are set false > (and some action is taken when true), but there is no harm in holding the > threadLock here, and it doesn't make the locking any more complicated. I have hoisted the locking from `trackAppResume()` up to `doPendingTasks()` where it is more visible. The change in that form is sufficient to fix the deadlock issues. Would it be ok for you to do further enhancements in a follow-up RFE? - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v9]
> 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: Hoist locking from trackAppResume() up to it its caller doPendingTasks() - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/53d6b9de..c884a76e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5849=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5849=07-08 Stats: 28 lines in 1 file changed: 12 ins; 12 del; 4 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
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v7]
On Thu, 21 Oct 2021 03:34:30 GMT, Chris Plummer wrote:
>> You already release threadLock temporarily in `trackAppResume()` and you
>> used to release it temporarily in doPendingTasks(). I'm suggesting to
>> instead release it in `threadControl_onEventHandlerExit()` before calling
>> `doPendingTasks()` and then grab it again (after first grabbing the
>> handlerLock) in `doPendingTasks()` before calling `trackAppResume()`. What
>> can be modified in the ThreadNode during that time that you are concerned
>> about? threadLock would still be held when `blockOnDebuggerSuspend()` is
>> called. The only interesting lines that are currently executed while
>> holding the threadLock but no longer would be are:
>>
>> 2193 if (node->handlingAppResume) {
>> 2194 jthread resumer = node->thread;
>> 2195 jthread resumee = getResumee(resumer);
>>
>> I don't think any of those can change, because only the currently executing
>> thread can change them (when the Thread.resume() breakpoint is hit).
>>
>> The suspendCount is only meaningful when in `blockOnDebuggerSuspend()`, and
>> we'll be under the threadLock by that point.
>
> BTW, normally I would not suggest less locking simply because I *thought* it
> might not be needed. However, you already temporarily release and then regrab
> the lock. This means there is a period of time where the locking is not
> needed. What I've suggested is to better define (and expand) that period of
> time so the locking can be made cleaner. I think the actual expansion of time
> where the lock would no longer be held is pretty small, with a well defined
> set of code being executed that to me does not appear to require the lock,
> and if it did require the lock, then probably releasing the lock in the first
> place is a bug.
> move the locking and unlocking into doPendingTasks(). At the start of the
> node->handlingAppResume block, grab handlerLock and threadLock, and explain
> that handlerLock is being grabbed because trackAppResume() will (indirectly)
> try to grab it, and grabbing it before threadLock is necessary for lock
> ordering. Then grab threadLock and hold onto it until the end of the block.
So threadLock would be released at the end of the new block that depends on
node->handlingAppResume. After that block follow accesses to `pendingInterrupt`
and `pendingStop` that would be unsynchronized then. I think they do need to be
synchronized through threadLock though.
-
PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v8]
> 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: Improve comments. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/0b0fef0e..53d6b9de Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5849=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5849=06-07 Stats: 7 lines in 1 file changed: 4 ins; 0 del; 3 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
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v7]
On Tue, 19 Oct 2021 20:21:59 GMT, Chris Plummer wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Call blockOnDebuggerSuspend() after setup of the resumer tracking. > > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 772: > >> 770: debugMonitorExit(threadLock); >> 771: eventHandler_lock(); /* for proper lock order */ >> 772: debugMonitorEnter(threadLock); > > Somewhere we need to mention that `trackAppResume()` exits with threadLock > still being held, although with my recommended changes below this would no > longer be the case. If we cannot release `threadLock` before calling `doPendingTasks()` then I would suggest to comment that the caller of `trackAppResume()` is expected to hold `threadLock` and that it will be temporarily released. > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 807: > >> 805: } >> 806: >> 807: eventHandler_unlock(); > > This is still a bit odd looking because we grabbed this lock for lock > ordering purposes, but are now releasing it out of order. But it's starting > to sink in with me that the root of our problem here is that lock order > dictates grabbing handlerLock first, and we need to eventually wait on > threadLock but with handlerLock released, which suggest the lock ordering is > reversed. There is probably some design bug here that results in that, but > I'd hate to mess with the ordering of these two locks to try to fix it. Maybe > a 3rd lock would help (wait on some new lock instead of threadLock). We could > grab that lock first in `doPendingTasks()`, and not have to exit > `trackAppResume()` with threadLock held, but again this might be more risk > than it is worth. > > So it we aren't going to change the lock ordering or introduce a 3rd lock, > then my suggestion would be (after making the above suggested change to > release threadLock before calling `threadControl_onEventHandlerExit()`) to > move the locking and unlocking into `doPendingTasks()`. At the start of the > `node->handlingAppResume` block, grab handlerLock and threadLock, and explain > that handlerLock is being grabbed because trackAppResume() will (indirectly) > try to grab it, and grabbing it before threadLock is necessary for lock > ordering. Then grab threadLock and hold onto it until the end of the block. > Between the `trackAppResume()` and `blockOnDebuggerSuspend()` calls, release > handlerLock and explain that it will not be needed anymore, and has to be > released before calling `blockOnDebuggerSuspend()`. To me it does not look odd. I'd think it is ok to release locks out of order. IMHO adding a new third lock to be waited on in `doPendingTasks()` would make things even more complicated. Accesses to `suspendCount` are synchronized through `threadLock`. Waiting on that lock for change of `suspendCount` is straightforward really. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v7]
On Tue, 19 Oct 2021 20:17:59 GMT, Chris Plummer wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Call blockOnDebuggerSuspend() after setup of the resumer tracking. > > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 770: > >> 768: * handlerLock which has to be acquired before threadLock. >> 769: */ >> 770: debugMonitorExit(threadLock); > > I think we can avoid the exit here. threadLock was grabbed in > `threadControl_onEventHandlerExit()`. It probably should be released before > calling `doPendingTasks()`. My suggestion would be to first release it right > after the `findThread()` call, and then in the `ei == EI_THREAD_END` block > grab it again at the start of the block and release at the end of the block. But fields of ThreadNode "node" (aka "resumer") are read and also modified in `doPendingTasks()` and also in `threadControl_onEventHandlerExit()`. IMHO this needs to be synchronized with threadLock. At least it is not obvious that the synchronization is not needed. > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 771: > >> 769: */ >> 770: debugMonitorExit(threadLock); >> 771: eventHandler_lock(); /* for proper lock order */ > > "for proper lock order during eventHandler_createInternalThreadOnly() calls" Done. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Fri, 15 Oct 2021 11:52:09 GMT, Richard Reingruber wrote: > > > Hm, I think this can be simplified by swaping blockOnDebuggerSuspend() and > trackAppResume(). Can't try it today but will on Monday. I've done that with commit https://github.com/openjdk/jdk/pull/5849/commits/0b0fef0e6670c20a0e1e34323847c5a622878469. The locking is clearer I would say. Since `resumeFrameDepth` is now set before resumee's suspendCount is 0 we must block debugger suspends only if `!handlingAppResume` because then we know resumee's suspendCount actually is 0. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v7]
> 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: Call blockOnDebuggerSuspend() after setup of the resumer tracking. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/20f1f31b..0b0fef0e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5849=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5849=05-06 Stats: 95 lines in 2 files changed: 31 ins; 36 del; 28 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
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: 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=5849=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5849=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
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Wed, 13 Oct 2021 20:59:44 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. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v5]
> 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 two additional commits since the last revision: - Adding source filename and line numbers to printStack(). - Changes based on plummercj's comments. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/8c51e71f..806bceb8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5849=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5849=03-04 Stats: 24 lines in 2 files changed: 12 ins; 5 del; 7 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
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Tue, 12 Oct 2021 22:55:05 GMT, Chris Plummer wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Improve @summary section of test. > > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 2200: > >> 2198: /* trackAppResume() needs handlerLock */ >> 2199: debugMonitorExit(threadLock); >> 2200: eventHandler_lock(); /* for proper lock order */ > > Is it still necessary for the handlerLock to be held when calling > `blockOnDebuggerSuspend()` (presuming you don't also add the new handlerLock > related code in it)? It seems that only the threadLock is needed so it can > then wait on it. > > The main thing you've done to fix this issue is defer the > `blockOnDebuggerSuspend()` to be done after `event_callback()` has released > the handlerLock, and to make it so `blockOnDebuggerSuspend()` does not block > while holding the handlerLock, so is there really any reason to be grabbing > it at all? Please see my answer on your [comment for L2220](https://github.com/openjdk/jdk/pull/5849#discussion_r727573811) > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 2220: > >> 2218: * suspends a thread it will remain suspended. >> 2219: */ >> 2220: trackAppResume(resumer); > > `trackAppResume()` doesn't really need the handlerLock. However, it will grab > it when calling `eventHandler_createInternalThreadOnly()`. Since you want to > make sure it is grabbed before threadLock in order to preserve lock ordering, > that complicates things if we decided not to grab the handlerLock in the code > above. What I'm now thinking is all that is really needed is to hold the > threadLock around the call to `blockOnDebuggerSuspend()`, or better yet just > grab it from within `blockOnDebuggerSuspend()`. You probably don't need to do > anything with handlerLock or threadLock inside of `doPendingTasks()`. After returning from `blockOnDebuggerSuspend()` we have to make sure resumee cannot be suspended by JDWP means otherwise the current thread which is about to execute j.l.Thread.resume() will interfere with the debugger. This is achieved by holding threadLock until the tracking is established by `trackAppResume()`. For symmetry the set of owned locks should be equal before/after calling `blockOnDebuggerSuspend()` I think. Therefore handlerLock and threadLock are acquired before. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Wed, 13 Oct 2021 07:06:17 GMT, Chris Plummer wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Improve @summary section of test. > > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 750: > >> 748: * handlerLock and threadLock are owned when returning and the >> suspendCount of >> 749: * the given thread is 0. >> 750: */ > > How about: > > /* > * The caller must own handlerLock and threadLock. > * If the suspendCount of the given thread is greater than 0, then the > * current thread will release the handlerLock and wait on the threadLock. It > * must release the handlerLock first, because threadControl_resumeThread() > * and threadControl_resumeAll() need it, and calling them is how suspendCount > * will eventually be decremented to 0. > * handlerLock and threadLock are owned when returning and the suspendCount of > * the given thread is 0. > */ Reads better. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Tue, 12 Oct 2021 21:45:03 GMT, Chris Plummer wrote:
>> Richard Reingruber has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Improve @summary section of test.
>
> test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 100:
>
>> 98: // "resumee" is suspended now because of the breakpoint
>> 99: // Calling Thread.resume() will block this thread.
>> 100:
>
> no need for empty line here
Done.
> test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 176:
>
>> 174: mainThreadReturnedFromResumeCall = ((PrimitiveValue)
>> v).booleanValue();
>> 175: if (!resumedResumee) {
>> 176: // main thread should be still blocked.
>
> "...should still be blocked"
Done.
-
PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Tue, 12 Oct 2021 21:39:35 GMT, Chris Plummer wrote:
>> Richard Reingruber has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Improve @summary section of test.
>
> test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 159:
>
>> 157: ThreadReference resumee = bpe.thread();
>> 158: ObjectReference resumeeThreadObj =
>> resumee.frame(1).thisObject();
>> 159: printStack(resumee);
>
> As long as you're printing stacks, I think the stack of the main thread would
> be useful here, but you need to suspend it first. I don't think that
> interferes with the test.
> ```
> mainThread.suspend();
> printStack(mainThread);
> mainThread.resume();
I've added that.
> test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 167:
>
>> 165: log("Sleeping 500ms shows that the main thread is blocked
>> calling Thread.resume() on \"resumee\" Thread.");
>> 166: Thread.sleep(500);
>> 167: log("After sleep.");
>
> And after line 167 is also a good place to print the main thread's stack.
Ok.
-
PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Tue, 12 Oct 2021 21:35:59 GMT, Chris Plummer wrote:
>> Richard Reingruber has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Improve @summary section of test.
>
> test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 165:
>
>> 163: setField(resumeeThreadObj, "reachedBreakpoint",
>> vm().mirrorOf(true));
>> 164:
>> 165: log("Sleeping 500ms shows that the main thread is blocked
>> calling Thread.resume() on \"resumee\" Thread.");
>
> "shows" -> "so"
Ok.
-
PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Tue, 12 Oct 2021 21:06:00 GMT, Chris Plummer wrote:
>> Richard Reingruber has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Improve @summary section of test.
>
> test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 81:
>
>> 79: System.out.println();
>> 80: System.out.println("###(Target,"+ threadName +") " + m);
>> 81: System.out.println();
>
> I'm not sure why you have the two extra `println()` calls. Seems to just add
> unneeded blank lines in the log file.
I'll remove them.
I think I copied the method from another test where I used a lot of jit
compiler tracing.
> test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 222:
>
>> 220: System.out.println();
>> 221: System.out.println("###(Debugger) " + m);
>> 222: System.out.println();
>
> Same here with the extra `printlin()` calls
I'll remove them.
-
PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Tue, 12 Oct 2021 05:59:28 GMT, Chris Plummer wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Improve @summary section of test. > > test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 31: > >> 29: * the JDWP agent (in blockOnDebuggerSuspend()) because it >> called >> 30: * j.l.Thread.resume() on a thread R that was suspended by the >> 31: * debugger. > > This is hard to follow. Maybe instead document the steps the test takes. For > example maybe something like: Suspend Thread R via breakpoint. Thread T calls > j.l.Thread.resume() on Thread R, resulting in Thread T blocking in > blockOnDebuggerSuspend. Resume Thread R using ThreadReference.resume(). > Verify that Thread T is no longer blocked in blockOnDebuggerSuspend. > > Also, it would be nice if thread names in the description matched the names > used in the implementation. Thanks for looking at this. I've updated the test summary. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
> 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: Improve @summary section of test. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/e198e5ea..8c51e71f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5849=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5849=02-03 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 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
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v3]
> 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: Improve @summary section of test. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/6cc006b1..e198e5ea Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5849=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5849=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v2]
> 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: Improve @summary section of test. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/8c1b5cec..6cc006b1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5849=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5849=00-01 Stats: 30 lines in 1 file changed: 18 ins; 0 del; 12 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
RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend
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 - Commit messages: - Remove delay in cleanup again. - Fix that prevents deadlocks. - New test that deadlocks target VM calling ThreadReference.resume(). - Delay cleanup after JDWP Dispose command to trigger deadlock in the test dispose003 Changes: https://git.openjdk.java.net/jdk/pull/5849/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5849=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274687 Stats: 280 lines in 2 files changed: 262 ins; 13 del; 5 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
Integrated: 8274716: JDWP Spec: the description for the Dispose command confuses suspend with resume.
On Mon, 4 Oct 2021 13:44:44 GMT, Richard Reingruber wrote: > The following sentence in the JDWP Specification describing the Dispose > command confuses resume with suspend [1]: > > All threads suspended by the thread-level **resume** command or the VM-level > **resume** command are resumed as many times as necessary for them to run. > > It should be changed to > > All threads suspended by the thread-level **suspend** command or the > VM-level > **suspend** command are resumed as many times as necessary for them to run. > > [1] [JDWP Spec, Dispose Command > (JDK17).](https://docs.oracle.com/en/java/javase/17/docs/specs/jdwp/jdwp-protocol.html#JDWP_VirtualMachine_Dispose) This pull request has now been integrated. Changeset: 29dcbb72 Author:Richard Reingruber URL: https://git.openjdk.java.net/jdk/commit/29dcbb72a2d9b224203d92ad3224cf149a7d08de Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod 8274716: JDWP Spec: the description for the Dispose command confuses suspend with resume. Reviewed-by: alanb, cjplummer, sspitsyn - PR: https://git.openjdk.java.net/jdk/pull/5804
Re: RFR: 8274716: JDWP Spec: the description for the Dispose command confuses suspend with resume. [v2]
On Tue, 5 Oct 2021 07:38:28 GMT, Richard Reingruber wrote: >> The following sentence in the JDWP Specification describing the Dispose >> command confuses resume with suspend [1]: >> >> All threads suspended by the thread-level **resume** command or the >> VM-level >> **resume** command are resumed as many times as necessary for them to run. >> >> It should be changed to >> >> All threads suspended by the thread-level **suspend** command or the >> VM-level >> **suspend** command are resumed as many times as necessary for them to run. >> >> [1] [JDWP Spec, Dispose Command >> (JDK17).](https://docs.oracle.com/en/java/javase/17/docs/specs/jdwp/jdwp-protocol.html#JDWP_VirtualMachine_Dispose) > > Richard Reingruber has updated the pull request incrementally with one > additional commit since the last revision: > > Updated copyright. Thanks for the reviews! - PR: https://git.openjdk.java.net/jdk/pull/5804
Withdrawn: 8274687: JDWP can deadlock VM if Java thread waits in blockOnDebuggerSuspend
On Mon, 4 Oct 2021 14:07:18 GMT, Richard Reingruber wrote: > This change fixes the deadlock described in the JBS-bug by: > > * Releasing `handlerLock` before waiting on `threadLock` in > `blockOnDebuggerSuspend()` > > * Notifying on `threadLock` after resuming all threads in > `threadControl_reset()` > > The PR has 3 commits: > > The first commit delays the cleanup actions after leaving the loop in > `debugLoop_run()` because of a Dispose command. The delay 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 contains the fix described above. With it the deadlock > cannot be reproduced anymore. > > The third commit removes the diagnostic code introduced with the first commit > again. > > The fix passed our nightly regression testing: JCK and JTREG, also in Xcomp > mode with fastdebug and release builds on all platforms. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/5805
Re: RFR: 8274687: JDWP can deadlock VM if Java thread waits in blockOnDebuggerSuspend
On Mon, 4 Oct 2021 14:07:18 GMT, Richard Reingruber wrote: > This change fixes the deadlock described in the JBS-bug by: > > * Releasing `handlerLock` before waiting on `threadLock` in > `blockOnDebuggerSuspend()` > > * Notifying on `threadLock` after resuming all threads in > `threadControl_reset()` > > The PR has 3 commits: > > The first commit delays the cleanup actions after leaving the loop in > `debugLoop_run()` because of a Dispose command. The delay 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 contains the fix described above. With it the deadlock > cannot be reproduced anymore. > > The third commit removes the diagnostic code introduced with the first commit > again. > > The fix passed our nightly regression testing: JCK and JTREG, also in Xcomp > mode with fastdebug and release builds on all platforms. I've implemented a test that shows the jdwp agent deadlocks also trying to resume one/all threads if a thread is waiting in `blockOnDebuggerSuspend`. I will open a new pr that will include the test and a new attempt to fix the issues. - PR: https://git.openjdk.java.net/jdk/pull/5805
Re: RFR: 8274687: JDWP can deadlock VM if Java thread waits in blockOnDebuggerSuspend
On Tue, 5 Oct 2021 20:56:43 GMT, Chris Plummer wrote: >> Regarding threadControl_resumeThread() it does appear that it would block, >> as would threadControl_resumeAll(), which seems problematic in that >> blockOnDebuggerSuspend() won't exit until the suspendCount == 0. So it's >> unclear to me how this is suppose to work if a resume() can't be done. > > It seems if we had a test case where the debugger did a > ThreadReference.suspend(), then the debuggee did a Thread.resume() on the > same thread, and then debugger did a ThreadReference.resume(), it would > block, and with no way to unblock it. I'll see if I can implement such a test. Also I have I prototype that defers the actions of handleAppResumeBreakpoint() until doPendingTasks() is running. This seems to work in the various jdi/jdwp tests which likely do not cover the functionality very well though. - PR: https://git.openjdk.java.net/jdk/pull/5805
Re: RFR: 8274687: JDWP can deadlock VM if Java thread waits in blockOnDebuggerSuspend
On Tue, 5 Oct 2021 05:31:37 GMT, Chris Plummer wrote: > I'm not so sure this is always safe. It might be fine in the context of > resetting the connection, but not during normal debug agent operations. It > allows for another event to be processed when the lock is suppose to keep > event processing serialized. What happens for example if we hit the > `Thread.resume()` breakpoint again on a different thread? I agree that this is probably not safe. E.g. when releasing handlerLock the handler chain for EI_BREAKPOINT could be modified which is being iterated in the caller function event_callback(). https://github.com/openjdk/jdk/blob/8609ea55acdcc203408f58f7bf96ea9228aef613/src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.c#L647-L673 I should have checked before. > It might be best to limit doing this only when `threadControl_reset()` is > currently executing (you could set a flag there), although it seems more like > a band aid than a proper fix. I could imagine there might still be scenarios > were releasing the lock during reset might be problematic, although probably > extremely unlikely to ever be noticed. I looked at threadControl_resumeThread(). It appears to me that resuming a thread is not possible while some thread is waiting in blockOnDebuggerSuspend() because threadControl_resumeThread() locks handlerLock. https://github.com/openjdk/jdk/blob/32811026ce5ecb1d27d835eac33de9ccbd51fcbf/src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c#L1485 Am I missing something? Maybe the code in handleAppResumeBreakpoint() could moved to doPendingTasks()? - PR: https://git.openjdk.java.net/jdk/pull/5805
Re: RFR: 8274716: JDWP Spec: the description for the Dispose command confuses suspend with resume. [v2]
On Mon, 4 Oct 2021 18:20:42 GMT, Chris Plummer wrote: > > > Can you update the copyright please? Sure, thanks! > I checked the JDI spec and it looks correct there, which is actually > surprising since errors like this usually appear in both specs. Yes I noticed this too. Thanks for reviewing, Richard. - PR: https://git.openjdk.java.net/jdk/pull/5804
Re: RFR: 8274716: JDWP Spec: the description for the Dispose command confuses suspend with resume. [v2]
> The following sentence in the JDWP Specification describing the Dispose > command confuses resume with suspend [1]: > > All threads suspended by the thread-level **resume** command or the VM-level > **resume** command are resumed as many times as necessary for them to run. > > It should be changed to > > All threads suspended by the thread-level **suspend** command or the > VM-level > **suspend** command are resumed as many times as necessary for them to run. > > [1] [JDWP Spec, Dispose Command > (JDK17).](https://docs.oracle.com/en/java/javase/17/docs/specs/jdwp/jdwp-protocol.html#JDWP_VirtualMachine_Dispose) Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision: Updated copyright. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5804/files - new: https://git.openjdk.java.net/jdk/pull/5804/files/97396416..1d896c41 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5804=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5804=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5804.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5804/head:pull/5804 PR: https://git.openjdk.java.net/jdk/pull/5804
Re: RFR: 8274716: JDWP Spec: the description for the Dispose command confuses suspend with resume.
On Mon, 4 Oct 2021 14:22:50 GMT, Alan Bateman wrote: > > > It looks like this typo goes all the way back to JDK 1.2 but was not noticed. Thanks for reviewing. JDK 1.2 is surely long ago. Nevertheless not too many will have read that sentence since then I reckon ;) - PR: https://git.openjdk.java.net/jdk/pull/5804
RFR: 8274687: JDWP can deadlock VM if Java thread waits in blockOnDebuggerSuspend
This change fixes the deadlock described in the JBS-bug by: * Releasing `handlerLock` before waiting on `threadLock` in `blockOnDebuggerSuspend()` * Notifying on `threadLock` after resuming all threads in `threadControl_reset()` The PR has 3 commits: The first commit delays the cleanup actions after leaving the loop in `debugLoop_run()` because of a Dispose command. The delay 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 contains the fix described above. With it the deadlock cannot be reproduced anymore. The third commit removes the diagnostic code introduced with the first commit again. The fix passed our nightly regression testing: JCK and JTREG, also in Xcomp mode with fastdebug and release builds on all platforms. - Commit messages: - Remove delay in cleanup again. - Fix that prevents deadlock. - Delay cleanup after JDWP Dispose command to trigger deadlock in the test dispose003 Changes: https://git.openjdk.java.net/jdk/pull/5805/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5805=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274687 Stats: 11 lines in 1 file changed: 11 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5805.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5805/head:pull/5805 PR: https://git.openjdk.java.net/jdk/pull/5805
RFR: 8274716: JDWP Spec: the description for the Dispose command confuses suspend with resume.
The following sentence in the JDWP Specification describing the Dispose command confuses resume with suspend [1]: All threads suspended by the thread-level **resume** command or the VM-level **resume** command are resumed as many times as necessary for them to run. It should be changed to All threads suspended by the thread-level **suspend** command or the VM-level **suspend** command are resumed as many times as necessary for them to run. [1] [JDWP Spec, Dispose Command (JDK17).](https://docs.oracle.com/en/java/javase/17/docs/specs/jdwp/jdwp-protocol.html#JDWP_VirtualMachine_Dispose) - Commit messages: - Correct JDWP spec, Dispose command. Changes: https://git.openjdk.java.net/jdk/pull/5804/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5804=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274716 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5804.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5804/head:pull/5804 PR: https://git.openjdk.java.net/jdk/pull/5804
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v7]
On Tue, 25 May 2021 07:06:58 GMT, Robbin Ehn wrote: >> Please consider this change which removes the manual transitions to blocked. >> This adds a preprocess template/functor which is executed in the destructor >> of 'ThreadBlockInVM' if we are going to do any processing. >> This gives us a way to backout of the object/raw monitor before suspend or >> other processing, such as a safepoint. >> >> The object monitor code could be straight forward changed to use this >> instead of manual transitions. >> >> Raw monitors on the other hand are a bit more complicated due to 'implicit' >> rules (consequences of the specs). >> Added a comment in hpp trying to explain it; we cannot simply transition >> with a raw monitor held. >> This caused the change in the destructor ~ThreadInVMfromNative() (this >> specific change have also been tested in unrelated exploration of >> transition), now this RAII does the same as we do when going to native from >> Java, just setting the state. >> Since we are going from an unsafe state, in VM, to a safe state, in native, >> we do not need to check the poll. >> That made it possible to careful use ThreadInVMfromNative in raw monitors. >> >> I also remove the early CAS in raw_enter. >> We lock a lock to do a CAS, in the uncontended case means CAS on lock then >> CAS raw monitor. >> Now we instead do a transitions, in the uncontended case means fence, CAS >> raw monitor, fence. >> (multiple fence (CAS is also a fence) very close to each other have little >> additional performance impact on contemporary hardware) >> >> Passes t1-t7 and manual stressing relevant test groups. > > Robbin Ehn 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 10 additional commits since > the last revision: > > - Fixes for Dan > - Merge branch 'master' into 8265753 > - Final fixes: last famous words > - Review fixes 2 > - Merge branch 'master' into 8265753 > - Review fixes > - Merge branch 'master' into 8265753 > - Fixes for Dan > - Merge branch 'master' into 8265753 > - Removed manual transitions Still looks good to me. Cheer's, Richard. - PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v4]
On Wed, 19 May 2021 07:26:14 GMT, Robbin Ehn wrote: >> Please consider this change which removes the manual transitions to blocked. >> This adds a preprocess template/functor which is executed in the destructor >> of 'ThreadBlockInVM' if we are going to do any processing. >> This gives us a way to backout of the object/raw monitor before suspend or >> other processing, such as a safepoint. >> >> The object monitor code could be straight forward changed to use this >> instead of manual transitions. >> >> Raw monitors on the other hand are a bit more complicated due to 'implicit' >> rules (consequences of the specs). >> Added a comment in hpp trying to explain it; we cannot simply transition >> with a raw monitor held. >> This caused the change in the destructor ~ThreadInVMfromNative() (this >> specific change have also been tested in unrelated exploration of >> transition), now this RAII does the same as we do when going to native from >> Java, just setting the state. >> Since we are going from an unsafe state, in VM, to a safe state, in native, >> we do not need to check the poll. >> That made it possible to careful use ThreadInVMfromNative in raw monitors. >> >> I also remove the early CAS in raw_enter. >> We lock a lock to do a CAS, in the uncontended case means CAS on lock then >> CAS raw monitor. >> Now we instead do a transitions, in the uncontended case means fence, CAS >> raw monitor, fence. >> (multiple fence (CAS is also a fence) very close to each other have little >> additional performance impact on contemporary hardware) >> >> Passes t1-t7 and manual stressing relevant test groups. > > Robbin Ehn has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains five additional commits since > the last revision: > > - Review fixes > - Merge branch 'master' into 8265753 > - Fixes for Dan > - Merge branch 'master' into 8265753 > - Removed manual transitions Just one more, rather unimportant comment... Either way: LGTM! Thanks, Richard. src/hotspot/share/prims/jvmtiRawMonitor.cpp line 382: > 380: > 381: _recursions = 0; > 382: ret = simple_wait(self, millis); IMHO the guarantee at L379 is redundant with the newly added identical guarantee in `JvmtiRawMonitor::simple_wait()` at L240. In case you agree to remove the guarantee, I don't see why the following statements cannot be pulled out of the if-statement. _recursions = 0; ret = simple_wait(self, millis); _recursions = save; - Marked as reviewed by rrich (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v3]
On Wed, 12 May 2021 08:04:47 GMT, Robbin Ehn wrote: >> Please consider this change which removes the manual transitions to blocked. >> This adds a preprocess template/functor which is executed in the destructor >> of 'ThreadBlockInVM' if we are going to do any processing. >> This gives us a way to backout of the object/raw monitor before suspend or >> other processing, such as a safepoint. >> >> The object monitor code could be straight forward changed to use this >> instead of manual transitions. >> >> Raw monitors on the other hand are a bit more complicated due to 'implicit' >> rules (consequences of the specs). >> Added a comment in hpp trying to explain it; we cannot simply transition >> with a raw monitor held. >> This caused the change in the destructor ~ThreadInVMfromNative() (this >> specific change have also been tested in unrelated exploration of >> transition), now this RAII does the same as we do when going to native from >> Java, just setting the state. >> Since we are going from an unsafe state, in VM, to a safe state, in native, >> we do not need to check the poll. >> That made it possible to careful use ThreadInVMfromNative in raw monitors. >> >> I also remove the early CAS in raw_enter. >> We lock a lock to do a CAS, in the uncontended case means CAS on lock then >> CAS raw monitor. >> Now we instead do a transitions, in the uncontended case means fence, CAS >> raw monitor, fence. >> (multiple fence (CAS is also a fence) very close to each other have little >> additional performance impact on contemporary hardware) >> >> Passes t1-t7 and manual stressing relevant test groups. > > Robbin Ehn has updated the pull request incrementally with one additional > commit since the last revision: > > Fixes for Dan src/hotspot/share/prims/jvmtiRawMonitor.cpp line 395: > 393: ThreadBlockInVMPreprocess tbivmp(jt, eos); > 394: simple_enter(jt); > 395: _recursions = save; I think you should restore `_recursions` only when finally leaving the loop because if you do it here and exit the monitor again because of a suspend then another thread could hit the `guarantee(_recursions == 0, "invariant")` at L349. This is a preexisting issue but since you're modifying this I reckon it makes sense to correct it. - PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v3]
On Wed, 12 May 2021 08:04:47 GMT, Robbin Ehn wrote: >> Please consider this change which removes the manual transitions to blocked. >> This adds a preprocess template/functor which is executed in the destructor >> of 'ThreadBlockInVM' if we are going to do any processing. >> This gives us a way to backout of the object/raw monitor before suspend or >> other processing, such as a safepoint. >> >> The object monitor code could be straight forward changed to use this >> instead of manual transitions. >> >> Raw monitors on the other hand are a bit more complicated due to 'implicit' >> rules (consequences of the specs). >> Added a comment in hpp trying to explain it; we cannot simply transition >> with a raw monitor held. >> This caused the change in the destructor ~ThreadInVMfromNative() (this >> specific change have also been tested in unrelated exploration of >> transition), now this RAII does the same as we do when going to native from >> Java, just setting the state. >> Since we are going from an unsafe state, in VM, to a safe state, in native, >> we do not need to check the poll. >> That made it possible to careful use ThreadInVMfromNative in raw monitors. >> >> I also remove the early CAS in raw_enter. >> We lock a lock to do a CAS, in the uncontended case means CAS on lock then >> CAS raw monitor. >> Now we instead do a transitions, in the uncontended case means fence, CAS >> raw monitor, fence. >> (multiple fence (CAS is also a fence) very close to each other have little >> additional performance impact on contemporary hardware) >> >> Passes t1-t7 and manual stressing relevant test groups. > > Robbin Ehn has updated the pull request incrementally with one additional > commit since the last revision: > > Fixes for Dan > > > _Mailing list message from [David Holmes](mailto:david.hol...@oracle.com) on > [hotspot-runtime-dev](mailto:hotspot-runtime-...@mail.openjdk.java.net):_ > > On 12/05/2021 8:56 pm, Robbin Ehn wrote: > > > On Wed, 12 May 2021 08:27:33 GMT, Richard Reingruber > > wrote: > > > Hi Robbin, > > > I haven't found the time for a proper review yet but I've experimented a > > > little bit with lambdas. I could not make it work because g++ created > > > references to ::new which isn't allowed. > > > Thanks, Richard. > > > > > > Hi Richard, > > I tested lamdba, which is just a fancy way to write a crazy typed functor, > > we need to capture the lamdba so we can run it in the destructor. AFAICT > > the way to do that is using std::function. > > Regarding ThreadClosure, we could use it, maybe that is preferable?! > > Isn't a ThreadClosure for applying an operation to a set of threads? > That is not what we are doing here. No it isn't. A closure is just a set of variable bindings and a function that can be executed. And yes we're doing just that. A ThreadClosure is just an instance of this general concept. E.g. an AsyncHandshakeClosure (subclass of ThreadClosure) is a function (plus variable bindings) that is passed by a requesting thread to a target thread to be executed by it. Thanks, Richard. - PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v3]
On Wed, 12 May 2021 08:04:47 GMT, Robbin Ehn wrote:
>> Please consider this change which removes the manual transitions to blocked.
>> This adds a preprocess template/functor which is executed in the destructor
>> of 'ThreadBlockInVM' if we are going to do any processing.
>> This gives us a way to backout of the object/raw monitor before suspend or
>> other processing, such as a safepoint.
>>
>> The object monitor code could be straight forward changed to use this
>> instead of manual transitions.
>>
>> Raw monitors on the other hand are a bit more complicated due to 'implicit'
>> rules (consequences of the specs).
>> Added a comment in hpp trying to explain it; we cannot simply transition
>> with a raw monitor held.
>> This caused the change in the destructor ~ThreadInVMfromNative() (this
>> specific change have also been tested in unrelated exploration of
>> transition), now this RAII does the same as we do when going to native from
>> Java, just setting the state.
>> Since we are going from an unsafe state, in VM, to a safe state, in native,
>> we do not need to check the poll.
>> That made it possible to careful use ThreadInVMfromNative in raw monitors.
>>
>> I also remove the early CAS in raw_enter.
>> We lock a lock to do a CAS, in the uncontended case means CAS on lock then
>> CAS raw monitor.
>> Now we instead do a transitions, in the uncontended case means fence, CAS
>> raw monitor, fence.
>> (multiple fence (CAS is also a fence) very close to each other have little
>> additional performance impact on contemporary hardware)
>>
>> Passes t1-t7 and manual stressing relevant test groups.
>
> Robbin Ehn has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixes for Dan
Hi Robbin,
there seem to be issues in the jvmtiRawMonitor part of the change. Besides that
it looks good.
Cheers, Richard.
src/hotspot/share/prims/jvmtiRawMonitor.cpp line 47:
> 45: JavaThread* current_java_thread = JavaThread::current();
> 46: assert(current_java_thread->thread_state() == _thread_in_vm, "Must be
> in vm");
> 47: {
Looks like the assertion in L46 is redundant now. ThreadToNativeFromVM asserts
as well.
src/hotspot/share/prims/jvmtiRawMonitor.cpp line 386:
> 384: _waiters++;
> 385: ret = simple_wait(self, millis);
> 386: _waiters--;
We don't own the monitor yet so you cannot modify `_waiters` here.
Any reason you moved and duplicated this block?
src/hotspot/share/prims/jvmtiRawMonitor.cpp line 410:
> 408: ret = simple_wait(self, millis);
> 409: _waiters--;
> 410: _recursions = save;
The values of `_waiters` and `_recursions` should be adjusted after the monitor
is owned again.
src/hotspot/share/prims/jvmtiRawMonitor.hpp line 52:
> 50: // native for all operations. However we need to honor a suspend request,
> not
> 51: // entering a monitor if suspended, and check for interrupts. Honoring a
> suspend
> 52: // request// and reading the interrupt flag must be done from VM state
`s/request///request/`
-
Changes requested by rrich (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: JDK-8255661: TestHeapDumpOnOutOfMemoryError fails with EOFException [v2]
On Fri, 14 May 2021 07:14:07 GMT, Ralf Schmelter wrote: >> This fixes a race condition in the CompressionBackend class of the heap dump >> code. >> >> The race happens when the thread iterating the heap wants to write the data >> it has collected. If the compression backend has worker threads, the buffer >> to write would just be added to a queue and the worker threads would then >> compress (if needed) and write the buffer. But if no worker threads are >> present, the thread doing the iteration must do this itself. >> >> The iterating thread checks the _nr_of_threads member under lock protection >> and if it is 0, it assume it would have to do the work itself. It then >> releases the lock and enters the loop of the worker threads for one round. >> But after the lock has been released, a worker thread could be registered >> and handle the buffer itself. Then the iterating thread would wait until >> another buffer is available, which will never happen. >> >> The fix is to take the buffer to write out of the queue in the iterating >> thread under lock protection and the do the unlocking. > > Ralf Schmelter has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains six additional > commits since the last revision: > > - Simplify thread_loop() > - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8255661 > - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8255661 > - Fix punctuation > - Simplify code > - Fix race in heap dump compression backend LGTM - Marked as reviewed by rrich (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3628
Re: RFR: JDK-8255661: TestHeapDumpOnOutOfMemoryError fails with EOFException
On Thu, 22 Apr 2021 14:16:21 GMT, Ralf Schmelter wrote:
> This fixes a race condition in the CompressionBackend class of the heap dump
> code.
>
> The race happens when the thread iterating the heap wants to write the data
> it has collected. If the compression backend has worker threads, the buffer
> to write would just be added to a queue and the worker threads would then
> compress (if needed) and write the buffer. But if no worker threads are
> present, the thread doing the iteration must do this itself.
>
> The iterating thread checks the _nr_of_threads member under lock protection
> and if it is 0, it assume it would have to do the work itself. It then
> releases the lock and enters the loop of the worker threads for one round.
> But after the lock has been released, a worker thread could be registered and
> handle the buffer itself. Then the iterating thread would wait until another
> buffer is available, which will never happen.
>
> The fix is to take the buffer to write out of the queue in the iterating
> thread under lock protection and the do the unlocking.
Hi Ralf,
your change looks good to me.
Thanks for fixing,
Richard.
src/hotspot/share/services/heapDumperCompression.cpp line 262:
> 260: }
> 261:
> 262: void CompressionBackend::thread_loop() {
You could simplify `CompressionBackend::thread_loop()` further:
void CompressionBackend::thread_loop() {
{
MonitorLocker ml(_lock, Mutex::_no_safepoint_check_flag);
_nr_of_threads++;
}
WriteWork* work = get_work();
while (work != NULL) {
do_compress(work);
finish_work(work);
work = get_work();
}
MonitorLocker ml(_lock, Mutex::_no_safepoint_check_flag);
_nr_of_threads--;
assert(_nr_of_threads >= 0, "Too many threads finished");
ml.notify_all();
}
-
Marked as reviewed by rrich (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3628
Re: RFR: JDK-8255661: TestHeapDumpOnOutOfMemoryError fails with EOFException
On Wed, 12 May 2021 14:37:14 GMT, Richard Reingruber wrote:
>> This fixes a race condition in the CompressionBackend class of the heap dump
>> code.
>>
>> The race happens when the thread iterating the heap wants to write the data
>> it has collected. If the compression backend has worker threads, the buffer
>> to write would just be added to a queue and the worker threads would then
>> compress (if needed) and write the buffer. But if no worker threads are
>> present, the thread doing the iteration must do this itself.
>>
>> The iterating thread checks the _nr_of_threads member under lock protection
>> and if it is 0, it assume it would have to do the work itself. It then
>> releases the lock and enters the loop of the worker threads for one round.
>> But after the lock has been released, a worker thread could be registered
>> and handle the buffer itself. Then the iterating thread would wait until
>> another buffer is available, which will never happen.
>>
>> The fix is to take the buffer to write out of the queue in the iterating
>> thread under lock protection and the do the unlocking.
>
> src/hotspot/share/services/heapDumperCompression.cpp line 262:
>
>> 260: }
>> 261:
>> 262: void CompressionBackend::thread_loop() {
>
> You could simplify `CompressionBackend::thread_loop()` further:
>
>
> void CompressionBackend::thread_loop() {
> {
> MonitorLocker ml(_lock, Mutex::_no_safepoint_check_flag);
> _nr_of_threads++;
> }
>
> WriteWork* work = get_work();
> while (work != NULL) {
> do_compress(work);
> finish_work(work);
> work = get_work();
> }
>
> MonitorLocker ml(_lock, Mutex::_no_safepoint_check_flag);
> _nr_of_threads--;
> assert(_nr_of_threads >= 0, "Too many threads finished");
> ml.notify_all();
> }
BTW: why is `ml.notify_all()` in line 275 needed at all?
-
PR: https://git.openjdk.java.net/jdk/pull/3628
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v3]
On Wed, 12 May 2021 10:53:21 GMT, Robbin Ehn wrote: > > > > Hi Robbin, > > I haven't found the time for a proper review yet but I've experimented a > > little bit with lambdas. I could not make it work because g++ created > > references to ::new which isn't allowed. > > Thanks, Richard. > > Hi Richard, > I tested lamdba, which is just a fancy way to write a crazy typed functor, we > need to capture the lamdba so we can run it in the destructor. AFAICT the way > to do that is using std::function. Yes, I'd think so too. > Regarding ThreadClosure, we could use it, maybe that is preferable?! Personally I'd prefer it but I'm fine with the current version too. - PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v2]
On Wed, 12 May 2021 08:04:24 GMT, Robbin Ehn wrote: >> src/hotspot/share/runtime/interfaceSupport.inline.hpp line 230: >> >>> 228: }; >>> 229: >>> 230: template >> >> When you mentioned doing this with templates, I was having >> nightmares, but this one is not bad at all... > > :) Any reason why you don't use a ThreadClosure instead of the template parameter. I reckon it cannot be performance since it is only used on the slow path. - PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v3]
On Wed, 12 May 2021 08:04:47 GMT, Robbin Ehn wrote: >> Please consider this change which removes the manual transitions to blocked. >> This adds a preprocess template/functor which is executed in the destructor >> of 'ThreadBlockInVM' if we are going to do any processing. >> This gives us a way to backout of the object/raw monitor before suspend or >> other processing, such as a safepoint. >> >> The object monitor code could be straight forward changed to use this >> instead of manual transitions. >> >> Raw monitors on the other hand are a bit more complicated due to 'implicit' >> rules (consequences of the specs). >> Added a comment in hpp trying to explain it; we cannot simply transition >> with a raw monitor held. >> This caused the change in the destructor ~ThreadInVMfromNative() (this >> specific change have also been tested in unrelated exploration of >> transition), now this RAII does the same as we do when going to native from >> Java, just setting the state. >> Since we are going from an unsafe state, in VM, to a safe state, in native, >> we do not need to check the poll. >> That made it possible to careful use ThreadInVMfromNative in raw monitors. >> >> I also remove the early CAS in raw_enter. >> We lock a lock to do a CAS, in the uncontended case means CAS on lock then >> CAS raw monitor. >> Now we instead do a transitions, in the uncontended case means fence, CAS >> raw monitor, fence. >> (multiple fence (CAS is also a fence) very close to each other have little >> additional performance impact on contemporary hardware) >> >> Passes t1-t7 and manual stressing relevant test groups. > > Robbin Ehn has updated the pull request incrementally with one additional > commit since the last revision: > > Fixes for Dan Hi Robbin, I haven't found the time for a proper review yet but I've experimented a little bit with lambdas. I could not make it work because g++ created references to ::new which isn't allowed. Thanks, Richard. - PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8257831: Suspend with handshakes [v14]
On Wed, 21 Apr 2021 07:59:09 GMT, Robbin Ehn wrote:
>> A suspend request is done by handshaking thread target thread(s). When
>> executing the handshake operation we know the target mutator thread is in a
>> dormant state (as in safepoint safe state). We have a guarantee that it will
>> check it's poll before leaving the dormant state. To stop the thread from
>> leaving the the dormant state we install a second asynchronous handshake to
>> be executed by the targeted thread. The asynchronous handshake will wait on
>> a monitor while the thread is suspended. The target thread cannot not leave
>> the dormant state without a resume request.
>>
>> Per thread suspend requests are naturally serialized by the per thread
>> HandshakeState lock (we can only execute one handshake at a time per thread).
>> Instead of having a separate lock we use this to our advantage and use
>> HandshakeState lock for serializing access to the suspend flag and for
>> wait/notify.
>>
>> Suspend:
>> Requesting thread -> synchronous handshake -> target thread
>> Inside synchronus handshake (HandshakeState lock is locked while
>> executing any handshake):
>> - Set suspended flag
>> - Install asynchronous handshake
>>
>> Target thread -> tries to leave dormant state -> Executes handshakes
>> Target only executes asynchronous handshake:
>> - While suspended
>> - Go to blocked
>> - Wait on HandshakeState lock
>>
>> Resume:
>> Resuming thread:
>> - Lock HandshakeState lock
>> - Clear suspended flag
>> - Notify HandshakeState lock
>> - Unlock HandshakeState lock
>>
>> The "suspend requested" flag is an optimization, without it a dormant thread
>> could be suspended and resumed many times and each would add a new
>> asynchronous handshake. Suspend requested flag means there is already an
>> asynchronous suspend handshake in queue which can be re-used, only the
>> suspend flag needs to be set.
>>
>>
>> Some code can be simplified or done in a smarter way but I refrained from
>> doing such changes instead tried to keep existing code as is as far as
>> possible. This concerns especially raw monitors.
>>
>>
>> Regarding the changed test, the documentation says:
>> "If the calling thread is specified in the request_list array, this function
>> will not return until some other thread resumes it."
>>
>> But the code:
>> LOG("suspendTestedThreads: before JVMTI SuspendThreadList");
>> err = jvmti->SuspendThreadList(threads_count, threads, results);
>> ...
>> // Allow the Main thread to inspect the result of tested threads
>> suspension
>> agent_unlock(jni);
>>
>> The thread will never return from SuspendThreadList until resumed, so it
>> cannot unlock with agent_unlock().
>> Thus main thread is stuck forever on:
>> // Block until the suspender thread competes the tested threads suspension
>> agent_lock(jni);
>>
>> And never checks and resumes the threads. So I removed that lock instead
>> just sleep and check until all thread have the expected suspended state.
>>
>>
>>
>> This version already contains updates after pre-review comments from
>> @dcubed-ojdk, @pchilano, @coleenp.
>> (Pre-review comments here:
>> https://github.com/openjdk/jdk/pull/2625)
>>
>>
>> Testing t1-t8, nsk_jdi/nsk_jvmti/jdk_jdi/tck, KS24, RunThese and
>> combinations like running with -XX:ZCollectionInterval=0.01 -
>> XX:ZFragmentationLimit=0.
>> Running above some of above concurrently (load ~240), slow debug,
>> etc...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 20 commits:
>
> - Merge branch 'master' into SuspendInHandshake
> - Merge branch 'master' into SuspendInHandshake
> - Removed double tty lock unlocks
> - Fixed comment from Dan and fixed name of handshake
> - Merge branch 'master' into SuspendInHandshake
> - Fixed DavidH second review
> - Fixed nits
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 4
> - Fixed flag undef dep + spelling error
> - ... and 10 more:
> https://git.openjdk.java.net/jdk/compare/7146104f...ec344661
I've followed the discussion and the increments. Still looks very good to me
-
Marked as reviewed by rrich (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v10]
On Mon, 19 Apr 2021 05:48:44 GMT, David Holmes wrote: > It is still not at all clear to me what suspended has to do with the execution > of this method. The new code just ignores thread suspension. The caller enters a safe state. It can be suspended iff safe, so the old code checked for suspension. With the new code suspension is part of handshake processing, so it is sufficient to check for pending handshakes. > It seems to me that the old code could also have ignored suspension if the > checks in handle_special_runtime_exit_condition had be reordered. In that case JavaThread::java_suspend_self_with_safepoint_check() would have to be changed to check for `Thread::is_obj_deopt_suspend()` and handle it. - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v4]
On Mon, 12 Apr 2021 06:42:47 GMT, Robbin Ehn wrote: > > > Technically you are correct. > I have tested to remove this is previously version and all tests passes fine. > The reason I kept it is because the suspender have identified a thread for > suspension and deemed it suspendable, so we play nice. > To know why suspend failed the suspender must check if thread is exiting > after a failed suspend. (originally I had a bug here which caused me to > wrongfully introduce this from the beginning) Thanks for the explanation. As I understand you want to be fault tolerant towards somewhat sloppy agents. That's of course ok. It should be explained in a comment though. > I'll remove it, since it simplifies the code and David's comments about this > code is now out-of-line can be fixed. Also it does not eliminate the race as far as I can tell. - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v4]
On Thu, 8 Apr 2021 07:17:48 GMT, Robbin Ehn wrote:
>> A suspend request is done by handshaking thread target thread(s). When
>> executing the handshake operation we know the target mutator thread is in a
>> dormant state (as in safepoint safe state). We have a guarantee that it will
>> check it's poll before leaving the dormant state. To stop the thread from
>> leaving the the dormant state we install a second asynchronous handshake to
>> be executed by the targeted thread. The asynchronous handshake will wait on
>> a monitor while the thread is suspended. The target thread cannot not leave
>> the dormant state without a resume request.
>>
>> Per thread suspend requests are naturally serialized by the per thread
>> HandshakeState lock (we can only execute one handshake at a time per thread).
>> Instead of having a separate lock we use this to our advantage and use
>> HandshakeState lock for serializing access to the suspend flag and for
>> wait/notify.
>>
>> Suspend:
>> Requesting thread -> synchronous handshake -> target thread
>> Inside synchronus handshake (HandshakeState lock is locked while
>> executing any handshake):
>> - Set suspended flag
>> - Install asynchronous handshake
>>
>> Target thread -> tries to leave dormant state -> Executes handshakes
>> Target only executes asynchronous handshake:
>> - While suspended
>> - Go to blocked
>> - Wait on HandshakeState lock
>>
>> Resume:
>> Resuming thread:
>> - Lock HandshakeState lock
>> - Clear suspended flag
>> - Notify HandshakeState lock
>> - Unlock HandshakeState lock
>>
>> The "suspend requested" flag is an optimization, without it a dormant thread
>> could be suspended and resumed many times and each would add a new
>> asynchronous handshake. Suspend requested flag means there is already an
>> asynchronous suspend handshake in queue which can be re-used, only the
>> suspend flag needs to be set.
>>
>>
>> Some code can be simplified or done in a smarter way but I refrained from
>> doing such changes instead tried to keep existing code as is as far as
>> possible. This concerns especially raw monitors.
>>
>>
>> Regarding the changed test, the documentation says:
>> "If the calling thread is specified in the request_list array, this function
>> will not return until some other thread resumes it."
>>
>> But the code:
>> LOG("suspendTestedThreads: before JVMTI SuspendThreadList");
>> err = jvmti->SuspendThreadList(threads_count, threads, results);
>> ...
>> // Allow the Main thread to inspect the result of tested threads
>> suspension
>> agent_unlock(jni);
>>
>> The thread will never return from SuspendThreadList until resumed, so it
>> cannot unlock with agent_unlock().
>> Thus main thread is stuck forever on:
>> // Block until the suspender thread competes the tested threads suspension
>> agent_lock(jni);
>>
>> And never checks and resumes the threads. So I removed that lock instead
>> just sleep and check until all thread have the expected suspended state.
>>
>>
>>
>> This version already contains updates after pre-review comments from
>> @dcubed-ojdk, @pchilano, @coleenp.
>> (Pre-review comments here:
>> https://github.com/openjdk/jdk/pull/2625)
>>
>>
>> Testing t1-t8, nsk_jdi/nsk_jvmti/jdk_jdi/tck, KS24, RunThese and
>> combinations like running with -XX:ZCollectionInterval=0.01 -
>> XX:ZFragmentationLimit=0.
>> Running above some of above concurrently (load ~240), slow debug,
>> etc...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains six commits:
>
> - White space fixes
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes
> - Merge branch 'master' into SuspendInHandshake
> - Merge branch 'master' into SuspendInHandshake
> - 8257831: Suspend with handshake (review baseline)
Nice
Richard.
-
Marked as reviewed by rrich (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v4]
On Fri, 9 Apr 2021 16:12:16 GMT, Daniel D. Daugherty wrote:
>> src/hotspot/share/runtime/handshake.cpp line 630:
>>
>>> 628: // exiting.
>>> 629: }
>>> 630: }
>>
>> I need a little help learning the steps of this dance :)
>>
>> We reach here in _thread_in_vm. We cannot be suspended in this state. There
>> might be another thread waiting to handshake us to suspend us but why can't
>> we
>> just ignore that and do the `_handshakee->set_exiting();` even without
>> locking
>> HandshakeState::_lock?
>>
>> Adding a handshake operation is lock free, so even if the check
>> `SafepointMechanism::should_process(_handshakee)` in L619 returns false a new
>> operation to process could have been added concurrently such that
>> `SafepointMechanism::should_process(_handshakee)` would return true when we
>> execute `_handshakee->set_exiting();` in L620. What am I missing?
>
> `HandshakeState::suspend()` is a synchronous handshake and adding the
> handshake to the queue is lock free, but the execution of the synchronous
> handshake itself requires a `HandshakeState::claim_handshake()` call which
> does acquire the lock in question. We (the suspend requester) hold the lock
> while the handshake is being processed so we either detect that
> _handshakee->set_exiting() won the race (in the target thread) or we (the
> suspend requester) win the race of setting the suspend flag so the target
> thread can't exit yet.
>
> Hopefully that helps explain this dance.
Hi Dan,
thanks for picking up my question!
> `HandshakeState::suspend()` is a synchronous handshake and adding the
> handshake to the queue is lock free, but the execution of the synchronous
> handshake itself requires a `HandshakeState::claim_handshake()` call which
> does acquire the lock in question.
My point would be that the attempt to execute the synchronous handshake for
suspending a thread that is just about to call HandshakeState::thread_exit()
cannot make progress (blocks) while the target thread is not safe
(_thread_in_vm).
A synchronous handshake requires the target thread to be in a safe state for the
requester to execute the handshake operation. When executing
HandshakeState::thread_exit() the suspendee is _thread_in_vm. And the requester
will find it to be `_not_safe` when calling `possibly_can_process_handshake()`
before calling `HandshakeState::claim_handshake()` or when calling
`can_process_handshake()` afterwards. In both cases try_process() returns with
failure _not_safe and the lock is not held.
++
546 if (!possibly_can_process_handshake()) {
547// JT is observed in an unsafe state, it must notice the handshake
itself
548return HandshakeState::_not_safe;
549 }
550
551 // Claim the mutex if there still an operation to be executed.
552 if (!claim_handshake()) {
553return HandshakeState::_claim_failed;
554 }
555
556 // If we own the mutex at this point and while owning the mutex we
557 // can observe a safe state the thread cannot possibly continue
without
558 // getting caught by the mutex.
559 if (!can_process_handshake()) {
560_lock.unlock();
561return HandshakeState::_not_safe;
562 }
So isn't being unsafe sufficient to sync with suspend requests?
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v4]
On Thu, 8 Apr 2021 07:17:48 GMT, Robbin Ehn wrote:
>> A suspend request is done by handshaking thread target thread(s). When
>> executing the handshake operation we know the target mutator thread is in a
>> dormant state (as in safepoint safe state). We have a guarantee that it will
>> check it's poll before leaving the dormant state. To stop the thread from
>> leaving the the dormant state we install a second asynchronous handshake to
>> be executed by the targeted thread. The asynchronous handshake will wait on
>> a monitor while the thread is suspended. The target thread cannot not leave
>> the dormant state without a resume request.
>>
>> Per thread suspend requests are naturally serialized by the per thread
>> HandshakeState lock (we can only execute one handshake at a time per thread).
>> Instead of having a separate lock we use this to our advantage and use
>> HandshakeState lock for serializing access to the suspend flag and for
>> wait/notify.
>>
>> Suspend:
>> Requesting thread -> synchronous handshake -> target thread
>> Inside synchronus handshake (HandshakeState lock is locked while
>> executing any handshake):
>> - Set suspended flag
>> - Install asynchronous handshake
>>
>> Target thread -> tries to leave dormant state -> Executes handshakes
>> Target only executes asynchronous handshake:
>> - While suspended
>> - Go to blocked
>> - Wait on HandshakeState lock
>>
>> Resume:
>> Resuming thread:
>> - Lock HandshakeState lock
>> - Clear suspended flag
>> - Notify HandshakeState lock
>> - Unlock HandshakeState lock
>>
>> The "suspend requested" flag is an optimization, without it a dormant thread
>> could be suspended and resumed many times and each would add a new
>> asynchronous handshake. Suspend requested flag means there is already an
>> asynchronous suspend handshake in queue which can be re-used, only the
>> suspend flag needs to be set.
>>
>>
>> Some code can be simplified or done in a smarter way but I refrained from
>> doing such changes instead tried to keep existing code as is as far as
>> possible. This concerns especially raw monitors.
>>
>>
>> Regarding the changed test, the documentation says:
>> "If the calling thread is specified in the request_list array, this function
>> will not return until some other thread resumes it."
>>
>> But the code:
>> LOG("suspendTestedThreads: before JVMTI SuspendThreadList");
>> err = jvmti->SuspendThreadList(threads_count, threads, results);
>> ...
>> // Allow the Main thread to inspect the result of tested threads
>> suspension
>> agent_unlock(jni);
>>
>> The thread will never return from SuspendThreadList until resumed, so it
>> cannot unlock with agent_unlock().
>> Thus main thread is stuck forever on:
>> // Block until the suspender thread competes the tested threads suspension
>> agent_lock(jni);
>>
>> And never checks and resumes the threads. So I removed that lock instead
>> just sleep and check until all thread have the expected suspended state.
>>
>>
>>
>> This version already contains updates after pre-review comments from
>> @dcubed-ojdk, @pchilano, @coleenp.
>> (Pre-review comments here:
>> https://github.com/openjdk/jdk/pull/2625)
>>
>>
>> Testing t1-t8, nsk_jdi/nsk_jvmti/jdk_jdi/tck, KS24, RunThese and
>> combinations like running with -XX:ZCollectionInterval=0.01 -
>> XX:ZFragmentationLimit=0.
>> Running above some of above concurrently (load ~240), slow debug,
>> etc...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains six commits:
>
> - White space fixes
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes
> - Merge branch 'master' into SuspendInHandshake
> - Merge branch 'master' into SuspendInHandshake
> - 8257831: Suspend with handshake (review baseline)
Hi Robbin,
thanks for the update. This is almost ready to be pushed from my pov.
Richard.
src/hotspot/share/runtime/handshake.cpp line 630:
> 628: // exiting.
> 629: }
> 630: }
I need a little help learning the steps of this dance :)
We reach here in _thread_in_vm. We cannot be suspended in this state. There
might be another thread waiting to handshake us to suspend us but why can't we
just ignore that and do the `_handshakee->set_exiting();` even without locking
HandshakeState::_lock?
Adding a handshake operation is lock free, so even if the check
`SafepointMechanism::should_process(_handshakee)` in L619 returns false a new
operation to process could have been added concurrently such that
`SafepointMechanism::should_process(_handshakee)` would return true when we
execute `_handshakee->set_exiting();` in L620. What am I missing?
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v3]
On Wed, 7 Apr 2021 13:33:45 GMT, Robbin Ehn wrote:
>> src/hotspot/share/runtime/thread.cpp line 2102:
>>
>>> 2100: do {
>>> 2101: set_thread_state(_thread_blocked);
>>> 2102: // Check if _external_suspend was set in the previous loop
>>> iteration.
>>
>> Note that the comment for this method at line 1806 needs updating as it
>> refers to a now deleted method.
>
> I removed that line, not sure if the comment needs additional fixing.
I think David requested updating of the comment for
`JavaThread::java_suspend_self_with_safepoint_check()`.
I hope to delete the whole method soon and build on the new suspend mechanism,
if Robbin is ok with that. For now I'd like to propose the following:
// Wait for another thread to perform object reallocation and relocking on
behalf of
// this thread.
// Raw thread state transition to _thread_blocked and back again to the original
// state before returning are performed. The current thread is required to
// change to _thread_blocked in order to be seen to be safepoint/handshake safe
// whilst suspended and only after becoming handshake safe, the other thread can
// complete the handshake used to synchronize with this thread and then perform
// the reallocation and relocking. We cannot use the thread state transition
// helpers because we arrive here in various states and also because the helpers
// indirectly call this method. After leaving _thread_blocked we have to check
// for safepoint/handshake, except if _thread_in_native. The thread is safe
// without blocking then. Allowed states are enumerated in
// SafepointSynchronize::block(). See also EscapeBarrier::sync_and_suspend_*()
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v3]
On Wed, 7 Apr 2021 12:04:42 GMT, Robbin Ehn wrote: > Today the ThreadBlockInVM tbivm(current); is inside scope of > JavaThreadBlockedOnMonitorEnterState jtbmes(current, this);. > So this can happen today also. > > If you are context switch just before > current->set_current_pending_monitor(NULL);. > Someone suspends you and look at those states. > You mean the JVMTI agent suspends the current thread and then observes that the thread state first has the attribute JVMTI_THREAD_STATE_BLOCKED_ON_MONITOR_ENTER and in a later call it does not have it anymore (~ThreadBlockInVM doesn't check for suspend)? Yes that's problematic too. With the new code we could remain suspended with JVMTI_THREAD_STATE_BLOCKED_ON_MONITOR_ENTER. I think the OM would not be reported as owned monitor but another thread could not lock it. > If you agree that the issue is preexisting I prefer handling that outside > scope of this. I'm ok with that. A simple solution could then be making use of ThreadBlockInVM. When returning from EnterI in L413 we would set a rollback function in the HandshakeState which can be called in HandshakeState::suspend_in_handshake() to exit the OM. - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v2]
On Wed, 7 Apr 2021 06:51:22 GMT, Robbin Ehn wrote: >> But I still think we can just call is_suspended() here. If is_suspended() >> returns false, then Z hasn't finished suspending A yet (either async >> operation was still not added to the queue in case this is the first suspend >> for A, or do_handshake() hasn't yet finished for that SuspendThreadHandshake >> operation in case there was already an async operation in the queue). Since >> we have already acquired OM, then Z will not see that after being suspended >> A switched from not owning OM to owning it. It will always see that A owned >> OM. And A will still block for the suspension in the 'else' clause since >> poll is armed due to SuspendThreadHandshake operation(even if async >> operation was still not added to the queue at the time A called >> is_suspended()), but we wouldn't need to release OM. >> If is_suspended() returns true then at most we will have a false positive, >> since it could be that after the check somebody already resumed A, but that >> can also happen even with suspend_request_pending() right after releasing >> HandshakeState::_lock. >> Is that correct or am I missing something? > > I'm not saying it doesn't work, I'm saying it works by accident (I never > intended to be able to read the flag without HandshakeState lock). > So yes, if the flag is false (lockless read), it is not possible for the > suspend to have happened before we locked the OM. > And once poll is armed for the synchrone handshake it will stay armed until > after we executed the asynchrone handshake trap. > > What is possible is that we have armed for the suspend but not yet set the > flag. > If we consider the flag as the tipping point and not the start of execution > of the suspend sync handshake, we can do the lockless read. > But I still think we can just call is_suspended() here @pchilano, potentially I had similar thoughts when I asked about it before the long weekend: the current thread will suspend either way in the then or else block if the execution of the SuspendThreadHandshake was started while the current thread was _thread_blocked. If the suspend is ordered with operations on OM then is_suspended() will return true. Otherwise it is ok to become the owner. This is a more intuitive reasoning. I prefer yours. While looking at this I noticed that suspending in L428 could be problematic because the OM is just entered half way. Maybe we could use plain ThreadBlockInVM and deal with unlocking the OM very late just before waiting in HandshakeState::suspend_in_handshake(). - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v3]
On Wed, 31 Mar 2021 08:27:35 GMT, Robbin Ehn wrote:
>> A suspend request is done by handshaking thread target thread(s). When
>> executing the handshake operation we know the target mutator thread is in a
>> dormant state (as in safepoint safe state). We have a guarantee that it will
>> check it's poll before leaving the dormant state. To stop the thread from
>> leaving the the dormant state we install a second asynchronous handshake to
>> be executed by the targeted thread. The asynchronous handshake will wait on
>> a monitor while the thread is suspended. The target thread cannot not leave
>> the dormant state without a resume request.
>>
>> Per thread suspend requests are naturally serialized by the per thread
>> HandshakeState lock (we can only execute one handshake at a time per thread).
>> Instead of having a separate lock we use this to our advantage and use
>> HandshakeState lock for serializing access to the suspend flag and for
>> wait/notify.
>>
>> Suspend:
>> Requesting thread -> synchronous handshake -> target thread
>> Inside synchronus handshake (HandshakeState lock is locked while
>> executing any handshake):
>> - Set suspended flag
>> - Install asynchronous handshake
>>
>> Target thread -> tries to leave dormant state -> Executes handshakes
>> Target only executes asynchronous handshake:
>> - While suspended
>> - Go to blocked
>> - Wait on HandshakeState lock
>>
>> Resume:
>> Resuming thread:
>> - Lock HandshakeState lock
>> - Clear suspended flag
>> - Notify HandshakeState lock
>> - Unlock HandshakeState lock
>>
>> The "suspend requested" flag is an optimization, without it a dormant thread
>> could be suspended and resumed many times and each would add a new
>> asynchronous handshake. Suspend requested flag means there is already an
>> asynchronous suspend handshake in queue which can be re-used, only the
>> suspend flag needs to be set.
>>
>>
>> Some code can be simplified or done in a smarter way but I refrained from
>> doing such changes instead tried to keep existing code as is as far as
>> possible. This concerns especially raw monitors.
>>
>>
>> Regarding the changed test, the documentation says:
>> "If the calling thread is specified in the request_list array, this function
>> will not return until some other thread resumes it."
>>
>> But the code:
>> LOG("suspendTestedThreads: before JVMTI SuspendThreadList");
>> err = jvmti->SuspendThreadList(threads_count, threads, results);
>> ...
>> // Allow the Main thread to inspect the result of tested threads
>> suspension
>> agent_unlock(jni);
>>
>> The thread will never return from SuspendThreadList until resumed, so it
>> cannot unlock with agent_unlock().
>> Thus main thread is stuck forever on:
>> // Block until the suspender thread competes the tested threads suspension
>> agent_lock(jni);
>>
>> And never checks and resumes the threads. So I removed that lock instead
>> just sleep and check until all thread have the expected suspended state.
>>
>>
>>
>> This version already contains updates after pre-review comments from
>> @dcubed-ojdk, @pchilano, @coleenp.
>> (Pre-review comments here:
>> https://github.com/openjdk/jdk/pull/2625)
>>
>>
>> Testing t1-t8, nsk_jdi/nsk_jvmti/jdk_jdi/tck, KS24, RunThese and
>> combinations like running with -XX:ZCollectionInterval=0.01 -
>> XX:ZFragmentationLimit=0.
>> Running above some of above concurrently (load ~240), slow debug,
>> etc...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains three commits:
>
> - Merge branch 'master' into SuspendInHandshake
> - Merge branch 'master' into SuspendInHandshake
> - 8257831: Suspend with handshake (review baseline)
src/hotspot/share/runtime/objectMonitor.cpp line 428:
> 426: } else {
> 427: // Only exit path from for loop
> 428: SafepointMechanism::process_if_requested(current);
I think the current thread can suspend here in L428. This seems problematic as
entering OM has been started but not completed.
- The current thread is set as owner in ObjectMonitor::_owner
- The thread state will still be JVMTI_THREAD_STATE_BLOCKED_ON_MONITOR_ENTER
because of the JavaThreadBlockedOnMonitorEnterState in L389.
- Thread::_current_pending_monitor has not been reset to NULL.
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v2]
On Tue, 6 Apr 2021 10:05:08 GMT, Robbin Ehn wrote:
>> src/hotspot/share/prims/jvmtiRawMonitor.cpp line 364:
>>
>>> 362: for (;;) {
>>> 363: simple_enter(jt);
>>> 364: if (!SafepointMechanism::should_process(jt)) {
>>
>> It seems to be likely that the condition is false in the first loop
>> iteration and the thread has to do another iteration even if not suspended.
>> Wouldn't it be ok to break from the loop if `!jt->is_suspended()`?
>> I'm asking because in ObjectMonitor::enter() L414 there is similar code and
>> the condition there is `SafepointMechanism::should_process(current) &&
>> current->suspend_request_pending()`
>
> I didn't add that optimization here because I don't think it is needed, but I
> can add it.
Your version is good.
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v2]
On Tue, 6 Apr 2021 12:54:40 GMT, Robbin Ehn wrote:
>> The only reason _suspended is volatile is to be able to make the the fast
>> check in resume().
>> So disregard that early check and that it is volatile, the users of the flag
>> uses HandshakeState lock for synchronizing reads and stores to that flag.
>>
>> E.g
>> set_suspend(true);
>> set_async_suspend_handshake(true);
>> log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " suspended, arming
>> ThreadSuspension", p2i(_handshakee));
>> ThreadSuspensionHandshake* ts = new ThreadSuspensionHandshake();
>> Handshake::execute(ts, _handshakee);
>>
>> Before we have enqueued the async handshake there is no trap and but the
>> flag set_suspend(true).
>> This means we will miss the async handshake and continue to executed with
>> suspended flag.
>>
>> Both flags and async handshake are set atomically by serializing over the
>> HnadshakeState lock.
>>
>> In this case we want to both know if we are suspended if so process the
>> suspension.
>
> (technically this will be ordered by the poll is since polls are only
> disarmed by threads them selfs)
> (meaning if you really promise to call should_process() after you have seen
> "set_suspend(true);" you will see the async handshake since then we do take
> the HandshakeState lock.)
> Also when the suspend request happened while A was blocked then after
> current->set_thread_state_fence(_thread_blocked_trans); the check of
> is_suspended() will return true even if the handshake state lock is not
> acquired for the check
That statement of mine is wrong. (Hopefully) correct is that after the complete
state change from _thread_blocked to _thread_in_vm which includes being blocked
for a safepoint/handshake the current thread would be able to check
is_suspended() without holding the handshake state lock. It does not make a lot
of sense then because in an unsafe state JavaThread::current()->is_suspended()
will always return false.
> Thread A wait on OM X in blocked.
> Thread Z suspends Thread A, thread Z have no relation to OM X.
> Thread B unlocks OM X, thread B have no relation to the suspend request.
> Thread A locks OM X while blocked.
> Thread A was not allowed to lock OM X due to it is actually suspended.
> Thread A unlocks OM X and waits until resumed.
I understand that example now too: OM and suspend operations are unrelated. So I
thought it would be ok for A to enter OM X, but it is not. A thread must not
leave a safe state if it was suspended in that state (with a handshake). If it
did, e.g., its stack could become not walkable. And it must not enter the
monitor either.
Sorry for the confusion. The check is good.
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v2]
On Tue, 6 Apr 2021 11:56:36 GMT, Robbin Ehn wrote: > > > I do not follow, the OM is unrelated. > The suspender do not hold the OM. > > What happens is: > > * Thread A wait on OM X in blocked. > > * Thread Z suspends Thread A, thread Z have no relation to OM X. > > * Thread B unlocks OM X, thread B have no relation to the suspend request. > > * Thread A locks OM X while blocked. > > * Thread A was not allowed to lock OM X due to it is actually suspended. > > * Thread A unlocks OM X and waits until resumed. > > > So while A and B fight over OM X, A is trying to figure out if Z suspended > him while grabbing the lock. If understand the example correctly then the suspend operation and the operations on OM X are unordered with respect to each other. If so, then we can put them into an order where the suspend happens after "Thread A locks OM X while blocked.". We are free to do this; there's no synchronization that would prevent it. Also when the suspend request happened while A was blocked then after `current->set_thread_state_fence(_thread_blocked_trans);` the check of `is_suspended()` will return true even if the handshake state lock is not acquired for the check. And if Z tried to suspend A after the state change then Z will block because A is not safe anymore. Sorry for insisting. I really hope I'm not wrong ;) - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v2]
On Tue, 6 Apr 2021 11:28:26 GMT, Richard Reingruber wrote: >> To clarify, is_suspended() can only be checked when the JavaThread is >> _unsafe_, *after* you have transitioned from the safe state. >> In this case we are checking if we are suspended before we have completed >> the transition because we need to know if we should drop the OM lock. > >> >> >> We don't want to always to take a lock (handshake state lock), if the poll >> is disarmed there cannot be any suspend request. >> We only need to unlock the OM for suspends that happened before we grabbed >> the OM lock in EnterI(). >> >> When we leave EnterI we are still blocked, this means a thread might be >> processing the suspend handshake, which was emitted before we did EnterI(). >> To synchronize the suspend check with such a thread we need to grab the >> HandshakeState lock, e.g. we cannot racy check the suspended flag. > > I don't think that checking the suspended flag without holding the handshake > state lock would be too racy. Holding the OM is sufficient synchronization for > checking the suspended flag here. > > If the suspender thread S did the suspend request while holding the OM then > the > current thread T will see that it was suspended when it has entered the OM. > If S did the suspend without holding OM, only then the check would be racy > but that would be ok. > >> I choosed to check for an async suspension handshake that needs to be >> processed. (We can have such without being suspended). We could ignored that >> async handshake by just checking is_suspended, it would be processed in the >> else statement instead with >> "SafepointMechanism::process_if_requested(current);" instead. >> >> But we avoid releasing the OM lock in cases where we already have been >> resumed. >> >> So I can change to is_suspended inside the the method, but we still must do >> it under the HandshakeState lock. > > > To clarify, is_suspended() can only be checked when the JavaThread is > _unsafe_, _after_ you have transitioned from the safe state. For a minute I misunderstood this and thought you meant this as a general rule for calling `is_suspended()` while there are examples where it is called without any synchronization (e.g. in `JvmtiEnv::GetThreadState()`) which can be ok. > In this case we are checking if we are suspended before we have completed the > transition because we need to know if we should drop the OM lock. I think it is sufficient to hold the OM when calling `is_suspended` to decide if OM has to be dropped. - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v2]
On Tue, 6 Apr 2021 10:07:56 GMT, Robbin Ehn wrote: > > > Sorry, the PauseNoSafepointVerifier needs the nsv in it's constructor and > it's not in this scope. > I can't easily add that. Sorry, I overlooked that. - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v2]
On Tue, 6 Apr 2021 09:52:06 GMT, Robbin Ehn wrote: >> We don't want to always to take a lock (handshake state lock), if the poll >> is disarmed there cannot be any suspend request. >> We only need to unlock the OM for suspends that happened before we grabbed >> the OM lock in EnterI(). >> >> When we leave EnterI we are still blocked, this means a thread might be >> processing the suspend handshake, which was emitted before we did EnterI(). >> To synchronize the suspend check with such a thread we need to grab the >> HandshakeState lock, e.g. we cannot racy check the suspended flag. >> >> I choosed to check for an async suspension handshake that needs to be >> processed. (We can have such without being suspended). We could ignored that >> async handshake by just checking is_suspended, it would be processed in the >> else statement instead with >> "SafepointMechanism::process_if_requested(current);" instead. >> >> But we avoid releasing the OM lock in cases where we already have been >> resumed. >> >> So I can change to is_suspended inside the the method, but we still must do >> it under the HandshakeState lock. > > To clarify, is_suspended() can only be checked when the JavaThread is > _unsafe_, *after* you have transitioned from the safe state. > In this case we are checking if we are suspended before we have completed the > transition because we need to know if we should drop the OM lock. > > > We don't want to always to take a lock (handshake state lock), if the poll is > disarmed there cannot be any suspend request. > We only need to unlock the OM for suspends that happened before we grabbed > the OM lock in EnterI(). > > When we leave EnterI we are still blocked, this means a thread might be > processing the suspend handshake, which was emitted before we did EnterI(). > To synchronize the suspend check with such a thread we need to grab the > HandshakeState lock, e.g. we cannot racy check the suspended flag. I don't think that checking the suspended flag without holding the handshake state lock would be too racy. Holding the OM is sufficient synchronization for checking the suspended flag here. If the suspender thread S did the suspend request while holding the OM then the current thread T will see that it was suspended when it has entered the OM. If S did the suspend without holding OM, only then the check would be racy but that would be ok. > I choosed to check for an async suspension handshake that needs to be > processed. (We can have such without being suspended). We could ignored that > async handshake by just checking is_suspended, it would be processed in the > else statement instead with > "SafepointMechanism::process_if_requested(current);" instead. > > But we avoid releasing the OM lock in cases where we already have been > resumed. > > So I can change to is_suspended inside the the method, but we still must do > it under the HandshakeState lock. - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v2]
On Wed, 31 Mar 2021 09:18:56 GMT, David Holmes wrote: >> Another thread can win the race to suspend the current thread (if >> claim_handshake() in try_process() fails). Then JVMTI_ERROR_THREAD_SUSPENDED >> should be returned. > > My comment was about JVMTI_ERROR_THREAD_NOT_ALIVE Sure. I agree with your comment. - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v3]
On Wed, 31 Mar 2021 06:42:27 GMT, David Holmes wrote:
>> Robbin Ehn has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains three commits:
>>
>> - Merge branch 'master' into SuspendInHandshake
>> - Merge branch 'master' into SuspendInHandshake
>> - 8257831: Suspend with handshake (review baseline)
>
> src/hotspot/share/runtime/thread.cpp line 2105:
>
>> 2103: if (is_external_suspend()) {
>> 2104: java_suspend_self();
>> 2105: }
>
> It is not at all clear to me how this method should interact with thread
> suspension ??
In JavaThread::wait_for_object_deoptimization() the current thread can
transition to the safe state _thread_blocked. In that state it can be
suspended. In the baseline version wait_for_object_deoptimization() checks for
suspension explicitly and self suspends if positive. With this enhancement it
happens implicitly by calling SafepointMechanism::process_if_requested().
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v2]
On Wed, 31 Mar 2021 05:27:05 GMT, David Holmes wrote:
>> src/hotspot/share/runtime/handshake.cpp line 485:
>>
>>> 483: } else {
>>> 484: // Asynchronous may block so they may not execute
>>> ~PreserveExceptionMark before safepointing
>>> 485: // in outer loop.
>>
>> Sorry, I don't understand the comment.
>
> I think this relates to why the PEM was moved from the loop-scope to the sync
> op case only. That said it isn't clear why we need the HM or PEM.
I guess it should be "... must not execute ~PreserveExceptionMark ..."
~PreserveExceptionMark calls _thread->pending_exception() which is an oop and
that would be illegal as the vm could be at a safepoint when the async
handshake returns.
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v2]
On Wed, 31 Mar 2021 03:40:49 GMT, David Holmes wrote:
>> Robbin Ehn has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains two commits:
>>
>> - Merge branch 'master' into SuspendInHandshake
>> - 8257831: Suspend with handshake (review baseline)
>
> src/hotspot/share/prims/jvmtiEnv.cpp line 1009:
>
>> 1007: if (self_index >= 0) {
>> 1008: if (!JvmtiSuspendControl::suspend(current)) {
>> 1009: results[self_index] = JVMTI_ERROR_THREAD_NOT_ALIVE;
>
> Surely impossible when dealing with the current thread!
Another thread can win the race to suspend the current thread (if
claim_handshake() in try_process() fails). Then JVMTI_ERROR_THREAD_SUSPENDED
should be returned.
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v2]
On Tue, 30 Mar 2021 09:33:19 GMT, Richard Reingruber wrote:
>> Robbin Ehn has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains two commits:
>>
>> - Merge branch 'master' into SuspendInHandshake
>> - 8257831: Suspend with handshake (review baseline)
>
> src/hotspot/share/runtime/handshake.cpp line 617:
>
>> 615: {
>> 616: MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
>> 617: if (!is_suspend_requested()) {
>
> The current thread is _thread_in_vm. Can it be suspended then? For a suspend
> it has to be in a safe thread state which it cannot leave while suspended. So
> it must have reached _thread_in_vm not suspended and it cannot be suspended
> while in that state.
Shouldn't `is_suspend_requested()` be replaced with `is_suspended()`? See also
comment on declaration of `_suspend_requested`.
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v2]
On Fri, 26 Mar 2021 13:21:43 GMT, Robbin Ehn wrote:
>> A suspend request is done by handshaking thread target thread(s). When
>> executing the handshake operation we know the target mutator thread is in a
>> dormant state (as in safepoint safe state). We have a guarantee that it will
>> check it's poll before leaving the dormant state. To stop the thread from
>> leaving the the dormant state we install a second asynchronous handshake to
>> be executed by the targeted thread. The asynchronous handshake will wait on
>> a monitor while the thread is suspended. The target thread cannot not leave
>> the dormant state without a resume request.
>>
>> Per thread suspend requests are naturally serialized by the per thread
>> HandshakeState lock (we can only execute one handshake at a time per thread).
>> Instead of having a separate lock we use this to our advantage and use
>> HandshakeState lock for serializing access to the suspend flag and for
>> wait/notify.
>>
>> Suspend:
>> Requesting thread -> synchronous handshake -> target thread
>> Inside synchronus handshake (HandshakeState lock is locked while
>> executing any handshake):
>> - Set suspended flag
>> - Install asynchronous handshake
>>
>> Target thread -> tries to leave dormant state -> Executes handshakes
>> Target only executes asynchronous handshake:
>> - While suspended
>> - Go to blocked
>> - Wait on HandshakeState lock
>>
>> Resume:
>> Resuming thread:
>> - Lock HandshakeState lock
>> - Clear suspended flag
>> - Notify HandshakeState lock
>> - Unlock HandshakeState lock
>>
>> The "suspend requested" flag is an optimization, without it a dormant thread
>> could be suspended and resumed many times and each would add a new
>> asynchronous handshake. Suspend requested flag means there is already an
>> asynchronous suspend handshake in queue which can be re-used, only the
>> suspend flag needs to be set.
>>
>>
>> Some code can be simplified or done in a smarter way but I refrained from
>> doing such changes instead tried to keep existing code as is as far as
>> possible. This concerns especially raw monitors.
>>
>>
>> Regarding the changed test, the documentation says:
>> "If the calling thread is specified in the request_list array, this function
>> will not return until some other thread resumes it."
>>
>> But the code:
>> LOG("suspendTestedThreads: before JVMTI SuspendThreadList");
>> err = jvmti->SuspendThreadList(threads_count, threads, results);
>> ...
>> // Allow the Main thread to inspect the result of tested threads
>> suspension
>> agent_unlock(jni);
>>
>> The thread will never return from SuspendThreadList until resumed, so it
>> cannot unlock with agent_unlock().
>> Thus main thread is stuck forever on:
>> // Block until the suspender thread competes the tested threads suspension
>> agent_lock(jni);
>>
>> And never checks and resumes the threads. So I removed that lock instead
>> just sleep and check until all thread have the expected suspended state.
>>
>>
>>
>> This version already contains updates after pre-review comments from
>> @dcubed-ojdk, @pchilano, @coleenp.
>> (Pre-review comments here:
>> https://github.com/openjdk/jdk/pull/2625)
>>
>>
>> Testing t1-t8, nsk_jdi/nsk_jvmti/jdk_jdi/tck, KS24, RunThese and
>> combinations like running with -XX:ZCollectionInterval=0.01 -
>> XX:ZFragmentationLimit=0.
>> Running above some of above concurrently (load ~240), slow debug,
>> etc...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains two commits:
>
> - Merge branch 'master' into SuspendInHandshake
> - 8257831: Suspend with handshake (review baseline)
Hi Robbin,
this is a great simplification. Excellent work! :)
Thanks, Richard.
src/hotspot/share/prims/jvmtiRawMonitor.cpp line 318:
> 316: if (self->is_Java_thread()) {
> 317: jt = self->as_Java_thread();
> 318: while (true) {
>From the PR description:
> Some code can be simplified or done in a smarter way but I refrained from
> doing such changes instead tried to keep existing code as is as far as
> possible. This concerns especially raw monitors.
I read this section but then forgot about it. So you can skip the following
comment.
Possible follow-up: It could be worth the attempt to make use of the generic
state transition classes provided by interface.hpp. Directly I don't see why
this wouldn't be feasible and I doubt the [old comment in
JvmtiEnv::RawMonitorEnter()](https://github.com/reinrich/jdk/blob/aefc1560b51f0ce96d8f5ce396ba0d2fe08fd650/src/hotspot/share/prims/jvmtiEnv.cpp#L3208-L3214).
This would make the custom suspend logic here superfluous.
If ThreadBlockInVM was changed to take a generic callback for unlocking, then
this could replace the custom logic in L360-L374.
src/hotspot/share/prims/jvmtiRawMonitor.cpp line 323:
> 321: // Suspend request
Re: RFR: 8257831: Suspend with handshakes
On Fri, 26 Mar 2021 12:06:31 GMT, Robbin Ehn wrote: > > > > Hi Robbin, > > I think the preview/pre-review #2625 of this was affected by mailing-list > > hick-up as I've not received a notification about it and I could not find > > it in the archives either. Just wanted to let you know in case you aren't > > already aware. > > Looks promising though :) > > Hi @reinrich, I think that is because that PR was only in draft state. > Ah, alright, makes sense :) Thanks, Richard. - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes
On Thu, 25 Mar 2021 10:56:23 GMT, Robbin Ehn wrote:
> A suspend request is done by handshaking thread target thread(s). When
> executing the handshake operation we know the target mutator thread is in a
> dormant state (as in safepoint safe state). We have a guarantee that it will
> check it's poll before leaving the dormant state. To stop the thread from
> leaving the the dormant state we install a second asynchronous handshake to
> be executed by the targeted thread. The asynchronous handshake will wait on a
> monitor while the thread is suspended. The target thread cannot not leave the
> dormant state without a resume request.
>
> Per thread suspend requests are naturally serialized by the per thread
> HandshakeState lock (we can only execute one handshake at a time per thread).
> Instead of having a separate lock we use this to our advantage and use
> HandshakeState lock for serializing access to the suspend flag and for
> wait/notify.
>
> Suspend:
> Requesting thread -> synchronous handshake -> target thread
> Inside synchronus handshake (HandshakeState lock is locked while
> executing any handshake):
> - Set suspended flag
> - Install asynchronous handshake
>
> Target thread -> tries to leave dormant state -> Executes handshakes
> Target only executes asynchronous handshake:
> - While suspended
> - Go to blocked
> - Wait on HandshakeState lock
>
> Resume:
> Resuming thread:
> - Lock HandshakeState lock
> - Clear suspended flag
> - Notify HandshakeState lock
> - Unlock HandshakeState lock
>
> The "suspend requested" flag is an optimization, without it a dormant thread
> could be suspended and resumed many times and each would add a new
> asynchronous handshake. Suspend requested flag means there is already an
> asynchronous suspend handshake in queue which can be re-used, only the
> suspend flag needs to be set.
>
>
> Some code can be simplified or done in a smarter way but I refrained from
> doing such changes instead tried to keep existing code as is as far as
> possible. This concerns especially raw monitors.
>
>
> Regarding the changed test, the documentation says:
> "If the calling thread is specified in the request_list array, this function
> will not return until some other thread resumes it."
>
> But the code:
> LOG("suspendTestedThreads: before JVMTI SuspendThreadList");
> err = jvmti->SuspendThreadList(threads_count, threads, results);
> ...
> // Allow the Main thread to inspect the result of tested threads suspension
> agent_unlock(jni);
>
> The thread will never return from SuspendThreadList until resumed, so it
> cannot unlock with agent_unlock().
> Thus main thread is stuck forever on:
> // Block until the suspender thread competes the tested threads suspension
> agent_lock(jni);
>
> And never checks and resumes the threads. So I removed that lock instead just
> sleep and check until all thread have the expected suspended state.
>
>
>
> This version already contains updates after pre-review comments from
> @dcubed-ojdk, @pchilano, @coleenp.
> (Pre-review comments here:
> https://github.com/openjdk/jdk/pull/2625)
>
>
> Testing t1-t8, nsk_jdi/nsk_jvmti/jdk_jdi/tck, KS24, RunThese and
> combinations like running with -XX:ZCollectionInterval=0.01 -
> XX:ZFragmentationLimit=0.
> Running above some of above concurrently (load ~240), slow debug,
> etc...
Hi Robbin,
I think the preview/pre-review #2625 of this was affected by mailing-list
hick-up as I've not received a notification about it and I could not find it in
the archives either. Just wanted to let you know in case you aren't already
aware.
Looks promising though :)
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Fri, 5 Mar 2021 11:11:44 GMT, Alan Hayward wrote: >>> I was building this PR on a new machine, and I now get the following error: >>> >>> > /Users/alahay01/java/gerrit_jdk/src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_MidiUtils.c:258:31: >>> > error: cast to smaller integer type 'MIDIClientRef' (aka 'unsigned int') >>> > from 'void *' [-Werror,-Wvoid-pointer-to-int-cast] >>> > static MIDIClientRef client = (MIDIClientRef) NULL; >>> > ^~~~ >>> > /Users/alahay01/java/gerrit_jdk/src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_MidiUtils.c:259:29: >>> > error: cast to smaller integer type 'MIDIPortRef' (aka 'unsigned int') >>> > from 'void *' [-Werror,-Wvoid-pointer-to-int-cast] >>> > static MIDIPortRef inPort = (MIDIPortRef) NULL; >>> > ^~ >>> > /Users/alahay01/java/gerrit_jdk/src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_MidiUtils.c:260:30: >>> > error: cast to smaller integer type 'MIDIPortRef' (aka 'unsigned int') >>> > from 'void *' [-Werror,-Wvoid-pointer-to-int-cast] >>> > static MIDIPortRef outPort = (MIDIPortRef) NULL; >>> > ^~ >>> > /Users/alahay01/java/gerrit_jdk/src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_MidiUtils.c:466:32: >>> > error: cast to smaller integer type 'MIDIEndpointRef' (aka 'unsigned >>> > int') from 'void *' [-Werror,-Wvoid-pointer-to-int-cast] >>> > MIDIEndpointRef endpoint = (MIDIEndpointRef) NULL; >>> > ^~ >>> > 4 errors generated. >>> >>> As far as I can tell the only difference between the two systems is the >>> xcode version: >>> >>> New system (failing) >>> % xcodebuild -version >>> Xcode 12.5 >>> Build version 12E5244e >>> >>> Old system (working) >>> % xcodebuild -version >>> Xcode 12.4 >>> Build version 12D4e >>> >>> Looks like the newer version of Xcode is being a little stricter with >>> casting? >>> Replacing the NULL with 0 seems to fix the issue. >> >> Hello >> there is one issue with the info you provided, it's from Xcode12.5 beta. >> And beta license agreement forbids sharing output of beta version of >> compiler >> So we can't say we have issue with newer xcode beta until that beta went >> public & released. >> Fixing issues you found now will mean someone have violated xcode beta >> license agreement and made these issues public. > >> Hello >> there is one issue with the info you provided, it's from Xcode12.5 beta. >> And beta license agreement forbids sharing output of beta version of >> compiler >> So we can't say we have issue with newer xcode beta until that beta went >> public & released. >> Fixing issues you found now will mean someone have violated xcode beta >> license agreement and made these issues public. > > Ok, I wasn't aware of that. I'll downgrade back to the non-beta version. Hi, @VladimirKempik reported failure of CompressedClassPointers.largeHeapAbove32GTest() with [JDK-8262895](https://bugs.openjdk.java.net/browse/JDK-8262895) on macos_aarch64. He also helped analyzing the issue (thanks!). Apparently the vm has difficulties mapping the heap of 31GB at one of the preferred addresses given in [`get_attach_addresses_for_disjoint_mode()`](https://github.com/openjdk/jdk/blob/8d3de4b1bdb5dc13bb7724596dc2123ba05bbb81/src/hotspot/share/memory/virtualspace.cpp#L477). This causes the test failure indirectly. IMO it could be worth the effort to find out why the heap cannot be mapped at the preferred addresses. Reviewers should decide on it. I wouldn't be able to do it myself though as I don't have access to a macos_aarch64 system. Alternatively I'd suggest to exlude macos_aarch64 from the subtest largeHeapAbove32GTest. Best regards, Richard. -- Details of the analysis In the following trace we see the vm trying to allocate the heap at addresses given in [`get_attach_addresses_for_disjoint_mode()`](https://github.com/openjdk/jdk/blob/8d3de4b1bdb5dc13bb7724596dc2123ba05bbb81/src/hotspot/share/memory/virtualspace.cpp#L477) without success: images/jdk/bin/java -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions -Xmx31g -XX:-UseAOT -Xlog:gc+metaspace=trace,cds=trace,heap+gc+exit=info,gc+heap+coops=trace -Xshare:off -XX:+VerifyBeforeGC -XX:HeapSearchSteps=40 -version OpenJDK 64-Bit Server VM warning: Shared spaces are not supported in this VM [0.005s][trace][gc,heap,coops] Trying to allocate at address 0x0010 heap of size 0x7c100 [0.005s][trace][gc,heap,coops] Trying to allocate at address 0x0018 heap of size 0x7c100 [0.005s][trace][gc,heap,coops] Trying to allocate at address 0x0020 heap of size 0x7c100 [0.005s][trace][gc,heap,coops] Trying to allocate at address 0x0040 heap of size 0x7c100 [0.005s][trace][gc,heap,coops] Trying to allocate at address 0x0050 heap of size 0x7c100 [0.005s][trace][gc,heap,coops] Trying to allocate at address 0x0008 heap of size 0x7c100
Integrated: 8259627: Potential memory leaks in JVMTI after JDK-8227745
On Tue, 12 Jan 2021 19:09:43 GMT, Richard Reingruber wrote: > This change eliminates memory leaks in the JVMTI implementation reported by > SonarCloud. > > The leaks occur when the Java heap is exhausted. This pull request has now been integrated. Changeset: 6d4a593f Author:Richard Reingruber URL: https://git.openjdk.java.net/jdk/commit/6d4a593f Stats: 16 lines in 1 file changed: 8 ins; 8 del; 0 mod 8259627: Potential memory leaks in JVMTI after JDK-8227745 Reviewed-by: shade, stuefe, dholmes, sspitsyn - PR: https://git.openjdk.java.net/jdk/pull/2055
Re: RFR: 8259627: Potential memory leaks in JVMTI after JDK-8227745
On Wed, 13 Jan 2021 09:09:37 GMT, Serguei Spitsyn wrote: >> This change eliminates memory leaks in the JVMTI implementation reported by >> SonarCloud. >> >> The leaks occur when the Java heap is exhausted. > > Hi Richard, > LGTM++ > Thanks, > Serguei Thanks for the reviews David and Serguei. - PR: https://git.openjdk.java.net/jdk/pull/2055
Re: RFR: 8259627: Potential memory leaks in JVMTI after JDK-8227745
On Wed, 13 Jan 2021 08:42:59 GMT, Thomas Stuefe wrote: >> This change eliminates memory leaks in the JVMTI implementation reported by >> SonarCloud. >> >> The leaks occur when the Java heap is exhausted. > > Looks good. > > > > @shipilev thanks for doing the analysis and reporting the issues. I found 2 > > leaks. Do you see more that could be related to JDK-8227745 > > ([40f847e](https://github.com/openjdk/jdk/commit/40f847e2))? > > I wanted to do a SonarCloud scan myself but was uncertain about the > > requested permissions, me using the service for work, etc. Would it be > > possible to publish the SC report? > > I think those are only two instances. I am only aware of [this > one](https://sonarcloud.io/project/issues?fileUuids=AXaE0amt8L9hkQskFrwX=jdk=false=BUG). > I meant to set up my own some time later. Excellent! I googled for public scans but failed. Now I see that scans are listed on the "Explore" page of SonarCloud. Cool the tool found the leaks! Thanks for the review also. > > > Looks good. Thanks for looking! - PR: https://git.openjdk.java.net/jdk/pull/2055
Re: RFR: 8259627: Potential memory leaks in JVMTI after JDK-8227745
On Wed, 13 Jan 2021 08:19:59 GMT, Aleksey Shipilev wrote: >> This change eliminates memory leaks in the JVMTI implementation reported by >> SonarCloud. >> >> The leaks occur when the Java heap is exhausted. > > This looks good to me, thanks! @shipilev thanks for doing the analysis and reporting the issues. I found 2 leaks. Do you see more that could be related to JDK-8227745 (https://github.com/openjdk/jdk/commit/40f847e2)? I wanted to do a SonarCloud scan myself but was uncertain about the requested permissions, me using the service for work, etc. Would it be possible to publish the SC report? - PR: https://git.openjdk.java.net/jdk/pull/2055
RFR: 8259627: Potential memory leaks in JVMTI after JDK-8227745
This change eliminates memory leaks in the JVMTI implementation reported by SonarCloud. The leaks occur when the Java heap is exhausted. - Commit messages: - 8259627: Potential memory leaks in JVMTI after JDK-8227745 Changes: https://git.openjdk.java.net/jdk/pull/2055/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2055=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8259627 Stats: 16 lines in 1 file changed: 8 ins; 8 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2055.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2055/head:pull/2055 PR: https://git.openjdk.java.net/jdk/pull/2055
Withdrawn: 8255381: com/sun/jdi/EATests.java should not suspend graal threads
On Fri, 4 Dec 2020 15:30:15 GMT, Richard Reingruber wrote: > This fixes a bug in the test test/jdk/com/sun/jdi/EATests.java that caused > timeout failures when graal is enabled. > > The fix is to avoid suspending all threads when a breakpoint is reached and > then resume > just the main thread again. This pattern was used in the test case > EAMaterializeLocalAtObjectPollReturnReturn. It caused timeouts because graal > threads remained suspended and, running with -Xbatch, the main thread waited > (with timeout) for completion of compile tasks. > The fix was applied to all breakpoints in the test. All explicit suspend > calls now apply only > to the main test thread and all explicit resume calls apply to all java > threads. > > Testing: duration of the test case EAMaterializeLocalAtObjectPollReturnReturn > is > reduced from 30s to 10s. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/1625
Re: RFR: 8255381: com/sun/jdi/EATests.java should not suspend graal threads [v3]
On Fri, 11 Dec 2020 08:43:16 GMT, Richard Reingruber wrote: >> Marked as reviewed by cjplummer (Reviewer). > > @plummercj, @sspitsyn, thanks again for the review. > > I would like to bring the fix to jdk16. According to > https://openjdk.java.net/jeps/3 this is possible in RDP1 for test bug fixes > without approval. > > I'll try to open a PR against jdk16. Would be great if you could review that > one too. > > Thanks, Richard. I'm closing this PR since the change was integrated in jdk16 with openjdk/jdk16#7 - PR: https://git.openjdk.java.net/jdk/pull/1625
Re: [jdk16] RFR: 8255381: com/sun/jdi/EATests.java should not suspend graal threads
On Fri, 11 Dec 2020 20:53:37 GMT, Chris Plummer wrote: >> This is a clone of https://github.com/openjdk/jdk/pull/1625 which was >> reviewed but not integrated before RDP1 >> >> The change is a test bug fix which can be integrated during RDP1 according >> to https://openjdk.java.net/jeps/3 >> >> --- Original Synopsis >> >> This fixes a bug in the test test/jdk/com/sun/jdi/EATests.java that caused >> timeout failures when graal is enabled. >> >> The fix is to avoid suspending all threads when a breakpoint is reached and >> then resume >> just the main thread again. This pattern was used in the test case >> EAMaterializeLocalAtObjectPollReturnReturn. It caused timeouts because graal >> threads remained suspended and, running with -Xbatch, the main thread waited >> (with timeout) for completion of compile tasks. >> The fix was applied to all breakpoints in the test. All explicit suspend >> calls now apply only >> to the main test thread and all explicit resume calls apply to all java >> threads. >> >> Testing: duration of the test case >> EAMaterializeLocalAtObjectPollReturnReturn is >> reduced from 30s to 10s. > > Marked as reviewed by cjplummer (Reviewer). Thanks for the reviews @plummercj, @sspitsyn, @TheRealMDoerr! - PR: https://git.openjdk.java.net/jdk16/pull/7
[jdk16] Integrated: 8255381: com/sun/jdi/EATests.java should not suspend graal threads
On Fri, 11 Dec 2020 09:03:44 GMT, Richard Reingruber wrote: > This is a clone of https://github.com/openjdk/jdk/pull/1625 which was > reviewed but not integrated before RDP1 > > The change is a test bug fix which can be integrated during RDP1 according to > https://openjdk.java.net/jeps/3 > > --- Original Synopsis > > This fixes a bug in the test test/jdk/com/sun/jdi/EATests.java that caused > timeout failures when graal is enabled. > > The fix is to avoid suspending all threads when a breakpoint is reached and > then resume > just the main thread again. This pattern was used in the test case > EAMaterializeLocalAtObjectPollReturnReturn. It caused timeouts because graal > threads remained suspended and, running with -Xbatch, the main thread waited > (with timeout) for completion of compile tasks. > The fix was applied to all breakpoints in the test. All explicit suspend > calls now apply only > to the main test thread and all explicit resume calls apply to all java > threads. > > Testing: duration of the test case EAMaterializeLocalAtObjectPollReturnReturn > is > reduced from 30s to 10s. This pull request has now been integrated. Changeset: 09e8675f Author:Richard Reingruber URL: https://git.openjdk.java.net/jdk16/commit/09e8675f Stats: 90 lines in 2 files changed: 34 ins; 10 del; 46 mod 8255381: com/sun/jdi/EATests.java should not suspend graal threads Reviewed-by: cjplummer, mdoerr, sspitsyn - PR: https://git.openjdk.java.net/jdk16/pull/7
Re: [jdk16] RFR: 8255381: com/sun/jdi/EATests.java should not suspend graal threads
On Fri, 11 Dec 2020 09:03:44 GMT, Richard Reingruber wrote: > This is a clone of https://github.com/openjdk/jdk/pull/1625 which was > reviewed but not integrated before RDP1 > > The change is a test bug fix which can be integrated during RDP1 according to > https://openjdk.java.net/jeps/3 > > --- Original Synopsis > > This fixes a bug in the test test/jdk/com/sun/jdi/EATests.java that caused > timeout failures when graal is enabled. > > The fix is to avoid suspending all threads when a breakpoint is reached and > then resume > just the main thread again. This pattern was used in the test case > EAMaterializeLocalAtObjectPollReturnReturn. It caused timeouts because graal > threads remained suspended and, running with -Xbatch, the main thread waited > (with timeout) for completion of compile tasks. > The fix was applied to all breakpoints in the test. All explicit suspend > calls now apply only > to the main test thread and all explicit resume calls apply to all java > threads. > > Testing: duration of the test case EAMaterializeLocalAtObjectPollReturnReturn > is > reduced from 30s to 10s. @plummercj, @sspitsyn, I would like to bring this test bug fix to jdk16. It is identical to openjdk/jdk#1625. Thanks, Richard. - PR: https://git.openjdk.java.net/jdk16/pull/7
[jdk16] RFR: 8255381: com/sun/jdi/EATests.java should not suspend graal threads
This is a clone of https://github.com/openjdk/jdk/pull/1625 which was reviewed but not integrated before RDP1 The change is a test bug fix which can be integrated during RDP1 according to https://openjdk.java.net/jeps/3 --- Original Synopsis This fixes a bug in the test test/jdk/com/sun/jdi/EATests.java that caused timeout failures when graal is enabled. The fix is to avoid suspending all threads when a breakpoint is reached and then resume just the main thread again. This pattern was used in the test case EAMaterializeLocalAtObjectPollReturnReturn. It caused timeouts because graal threads remained suspended and, running with -Xbatch, the main thread waited (with timeout) for completion of compile tasks. The fix was applied to all breakpoints in the test. All explicit suspend calls now apply only to the main test thread and all explicit resume calls apply to all java threads. Testing: duration of the test case EAMaterializeLocalAtObjectPollReturnReturn is reduced from 30s to 10s. - Commit messages: - Only main thread needs to be resumed in EARelockingObjectCurrentlyWaitingOn. - Changes based on Chris Plummer's feedback. - 8255381: com/sun/jdi/EATests.java should not suspend graal threads Changes: https://git.openjdk.java.net/jdk16/pull/7/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk16=7=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255381 Stats: 90 lines in 2 files changed: 34 ins; 10 del; 46 mod Patch: https://git.openjdk.java.net/jdk16/pull/7.diff Fetch: git fetch https://git.openjdk.java.net/jdk16 pull/7/head:pull/7 PR: https://git.openjdk.java.net/jdk16/pull/7
Re: RFR: 8255381: com/sun/jdi/EATests.java should not suspend graal threads [v3]
On Tue, 8 Dec 2020 19:25:30 GMT, Chris Plummer wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Only main thread needs to be resumed in >> EARelockingObjectCurrentlyWaitingOn. > > Marked as reviewed by cjplummer (Reviewer). @plummercj, @sspitsyn, thanks again for the review. I would like to bring the fix to jdk16. According to https://openjdk.java.net/jeps/3 this is possible in RDP1 for test bug fixes without approval. I'll try to open a PR against jdk16. Would be great if you could review that one too. Thanks, Richard. - PR: https://git.openjdk.java.net/jdk/pull/1625
Re: RFR: 8255381: com/sun/jdi/EATests.java should not suspend graal threads [v3]
On Thu, 10 Dec 2020 02:08:38 GMT, Serguei Spitsyn wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Only main thread needs to be resumed in >> EARelockingObjectCurrentlyWaitingOn. > > Hi Richard, > The fix looks okay to me. > Thanks, > Serguei Hi Serguei, thanks a lot for reviewing this. Richard. - PR: https://git.openjdk.java.net/jdk/pull/1625
Re: RFR: 8255381: com/sun/jdi/EATests.java should not suspend graal threads [v3]
On Tue, 8 Dec 2020 19:25:30 GMT, Chris Plummer wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Only main thread needs to be resumed in >> EARelockingObjectCurrentlyWaitingOn. > > Marked as reviewed by cjplummer (Reviewer). Thanks for the review @plummercj - PR: https://git.openjdk.java.net/jdk/pull/1625
Re: RFR: 8255381: com/sun/jdi/EATests.java should not suspend graal threads [v3]
> This fixes a bug in the test test/jdk/com/sun/jdi/EATests.java that caused > timeout failures when graal is enabled. > > The fix is to avoid suspending all threads when a breakpoint is reached and > then resume > just the main thread again. This pattern was used in the test case > EAMaterializeLocalAtObjectPollReturnReturn. It caused timeouts because graal > threads remained suspended and, running with -Xbatch, the main thread waited > (with timeout) for completion of compile tasks. > The fix was applied to all breakpoints in the test. All explicit suspend > calls now apply only > to the main test thread and all explicit resume calls apply to all java > threads. > > Testing: duration of the test case EAMaterializeLocalAtObjectPollReturnReturn > is > reduced from 30s to 10s. Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision: Only main thread needs to be resumed in EARelockingObjectCurrentlyWaitingOn. - Changes: - all: https://git.openjdk.java.net/jdk/pull/1625/files - new: https://git.openjdk.java.net/jdk/pull/1625/files/8e4b301f..ce12877f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1625=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1625=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1625.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1625/head:pull/1625 PR: https://git.openjdk.java.net/jdk/pull/1625
