Re: RFR: 8288222: ProblemList serviceability/jvmti/vthread/VThreadNotifyFramePopTest/VThreadNotifyFramePopTest.java
On Fri, 10 Jun 2022 15:30:41 GMT, Ioi Lam wrote: >> A trivial fix to ProblemList >> serviceability/jvmti/vthread/VThreadNotifyFramePopTest/VThreadNotifyFramePopTest.java. > > Marked as reviewed by iklam (Reviewer). @iklam - Thanks for the fast review. I have got to learn to type! - PR: https://git.openjdk.org/jdk19/pull/4
Integrated: 8288222: ProblemList serviceability/jvmti/vthread/VThreadNotifyFramePopTest/VThreadNotifyFramePopTest.java
On Fri, 10 Jun 2022 15:21:34 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList > serviceability/jvmti/vthread/VThreadNotifyFramePopTest/VThreadNotifyFramePopTest.java. This pull request has now been integrated. Changeset: 0164145a Author:Daniel D. Daugherty URL: https://git.openjdk.org/jdk19/commit/0164145afc178b550313b80f5b5252b3bbff17a2 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod 8288222: ProblemList serviceability/jvmti/vthread/VThreadNotifyFramePopTest/VThreadNotifyFramePopTest.java Reviewed-by: alanb, iklam - PR: https://git.openjdk.org/jdk19/pull/4
Re: RFR: 8288222: ProblemList serviceability/jvmti/vthread/VThreadNotifyFramePopTest/VThreadNotifyFramePopTest.java
On Fri, 10 Jun 2022 15:27:09 GMT, Alan Bateman wrote: >> A trivial fix to ProblemList >> serviceability/jvmti/vthread/VThreadNotifyFramePopTest/VThreadNotifyFramePopTest.java. > > Marked as reviewed by alanb (Reviewer). @AlanBateman - Thanks for the fast review! - PR: https://git.openjdk.org/jdk19/pull/4
RFR: 8288222: ProblemList serviceability/jvmti/vthread/VThreadNotifyFramePopTest/VThreadNotifyFramePopTest.java
A trivial fix to ProblemList serviceability/jvmti/vthread/VThreadNotifyFramePopTest/VThreadNotifyFramePopTest.java. - Commit messages: - 8288222: ProblemList serviceability/jvmti/vthread/VThreadNotifyFramePopTest/VThreadNotifyFramePopTest.java Changes: https://git.openjdk.org/jdk19/pull/4/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk19=4=00 Issue: https://bugs.openjdk.org/browse/JDK-8288222 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk19/pull/4.diff Fetch: git fetch https://git.openjdk.org/jdk19 pull/4/head:pull/4 PR: https://git.openjdk.org/jdk19/pull/4
Re: RFR: 8287877: Exclude vmTestbase/nsk/jvmti/AttachOnDemand/attach022/TestDescription.java until JDK-8277573 is fixed
On Tue, 7 Jun 2022 00:49:29 GMT, Leonid Mesnik wrote: > Please review following fix which exclude failing test. Thumbs up. This is a trivial change. - Marked as reviewed by dcubed (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/9050
Re: RFR: 8287281: adjust guarantee in Handshake::execute for the case of target thread being current [v4]
On Mon, 6 Jun 2022 07:20:10 GMT, David Holmes wrote: >> src/hotspot/share/prims/jvmtiEventController.cpp line 370: >> >>> 368: } >>> 369: EnterInterpOnlyModeClosure hs; >>> 370: assert(state->get_thread() != NULL, "sanity check"); >> >> Existing: We have already performed this check. We set `target` above and >> returned if it was `NULL`. > > We also no longer need L358 as `current` is now unused. JavaThread *target = state->get_thread(); Thread *current = Thread::current(); assert(state != NULL, "sanity check"); The `assert()` on L360 is in the wrong place. If `state == NULL`, we would have crashed on L357 before we got to the assert(). This bug is pre-existing and not due to this PR. - PR: https://git.openjdk.java.net/jdk/pull/8992
Re: RFR: 8287281: adjust guarantee in Handshake::execute for the case of target thread being current [v4]
On Mon, 6 Jun 2022 07:17:15 GMT, David Holmes wrote:
>> Johan Sjölén has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - do_thread(target) not self
>> - Remove checks for is_handshake_for, instead call Handshake::execute
>> - Switch order of handshake check
>
> src/hotspot/share/prims/jvmtiEnvThreadState.cpp line 401:
>
>> 399: Thread *current = Thread::current();
>> 400: Handshake::execute(, thread);
>> 401: guarantee(op.completed(), "Handshake failed. Target thread is not
>> alive?");
>
> I much prefer that the current-thread case is internalised by
> `Handshake::execute` now. The code creating the handshake op shouldn't have
> to worry about current thread or not.
Having `Handshake::execute()` handle the current-thread case will certainly
allow us to make the code consistent in all the callers of
`Handshake::execute()`.
> src/hotspot/share/runtime/handshake.cpp line 357:
>
>> 355: if (target->is_handshake_safe_for(self)) {
>> 356: hs_cl->do_thread(target);
>> 357: return;
>
> I like the idea of doing this, but I can't quite convince myself that it will
> always be safe when the target is not the current thread. ??
Because we're pushing the special case handling for current-thread down
into the three parameter version of `Handshake::execute()`, we'll also
directly execute the closure's `do_thread()` function in other calls to the
three parameter version of `Handshake::execute()` where we didn't change
the calling code site in this patch:
- src/hotspot/share/classfile/javaClasses.cpp: async_get_stack_trace()
- src/hotspot/share/prims/jvmtiExtensions.cpp: GetCarrierThread()
- src/hotspot/share/prims/whitebox.cpp: WB_HandshakeReadMonitors(),
WB_HandshakeWalkStack()
- src/hotspot/share/runtime/handshake.cpp: execute(HandshakeClosure* hs_cl,
JavaThread* target)
Of course, since the two parameter version of `Handshake::execute()` is
now a changed code path, that means that all callers to the two parameter
version of `Handshake::execute()` are also affected. No, I'm not going to
list all those call sites.
This is a change in behavior and I'm not saying that this is wrong, but it's
not clear to me that the repercussions are understood and discussed in
this PR.
What I'm mumbling about here might be the same thing that @dholmes-ora is
worried about, but I'm just being more verbose about it. :-)
-
PR: https://git.openjdk.java.net/jdk/pull/8992
Re: RFR: 8287281: adjust guarantee in Handshake::execute for the case of target thread being current [v4]
On Fri, 3 Jun 2022 09:45:31 GMT, Johan Sjölén wrote:
>> Please review this PR for fixing JDK-8287281.
>>
>> If a thread is handshake safe we immediately execute the closure, instead of
>> going through the regular Handshake process.
>>
>> Finally: Should `VirtualThreadGetThreadClosure` and its `do_thread()` body
>> be inlined instead? We can do this in this PR, imho, but I'm hoping to get
>> some input on this.
>>
>>
>> Passes tier1. Running tier2-5.
>
> Johan Sjölén has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - do_thread(target) not self
> - Remove checks for is_handshake_for, instead call Handshake::execute
> - Switch order of handshake check
This bug/PR is specifically about this block of code:
if (tlh == nullptr) {
guarantee(Thread::is_JavaThread_protected_by_TLH(target),
"missing ThreadsListHandle in calling context.");
target->handshake_state()->add_operation();
and the bug makes the claim that we need to adjust this
guarantee(). Okay, but this proposed fix is indirectly changing
the guarantee() by inserting this block of code before the
guarantee():
if (target->is_handshake_safe_for(self)) {
hs_cl->do_thread(target);
return;
}
so we still have the original guarantee() that checks a
specific state with respect to ThreadsListHandles and
we replace it with a check, the `is_handshake_safe_for()`
call, that has nothing to do with ThreadsListHandles!
The original purpose of this logic block:
if (tlh == nullptr) {
guarantee(Thread::is_JavaThread_protected_by_TLH(target),
"missing ThreadsListHandle in calling context.");
target->handshake_state()->add_operation();
is to require a protecting ThreadsListHandle to be in place
*somewhere* in the calling context since we have not
passed in a ThreadsListHandle from the calling context.
When I added the above block of code, I intentionally
updated all of the call sites that reached the new strict
check with ThreadsListHandles. This included calls sites
where the caller was the current thread. This was an
intentional change on my part to make sure that all the
JavaThreads being operated (including current) on are
protected by ThreadsListHandles.
When the Loom project was being developed, a number
of these carefully placed ThreadsListHandles were moved
and unprotected code paths were introduced. We believe
that these unprotected code paths are safe because we
believe that they are only used by the current thread and
the current thread does not really need a ThreadsListHandle.
That might be true, but it certainly complicates the reasoning
about the code paths.
The bug talks about adjusting the guarantee() to allow the
current thread to be unprotected by a ThreadsListHandle, but
the logic that we have switched to:
// A JavaThread can always safely operate on it self and other threads
// can do it safely if they are the active handshaker.
bool is_handshake_safe_for(Thread* th) const {
return _handshake.active_handshaker() == th || this == th;
}
does more than that. It also allows the target to be unprotected
by a ThreadsListHandle if the calling thread is the active handshaker.
I'm not (yet) convinced that is a good policy.
-
Changes requested by dcubed (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8992
Integrated: 8231491: JDI tc02x004 failed again due to wrong # of breakpoints
On Fri, 3 Jun 2022 17:02:44 GMT, Daniel D. Daugherty wrote: > A trivial fix that deletes an errant `debugee.resume()` call that causes a > race > between the debugger and debuggee. I've also corrected incorrect line number > values mentioned in comments. > > This fix has been tested with the updated failing test both with and without > the > debug code that makes the failure easy to reproduce. The real test will be > seeing > if the failure stops reproducing in my stress testing runs on my M1 MacMini in > my lab. This pull request has now been integrated. Changeset: e2cfe2e1 Author:Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk/commit/e2cfe2e14a03b638a5828625975716f9fed1f668 Stats: 5 lines in 2 files changed: 0 ins; 1 del; 4 mod 8231491: JDI tc02x004 failed again due to wrong # of breakpoints Reviewed-by: cjplummer - PR: https://git.openjdk.java.net/jdk/pull/9020
Re: RFR: 8231491: JDI tc02x004 failed again due to wrong # of breakpoints
On Fri, 3 Jun 2022 22:08:50 GMT, Chris Plummer wrote: >> Marked as reviewed by cjplummer (Reviewer). > >> @plummercj - Thanks for the review! >> >> Do you agree that this is a trivial change? > > Yes @plummercj - Thanks! - PR: https://git.openjdk.java.net/jdk/pull/9020
Re: RFR: 8231491: JDI tc02x004 failed again due to wrong # of breakpoints
On Fri, 3 Jun 2022 21:43:02 GMT, Chris Plummer wrote: >> A trivial fix that deletes an errant `debugee.resume()` call that causes a >> race >> between the debugger and debuggee. I've also corrected incorrect line number >> values mentioned in comments. >> >> This fix has been tested with the updated failing test both with and without >> the >> debug code that makes the failure easy to reproduce. The real test will be >> seeing >> if the failure stops reproducing in my stress testing runs on my M1 MacMini >> in >> my lab. > > Marked as reviewed by cjplummer (Reviewer). @plummercj - Thanks for the review! Do you agree that this is a trivial change? - PR: https://git.openjdk.java.net/jdk/pull/9020
Re: RFR: 8231491: JDI tc02x004 failed again due to wrong # of breakpoints
On Fri, 3 Jun 2022 17:02:44 GMT, Daniel D. Daugherty wrote: > A trivial fix that deletes an errant `debugee.resume()` call that causes a > race > between the debugger and debuggee. I've also corrected incorrect line number > values mentioned in comments. > > This fix has been tested with the updated failing test both with and without > the > debug code that makes the failure easy to reproduce. The real test will be > seeing > if the failure stops reproducing in my stress testing runs on my M1 MacMini in > my lab. Did a Mach5 run that executed the updated test 50X on each of four platforms: - macosx-x64 - windows-x64 - linux-aarch64 - linux-x64 All runs passed. - PR: https://git.openjdk.java.net/jdk/pull/9020
RFR: 8231491: JDI tc02x004 failed again due to wrong # of breakpoints
A trivial fix that deletes an errant `debugee.resume()` call that causes a race between the debugger and debuggee. I've also corrected incorrect line number values mentioned in comments. This fix has been tested with the updated failing test both with and without the debug code that makes the failure easy to reproduce. The real test will be seeing if the failure stops reproducing in my stress testing runs on my M1 MacMini in my lab. - Commit messages: - 8231491.cr0.patch Changes: https://git.openjdk.java.net/jdk/pull/9020/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9020=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8231491 Stats: 5 lines in 2 files changed: 0 ins; 1 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/9020.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/9020/head:pull/9020 PR: https://git.openjdk.java.net/jdk/pull/9020
Re: RFR: 8231491: JDI tc02x004 failed again due to wrong # of breakpoints
On Fri, 3 Jun 2022 17:02:44 GMT, Daniel D. Daugherty wrote: > A trivial fix that deletes an errant `debugee.resume()` call that causes a > race > between the debugger and debuggee. I've also corrected incorrect line number > values mentioned in comments. > > This fix has been tested with the updated failing test both with and without > the > debug code that makes the failure easy to reproduce. The real test will be > seeing > if the failure stops reproducing in my stress testing runs on my M1 MacMini in > my lab. I've also looked at the other similar tests in vmTestbase/nsk/jdi/BScenarios and checked for the same errant `debugee.resume()` call. I didn't find any other cases. - PR: https://git.openjdk.java.net/jdk/pull/9020
Re: RFR: 8286960: Test serviceability/jvmti/vthread/SuspendResume2 crashed: missing ThreadsListHandle in calling context
On Wed, 25 May 2022 07:23:36 GMT, Serguei Spitsyn wrote:
> A part of this issue was contributed with the following changeset:
>
> commit ea23e7333e03abb4aca3e9f3854bab418a4b70e2
> Author: Daniel D. Daugherty <[dcu...@openjdk.org](mailto:dcu...@openjdk.org)>
> Date: Mon Nov 8 14:45:04 2021 +
>
> 8249004: Reduce ThreadsListHandle overhead in relation to direct
> handshakes
> Reviewed-by: coleenp, sspitsyn, dholmes, rehn
>
> The following change in `src/hotspot/share/runtime/thread.cpp` added new
> assert:
>
> bool JavaThread::java_suspend() {
> - ThreadsListHandle tlh;
> - if (!tlh.includes(this)) {
> - log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " not on
> ThreadsList, no suspension", p2i(this));
> - return false;
> - }
> + guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */ true),
> + "missing ThreadsListHandle in calling context.");
> return this->handshake_state()->suspend();
> }
>
> This new assert misses a check for target thread as being current
> `JavaThread`.
>
> Also, the JVMTI SuspendThread is protected with TLH:
>
> JvmtiEnv::SuspendThread(jthread thread) {
> JavaThread* current = JavaThread::current();
> ThreadsListHandle tlh(current); <= TLS defined here!!!
>
>oop thread_oop = NULL;
>{
> JvmtiVTMSTransitionDisabler disabler(true);
>
>
> However, it is possible that a new carrier thread (and an associated
> `JavaThread`) can be created after the `TLH` was set and the target virtual
> thread can be mounted on new carrier thread. Then target virtual thread will
> be associated with newly created `JavaThread` which is unprotected by the TLH.
> The right way to be protected from this situation it is to prevent mount
> state transitions with `JvmtiVTMSTransitionDisabler` before the TLH is set as
> in the change below:
>
>
> @@ -929,13 +929,13 @@ JvmtiEnv::GetAllThreads(jint* threads_count_ptr,
> jthread** threads_ptr) {
> jvmtiError
> JvmtiEnv::SuspendThread(jthread thread) {
>JavaThread* current = JavaThread::current();
> - ThreadsListHandle tlh(current);
>
>jvmtiError err;
>JavaThread* java_thread = NULL;
>oop thread_oop = NULL;
>{
> JvmtiVTMSTransitionDisabler disabler(true);
> +ThreadsListHandle tlh(current);
>
> err = get_threadOop_and_JavaThread(tlh.list(), thread, _thread,
> _oop);
> if (err != JVMTI_ERROR_NONE) {
>
>
>
> This problem exist in all JVMTI Suspend functions:
> `SuspendThread`, `SuspendThreadList` and `SuspendAllVirtualThreads`.
Before Loom, there was a TLH in the outer JVM/TI SuspendThread() entry point
so the current thread was already protected. Here's the relevant code from
build/macosx-x86_64-normal-server-release/hotspot/variant-server/gensrc/jvmtifiles/jvmtiEnter.cpp
in JDK18:
static jvmtiError JNICALL
jvmti_SuspendThread(jvmtiEnv* env,
jthread thread) {
#if !INCLUDE_JVMTI
return JVMTI_ERROR_NOT_AVAILABLE;
#else
if(!JvmtiEnv::is_vm_live()) {
return JVMTI_ERROR_WRONG_PHASE;
}
Thread* this_thread = Thread::current_or_null();
if (this_thread == NULL || !this_thread->is_Java_thread()) {
return JVMTI_ERROR_UNATTACHED_THREAD;
}
JavaThread* current_thread = JavaThread::cast(this_thread);
MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, current_thread));
ThreadInVMfromNative __tiv(current_thread);
VM_ENTRY_BASE(jvmtiError, jvmti_SuspendThread , current_thread)
debug_only(VMNativeEntryWrapper __vew;)
PreserveExceptionMark __em(this_thread);
JvmtiEnv* jvmti_env = JvmtiEnv::JvmtiEnv_from_jvmti_env(env);
if (!jvmti_env->is_valid()) {
return JVMTI_ERROR_INVALID_ENVIRONMENT;
}
if (jvmti_env->get_capabilities()->can_suspend == 0) {
return JVMTI_ERROR_MUST_POSSESS_CAPABILITY;
}
jvmtiError err;
JavaThread* java_thread = NULL;
ThreadsListHandle tlh(this_thread);
if (thread == NULL) {
java_thread = current_thread;
} else {
err = JvmtiExport::cv_external_thread_to_JavaThread(tlh.list(), thread,
_thread, NULL);
if (err != JVMTI_ERROR_NONE) {
return err;
}
}
err = jvmti_env->SuspendThread(java_thread);
return err;
#endif // INCLUDE_JVMTI
}
so it was definitely my intent that the current thread be protected
by the TLH that was in the entry code. Yes, that protection is not
needed, but that's the way I implemented it when
is_JavaThread_protected_by_TLH() was added to the system.
-
PR: https://git.openjdk.java.net/jdk/pull/8878
Integrated: 8283651: nsk/jvmti/SuspendThread/suspendthrd003 may leak native memory
On Mon, 9 May 2022 20:15:21 GMT, Daniel D. Daugherty wrote: > A trivial fix to nsk_jvmti_threadByName() to solve a native memory leak. > > This fix has been stress tested with my > StressWrapper60M_NMT_detail_suspendthrd003.java > test and I did NMT detailed monitoring for the whole 60M run. Total committed > memory has > gone down from 1.17GB to 300MB. Prior to my last rebase, it was down to 239MB. > > I ran the fix thru Mach5 Tier[1-7] with no failures. I just rebased the fix > to the latest jdk/jdk > and Mach5 Tier[1-3] have passed with no failures. Tier[45] are still running. This pull request has now been integrated. Changeset: 61450bb0 Author:Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk/commit/61450bb061ecda9700ddbd387a1f0659ebd1cced Stats: 7 lines in 1 file changed: 6 ins; 0 del; 1 mod 8283651: nsk/jvmti/SuspendThread/suspendthrd003 may leak native memory Reviewed-by: lmesnik - PR: https://git.openjdk.java.net/jdk/pull/8609
Re: RFR: 8283651: nsk/jvmti/SuspendThread/suspendthrd003 may leak native memory
On Mon, 9 May 2022 20:15:21 GMT, Daniel D. Daugherty wrote: > A trivial fix to nsk_jvmti_threadByName() to solve a native memory leak. > > This fix has been stress tested with my > StressWrapper60M_NMT_detail_suspendthrd003.java > test and I did NMT detailed monitoring for the whole 60M run. Total committed > memory has > gone down from 1.17GB to 300MB. Prior to my last rebase, it was down to 239MB. > > I ran the fix thru Mach5 Tier[1-7] with no failures. I just rebased the fix > to the latest jdk/jdk > and Mach5 Tier[1-3] have passed with no failures. Tier[45] are still running. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/8609
Re: RFR: 8283651: nsk/jvmti/SuspendThread/suspendthrd003 may leak native memory
On Mon, 9 May 2022 21:57:41 GMT, Leonid Mesnik wrote: >> A trivial fix to nsk_jvmti_threadByName() to solve a native memory leak. >> >> This fix has been stress tested with my >> StressWrapper60M_NMT_detail_suspendthrd003.java >> test and I did NMT detailed monitoring for the whole 60M run. Total >> committed memory has >> gone down from 1.17GB to 300MB. Prior to my last rebase, it was down to >> 239MB. >> >> I ran the fix thru Mach5 Tier[1-7] with no failures. I just rebased the fix >> to the latest jdk/jdk >> and Mach5 Tier[1-3] have passed with no failures. Tier[45] are still running. > > Marked as reviewed by lmesnik (Reviewer). @lmesnik - Thanks for the review! Do you agree that this is a trivial fix? - PR: https://git.openjdk.java.net/jdk/pull/8609
Re: RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee [v4]
On Thu, 5 May 2022 13:02:58 GMT, Jaroslav Bachorik wrote: >> A gist of the fix is to allow relaxed special handling of code blob lookup >> when done for ASGCT. >> >> Currently, a guarantee will fail when we happen to hit a zombie method which >> is still on stack. While this would indicate a serious error for the normal >> execution flow, in case of ASGCT being in progress when the executing thread >> can be expected at any possible method this is something which may happen >> and we really should not take the profiled JVM down due to it. >> >> >> Unfortunately, I am not able to create a simple reproducer for the crash >> other that testing in our production where the crash is happening >> sporadically. >> However, thanks to @parttimenerd and his [ASGCT stress >> test](https://github.com/parttimenerd/asgct2-tester.git) the problem can be >> reproduced quite reliably. >> >> >> >> _Note: This is a followup PR for #8061_ > > Jaroslav Bachorik has updated the pull request incrementally with one > additional commit since the last revision: > > AGCT -> ASGCT > I would like to disagree: The abbreviation "ASGCT" is used in multiple places > in the JVM, > especially in `forte.cpp` (where "AGCT" cannot be found). That was a very old mistake on my part. It should have been `AGCT` instead of `ASGCT` and I never got around to fixing that. - PR: https://git.openjdk.java.net/jdk/pull/8549
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which >> JDK version to target. >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e I've reviewed the JVM/TI, JDWP and JDI changes and I'm good with those. I haven't reviewed the JDB changes (forgot about those) and I have not (yet) reviewed the Runtime changes. - Marked as reviewed by dcubed (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8166
Integrated: 8284687: validate-source failure after JDK-8283710
A trivial copyright fix to solve validate-source failure after JDK-8283710. - Commit messages: - 8284687: validate-source failure after JDK-8283710 Changes: https://git.openjdk.java.net/jdk/pull/8181/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8181=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284687 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8181.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8181/head:pull/8181 PR: https://git.openjdk.java.net/jdk/pull/8181
Integrated: 8284687: validate-source failure after JDK-8283710
On Mon, 11 Apr 2022 16:20:02 GMT, Daniel D. Daugherty wrote: > A trivial copyright fix to solve validate-source failure after JDK-8283710. This pull request has now been integrated. Changeset: 470a6684 Author: Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk/commit/470a66840cda88d3be07f2b7c4c164c3265603e1 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8284687: validate-source failure after JDK-8283710 Reviewed-by: iris - PR: https://git.openjdk.java.net/jdk/pull/8181
Re: Integrated: 8284687: validate-source failure after JDK-8283710
On Mon, 11 Apr 2022 16:21:34 GMT, Iris Clark wrote: >> A trivial copyright fix to solve validate-source failure after JDK-8283710. > > Marked as reviewed by iris (Reviewer). @irisclark - Thanks for the lightning fast review! - PR: https://git.openjdk.java.net/jdk/pull/8181
Integrated: 8284015: ProblemList containers/docker/TestJcmd.java on linux-x64
On Wed, 30 Mar 2022 15:09:08 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList containers/docker/TestJcmd.java on linux-x64. This pull request has now been integrated. Changeset: d9d19e96 Author: Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk/commit/d9d19e96b1c453f2e52c83de61f593810a5ed1e3 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8284015: ProblemList containers/docker/TestJcmd.java on linux-x64 Reviewed-by: bpb, hseigel - PR: https://git.openjdk.java.net/jdk/pull/8042
Integrated: 8284015: ProblemList containers/docker/TestJcmd.java on linux-x64
A trivial fix to ProblemList containers/docker/TestJcmd.java on linux-x64. - Commit messages: - 8284015: ProblemList containers/docker/TestJcmd.java on linux-x64 Changes: https://git.openjdk.java.net/jdk/pull/8042/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8042=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284015 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8042.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8042/head:pull/8042 PR: https://git.openjdk.java.net/jdk/pull/8042
Re: Integrated: 8284015: ProblemList containers/docker/TestJcmd.java on linux-x64
On Wed, 30 Mar 2022 15:11:36 GMT, Brian Burkhalter wrote: >> A trivial fix to ProblemList containers/docker/TestJcmd.java on linux-x64. > > Marked as reviewed by bpb (Reviewer). @bplb and @hseigel - Thanks for the fast reviews! - PR: https://git.openjdk.java.net/jdk/pull/8042
Re: RFR: 8283587: [BACKOUT] Invalid generic signature for redefined classes
On Thu, 24 Mar 2022 01:31:42 GMT, Alex Menkov wrote: > The change reverts the fix for JDK-8282241 which causes regression Thumbs up. Looks like a clean [BACKOUT] so this is a trivial fix. - Marked as reviewed by dcubed (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7934
Integrated: 8282314: nsk/jvmti/SuspendThread/suspendthrd003 may leak memory
On Wed, 9 Mar 2022 20:41:37 GMT, Daniel D. Daugherty wrote:
> A trivial fix to solve a memory leak/memory pinning for long runs of
> suspendthrd003.
>
> See the bug report for the gory analysis details.
>
> This fix was included in my jdk-19+12 stress runs so the updated test was
> executed
> in the NSK JVM/TI testsuite for 3 runs in {fastdebug,release,slowdebug}
> configs. The
> test was also executed in parallel for 3 runs with my
> StressWrapper_StopAtExit config
> for 101 minutes in {fastdebug,release,slowdebug} configs. Lastly, the updated
> test was
> tested with Mach5 Tier[1567] with no failures on any platform.
This pull request has now been integrated.
Changeset: bb7ee5a0
Author:Daniel D. Daugherty
URL:
https://git.openjdk.java.net/jdk/commit/bb7ee5a04ae21a9f9dc6c59a990f7e571e832f0d
Stats: 12 lines in 2 files changed: 10 ins; 0 del; 2 mod
8282314: nsk/jvmti/SuspendThread/suspendthrd003 may leak memory
Reviewed-by: dholmes, cjplummer, amenkov, lmesnik, mseledtsov
-
PR: https://git.openjdk.java.net/jdk/pull/7764
Re: RFR: 8282314: nsk/jvmti/SuspendThread/suspendthrd003 may leak memory
On Thu, 10 Mar 2022 18:11:58 GMT, Mikhailo Seledtsov
wrote:
>> A trivial fix to solve a memory leak/memory pinning for long runs of
>> suspendthrd003.
>>
>> See the bug report for the gory analysis details.
>>
>> This fix was included in my jdk-19+12 stress runs so the updated test was
>> executed
>> in the NSK JVM/TI testsuite for 3 runs in {fastdebug,release,slowdebug}
>> configs. The
>> test was also executed in parallel for 3 runs with my
>> StressWrapper_StopAtExit config
>> for 101 minutes in {fastdebug,release,slowdebug} configs. Lastly, the
>> updated test was
>> tested with Mach5 Tier[1567] with no failures on any platform.
>
> Marked as reviewed by mseledtsov (Committer).
@mseledts - Thanks for the review.
-
PR: https://git.openjdk.java.net/jdk/pull/7764
Re: RFR: 8282314: nsk/jvmti/SuspendThread/suspendthrd003 may leak memory
On Thu, 10 Mar 2022 16:17:22 GMT, Leonid Mesnik wrote:
>> A trivial fix to solve a memory leak/memory pinning for long runs of
>> suspendthrd003.
>>
>> See the bug report for the gory analysis details.
>>
>> This fix was included in my jdk-19+12 stress runs so the updated test was
>> executed
>> in the NSK JVM/TI testsuite for 3 runs in {fastdebug,release,slowdebug}
>> configs. The
>> test was also executed in parallel for 3 runs with my
>> StressWrapper_StopAtExit config
>> for 101 minutes in {fastdebug,release,slowdebug} configs. Lastly, the
>> updated test was
>> tested with Mach5 Tier[1567] with no failures on any platform.
>
> Marked as reviewed by lmesnik (Reviewer).
@lmesnik - Thanks for the review.
-
PR: https://git.openjdk.java.net/jdk/pull/7764
Re: RFR: 8282314: nsk/jvmti/SuspendThread/suspendthrd003 may leak memory
On Wed, 9 Mar 2022 20:41:37 GMT, Daniel D. Daugherty wrote:
> A trivial fix to solve a memory leak/memory pinning for long runs of
> suspendthrd003.
>
> See the bug report for the gory analysis details.
>
> This fix was included in my jdk-19+12 stress runs so the updated test was
> executed
> in the NSK JVM/TI testsuite for 3 runs in {fastdebug,release,slowdebug}
> configs. The
> test was also executed in parallel for 3 runs with my
> StressWrapper_StopAtExit config
> for 101 minutes in {fastdebug,release,slowdebug} configs. Lastly, the updated
> test was
> tested with Mach5 Tier[1567] with no failures on any platform.
@lmesnik and @mseledts - Can you guys take a look when you get the chance?
I'm making a minor addition to the NSK Log class.
-
PR: https://git.openjdk.java.net/jdk/pull/7764
Re: RFR: 8282314: nsk/jvmti/SuspendThread/suspendthrd003 may leak memory
On Thu, 10 Mar 2022 04:52:46 GMT, David Holmes wrote: >> The fix looks fine, but what about StopAtExit? That test seemed to use a lot >> more memory than suspendthrd003. > > @plummercj - see https://bugs.openjdk.java.net/browse/JDK-8282704 for > StopAtExit. @dholmes-ora, @plummercj and @alexmenkov - Thanks for the reviews! One clarification: > and only print them if an error actually occurs Log will also print them if the mode is set to verbose. I manually verified my fix by setting the log to verbose at the end of the test run and I verified that I only had logging lines for the last loop rather than all gazillion of them... - PR: https://git.openjdk.java.net/jdk/pull/7764
RFR: 8282314: nsk/jvmti/SuspendThread/suspendthrd003 may leak memory
A trivial fix to solve a memory leak/memory pinning for long runs of
suspendthrd003.
See the bug report for the gory analysis details.
This fix was included in my jdk-19+12 stress runs so the updated test was
executed
in the NSK JVM/TI testsuite for 3 runs in {fastdebug,release,slowdebug}
configs. The
test was also executed in parallel for 3 runs with my StressWrapper_StopAtExit
config
for 101 minutes in {fastdebug,release,slowdebug} configs. Lastly, the updated
test was
tested with Mach5 Tier[1567] with no failures on any platform.
-
Commit messages:
- 8282314.cr0.patch
Changes: https://git.openjdk.java.net/jdk/pull/7764/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7764=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8282314
Stats: 12 lines in 2 files changed: 10 ins; 0 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/7764.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7764/head:pull/7764
PR: https://git.openjdk.java.net/jdk/pull/7764
Integrated: 8282428: ProblemList jdk/jfr/jvm/TestWaste.java
A trivial fix to ProblemList jdk/jfr/jvm/TestWaste.java. - Commit messages: - 8282428: ProblemList jdk/jfr/jvm/TestWaste.java Changes: https://git.openjdk.java.net/jdk/pull/7631/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7631=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282428 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7631.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7631/head:pull/7631 PR: https://git.openjdk.java.net/jdk/pull/7631
Re: Integrated: 8282428: ProblemList jdk/jfr/jvm/TestWaste.java
On Sun, 27 Feb 2022 03:42:50 GMT, Mikael Vidstedt wrote: >> A trivial fix to ProblemList jdk/jfr/jvm/TestWaste.java. > > Marked as reviewed by mikael (Reviewer). @vidmik - Thanks for the lightning fast review! Especially considering that it's Saturday night... - PR: https://git.openjdk.java.net/jdk/pull/7631
Integrated: 8282428: ProblemList jdk/jfr/jvm/TestWaste.java
On Sun, 27 Feb 2022 03:37:11 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList jdk/jfr/jvm/TestWaste.java. This pull request has now been integrated. Changeset: 630ad1ac Author: Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk/commit/630ad1acb20abae8bde037b8d23dd2a14a70d732 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8282428: ProblemList jdk/jfr/jvm/TestWaste.java Reviewed-by: mikael - PR: https://git.openjdk.java.net/jdk/pull/7631
Re: RFR: 8281614: serviceability/sa/ClhsdbFindPC.java fails with java.lang.RuntimeException: 'In code in NMethod for jdk/test/lib/apps/LingeredApp.steadyState' missing from stdout/stderr [v2]
On Wed, 23 Feb 2022 05:56:18 GMT, Chris Plummer wrote: >> This test has 4 test cases/modes: two core files test cases and two process. >> Each runs with and w/o `-Xcomp`. The `-Xcomp` test cases are not suppose to >> run when `-XX:+DeoptimizeALot` is used, because the test does some checks >> that assume certain methods will be compiled. This is handled by adding the >> following to the test case: >> >> ` * @requires vm.opt.DeoptimizeALot != true` >> >> When this was first added, only the process test cases existed. Later on the >> core tests cases were added, and the `@requires` was copied improperly. It >> ended up with `#no-xcomp-process` rather than `#xcomp-core`, which allows >> the `#xcomp-core` test case to run even when `-XX:+DeoptimizeALot` is being >> used. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Move @requires for Xcomp to the correct test case. Still thumbs up. - Marked as reviewed by dcubed (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7542
Re: RFR: 8281614: serviceability/sa/ClhsdbFindPC.java fails with java.lang.RuntimeException: 'In code in NMethod for jdk/test/lib/apps/LingeredApp.steadyState' missing from stdout/stderr
On Tue, 22 Feb 2022 22:38:08 GMT, Chris Plummer wrote: >> test/hotspot/jtreg/serviceability/sa/ClhsdbFindPC.java line 61: >> >>> 59: * @bug 8193124 >>> 60: * @summary Test the clhsdb 'findpc' command w/o Xcomp on live process >>> 61: * @requires vm.hasSA >> >> Summary says w/o xcomp but no requires clause to reject Xcomp mode > > I think that is related to why xcomp-process and xcomp-core are not in > agreement. Probably also a bug introduced when adding the two core tests > (just like the DeoptimizeALot check was done wrong). As a reference, here's > what it looked like before adding the two core tests: > > > * @requires vm.hasSA > * @requires vm.compiler1.enabled > * @requires vm.opt.DeoptimizeALot != true > * @run main/othervm/timeout=480 ClhsdbFindPC true > > * @requires vm.compMode != "Xcomp" > * @requires vm.hasSA > * @requires vm.compiler1.enabled > * @run main/othervm/timeout=480 ClhsdbFindPC false > > The first test is running the debuggee with -Xcomp. The test itself can be > run with or without -Xcomp. The second runs the debuggee without -Xcomp. In > this case the test is not allowed to be run with -Xcomp, because it would be > passed to the debuggee (although with some work the test could have stripped > it). I believe Leonid added this restriction to avoid unnecessarily testing > the debuggee in -Xcomp mode twice. To be it consistent it looks like I need > to move the `@requires vm.compMode != "Xcomp"` from `id=xcomp-core` to > `no-xcomp-process`. The other choice is to just remove this restriction. I > wasn't a fan of it when it went in initially. Personally, I'm not a fan of running the debugger/debuggee tests with the debugger in -Xcomp mode. I don't think that represents a real world usage scenario. Exercising the debuggee in -Xcomp mode is useful IMHO. - PR: https://git.openjdk.java.net/jdk/pull/7542
Re: RFR: 8281614: serviceability/sa/ClhsdbFindPC.java fails with java.lang.RuntimeException: 'In code in NMethod for jdk/test/lib/apps/LingeredApp.steadyState' missing from stdout/stderr
On Sat, 19 Feb 2022 04:00:30 GMT, Chris Plummer wrote: > This test has 4 test cases/modes: two core files test cases and two process. > Each runs with and w/o `-Xcomp`. The `-Xcomp` test cases are not suppose to > run when `-XX:+DeoptimizeALot` is used, because the test does some checks > that assume certain methods will be compiled. This is handled by adding the > following to the test case: > > ` * @requires vm.opt.DeoptimizeALot != true` > > When this was first added, only the process test cases existed. Later on the > core tests cases were added, and the `@requires` was copied improperly. It > ended up with `#no-xcomp-process` rather than `#xcomp-core`, which allows the > `#xcomp-core` test case to run even when `-XX:+DeoptimizeALot` is being used. Thumbs up. - Marked as reviewed by dcubed (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7542
Re: RFR: 8281243: Test java/lang/instrument/RetransformWithMethodParametersTest.java is failing
On Fri, 4 Feb 2022 11:18:39 GMT, Alex Menkov wrote: > The test expects ClassFileReconstituter restores exactly the same bytes as > original classbytes. > This can be wrong if the class has more than 1 method (due to method sorting > in the VM). > MethodParametersTarget class had only 1 method (method1), but didn't have > constructors. This caused declaration of implicit default constructor, so the > class actually had 2 methods. > The fix converts the method to constructor to avoid default constructor > declaration. Thumbs up on the changes. However, there's no details on how this fix was tested. Please clarify. So far this test failure has been seen intermittently in Tier3. - Marked as reviewed by dcubed (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7345
Re: RFR: 8280889: java/lang/instrument/GetObjectSizeIntrinsicsTest.java fails with -XX:-UseCompressedOops
On Fri, 28 Jan 2022 15:37:59 GMT, Aleksey Shipilev wrote: > Recent test regression after adding new cases in the test. Without compressed > oops, ~1G elements `Object[]` array takes >8G of memory, which fails the > test. The fix cuts it down to 512M when reference size is 8 bytes. > Additionally, `ObjectAlignmentInBytes=32` is excessive for new test configs. > > Additional testing: > - [x] Linux x86_64 fastdebug, default, affected test still passes > - [x] Linux x86_32 fastdebug, default, affected test still passes > - [x] Linux x86_64 fastdebug, `-XX:-UseCompressedOops`, affected test now > passes Thumbs up. Thanks for fixing this issue so quickly. - Marked as reviewed by dcubed (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7269
Re: Integrated: 8280413: ProblemList jdk/jfr/event/oldobject/TestLargeRootSet.java on all X64 platforms
On Thu, 20 Jan 2022 20:22:42 GMT, Alexander Zvegintsev wrote: >> A trivial fix to ProblemList jdk/jfr/event/oldobject/TestLargeRootSet.java >> on all X64 platforms. > > Marked as reviewed by azvegint (Reviewer). @azvegint - Thanks for the fast review! - PR: https://git.openjdk.java.net/jdk/pull/7168
Integrated: 8280413: ProblemList jdk/jfr/event/oldobject/TestLargeRootSet.java on all X64 platforms
On Thu, 20 Jan 2022 20:21:17 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList jdk/jfr/event/oldobject/TestLargeRootSet.java on > all X64 platforms. This pull request has now been integrated. Changeset: 293fb46f Author:Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk/commit/293fb46f7cd28f2a08055e3eb8ec9459d64e9688 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8280413: ProblemList jdk/jfr/event/oldobject/TestLargeRootSet.java on all X64 platforms Reviewed-by: azvegint - PR: https://git.openjdk.java.net/jdk/pull/7168
Integrated: 8280413: ProblemList jdk/jfr/event/oldobject/TestLargeRootSet.java on all X64 platforms
A trivial fix to ProblemList jdk/jfr/event/oldobject/TestLargeRootSet.java on all X64 platforms. - Commit messages: - 8280413: ProblemList jdk/jfr/event/oldobject/TestLargeRootSet.java on all X64 platforms Changes: https://git.openjdk.java.net/jdk/pull/7168/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7168=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8280413 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7168.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7168/head:pull/7168 PR: https://git.openjdk.java.net/jdk/pull/7168
Re: [jdk18] Integrated: 8280034: ProblemList jdk/jfr/api/consumer/recordingstream/TestOnEvent.java on linux-x64
On Fri, 14 Jan 2022 17:46:12 GMT, Calvin Cheung wrote: >> A trivial fix to ProblemList >> jdk/jfr/api/consumer/recordingstream/TestOnEvent.java on linux-x64. > > LGTM. @calvinccheung - Thanks for the fast review! - PR: https://git.openjdk.java.net/jdk18/pull/102
[jdk18] Integrated: 8280034: ProblemList jdk/jfr/api/consumer/recordingstream/TestOnEvent.java on linux-x64
On Fri, 14 Jan 2022 17:41:20 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList > jdk/jfr/api/consumer/recordingstream/TestOnEvent.java on linux-x64. This pull request has now been integrated. Changeset: 09d61b61 Author:Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk18/commit/09d61b6187425ba528c568fb637087817ffb10c0 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8280034: ProblemList jdk/jfr/api/consumer/recordingstream/TestOnEvent.java on linux-x64 Reviewed-by: ccheung - PR: https://git.openjdk.java.net/jdk18/pull/102
[jdk18] Integrated: 8280034: ProblemList jdk/jfr/api/consumer/recordingstream/TestOnEvent.java on linux-x64
A trivial fix to ProblemList jdk/jfr/api/consumer/recordingstream/TestOnEvent.java on linux-x64. - Commit messages: - 8280034: ProblemList jdk/jfr/api/consumer/recordingstream/TestOnEvent.java on linux-x64 Changes: https://git.openjdk.java.net/jdk18/pull/102/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk18=102=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8280034 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk18/pull/102.diff Fetch: git fetch https://git.openjdk.java.net/jdk18 pull/102/head:pull/102 PR: https://git.openjdk.java.net/jdk18/pull/102
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12 [v2]
On Fri, 31 Dec 2021 23:54:22 GMT, David Nadlinger wrote: >> @gerard-ziemski - Thanks for the re-review! > > @dcubed-ojdk: I just ran into this in the Julia runtime and reached a similar > conclusion/fix. Did you find out anything more about why this happens? At a > glance, I didn't see any blatant `addr == -1`-style checks in the latest dyld > open-source dump. @dnadlinger - No I haven't chased this any further. Also please see this note in the bug report: https://bugs.openjdk.java.net/browse/JDK-8273967?focusedCommentId=14455403=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14455403 - PR: https://git.openjdk.java.net/jdk/pull/6193
[jdk18] Integrated: 8279081: ProblemList jdk/jfr/event/oldobject/TestLargeRootSet.java on 2 platforms
On Tue, 21 Dec 2021 19:56:07 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList jdk/jfr/event/oldobject/TestLargeRootSet.java on > 2 platforms This pull request has now been integrated. Changeset: 84dc Author:Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk18/commit/84dc7a979742021e759766a7290539b569f4 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8279081: ProblemList jdk/jfr/event/oldobject/TestLargeRootSet.java on 2 platforms Reviewed-by: azvegint, sspitsyn - PR: https://git.openjdk.java.net/jdk18/pull/61
Re: [jdk18] RFR: 8279081: ProblemList jdk/jfr/event/oldobject/TestLargeRootSet.java on 2 platforms
On Tue, 21 Dec 2021 20:36:05 GMT, Alexander Zvegintsev wrote: >> A trivial fix to ProblemList jdk/jfr/event/oldobject/TestLargeRootSet.java >> on 2 platforms > > Marked as reviewed by azvegint (Reviewer). @azvegint and @sspitsyn - Thanks for the reviews! - PR: https://git.openjdk.java.net/jdk18/pull/61
[jdk18] RFR: 8279081: ProblemList jdk/jfr/event/oldobject/TestLargeRootSet.java on 2 platforms
A trivial fix to ProblemList jdk/jfr/event/oldobject/TestLargeRootSet.java on 2 platforms - Commit messages: - 8279081: ProblemList jdk/jfr/event/oldobject/TestLargeRootSet.java on 2 platforms Changes: https://git.openjdk.java.net/jdk18/pull/61/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk18=61=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8279081 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk18/pull/61.diff Fetch: git fetch https://git.openjdk.java.net/jdk18 pull/61/head:pull/61 PR: https://git.openjdk.java.net/jdk18/pull/61
Re: [jdk18] RFR: 8132785: java/lang/management/ThreadMXBean/ThreadLists.java fails intermittently
On Mon, 13 Dec 2021 04:19:39 GMT, David Holmes wrote: > Investigation showed this test was experiencing interference from threads > created by other tests in agentvm mode. The simple solution is to run this > test isolated using othervm mode. Also added some diagnostics to the test > incase we see future failures. > > Testing: local and tier3. > > Thanks, > David Thumbs up. Thanks for including a comment about why othervm is needed. - Marked as reviewed by dcubed (Reviewer). PR: https://git.openjdk.java.net/jdk18/pull/14
[jdk18] Integrated: 8278521: ProblemList java/lang/management/ThreadMXBean/ThreadLists.java
On Thu, 9 Dec 2021 21:57:13 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList > java/lang/management/ThreadMXBean/ThreadLists.java on all configs. This pull request has now been integrated. Changeset: d40e90b4 Author:Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk18/commit/d40e90b4a1b55753a178d824c4c60209bc46efac Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8278521: ProblemList java/lang/management/ThreadMXBean/ThreadLists.java Reviewed-by: bpb - PR: https://git.openjdk.java.net/jdk18/pull/3
[jdk18] Integrated: 8278521: ProblemList java/lang/management/ThreadMXBean/ThreadLists.java
A trivial fix to ProblemList java/lang/management/ThreadMXBean/ThreadLists.java on all configs. - Commit messages: - 8278521: ProblemList java/lang/management/ThreadMXBean/ThreadLists.java Changes: https://git.openjdk.java.net/jdk18/pull/3/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk18=3=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8278521 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk18/pull/3.diff Fetch: git fetch https://git.openjdk.java.net/jdk18 pull/3/head:pull/3 PR: https://git.openjdk.java.net/jdk18/pull/3
Re: [jdk18] Integrated: 8278521: ProblemList java/lang/management/ThreadMXBean/ThreadLists.java
On Thu, 9 Dec 2021 21:59:19 GMT, Brian Burkhalter wrote: >> A trivial fix to ProblemList >> java/lang/management/ThreadMXBean/ThreadLists.java on all configs. > > Marked as reviewed by bpb (Reviewer). @bplb - Thanks for the lightning fast review! - PR: https://git.openjdk.java.net/jdk18/pull/3
Re: RFR: 8258512: serviceability/sa/TestJmapCore.java timed out on macOS 10.13.6
On Thu, 2 Dec 2021 17:36:43 GMT, Chris Plummer wrote: > Increase the test timeout to 480 just to make sure we don't see this timeout > anymore. This same change was already made to TestJmapCoreMetaspace.java a > long time ago. Thumbs up. This is a trivial change. - Marked as reviewed by dcubed (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6676
Integrated: 8277811: ProblemList vmTestbase/nsk/jdi/TypeComponent/isSynthetic/issynthetic001/TestDescription.java
On Wed, 24 Nov 2021 22:03:29 GMT, Daniel D. Daugherty wrote: > A couple of trivial ProblemListings: > > JDK-8277811 ProblemList > vmTestbase/nsk/jdi/TypeComponent/isSynthetic/issynthetic001/TestDescription.java > JDK-8277813 ProblemList > vmTestbase/nsk/jvmti/AttachOnDemand/attach002a/TestDescription.java This pull request has now been integrated. Changeset: 26472bd3 Author:Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk/commit/26472bd3bd8788b0839e2871ed220e438fb6d608 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod 8277811: ProblemList vmTestbase/nsk/jdi/TypeComponent/isSynthetic/issynthetic001/TestDescription.java 8277813: ProblemList vmTestbase/nsk/jvmti/AttachOnDemand/attach002a/TestDescription.java Reviewed-by: dholmes - PR: https://git.openjdk.java.net/jdk/pull/6547
Re: RFR: 8277811: ProblemList vmTestbase/nsk/jdi/TypeComponent/isSynthetic/issynthetic001/TestDescription.java
On Wed, 24 Nov 2021 22:08:31 GMT, David Holmes wrote: >> A couple of trivial ProblemListings: >> >> JDK-8277811 ProblemList >> vmTestbase/nsk/jdi/TypeComponent/isSynthetic/issynthetic001/TestDescription.java >> JDK-8277813 ProblemList >> vmTestbase/nsk/jvmti/AttachOnDemand/attach002a/TestDescription.java > > Marked as reviewed by dholmes (Reviewer). @dholmes-ora - Thanks for the fast review! - PR: https://git.openjdk.java.net/jdk/pull/6547
RFR: 8277811: ProblemList vmTestbase/nsk/jdi/TypeComponent/isSynthetic/issynthetic001/TestDescription.java
A couple of trivial ProblemListings: JDK-8277811 ProblemList vmTestbase/nsk/jdi/TypeComponent/isSynthetic/issynthetic001/TestDescription.java JDK-8277813 ProblemList vmTestbase/nsk/jvmti/AttachOnDemand/attach002a/TestDescription.java - Commit messages: - 8277813: ProblemList vmTestbase/nsk/jvmti/AttachOnDemand/attach002a/TestDescription.java - 8277811: ProblemList vmTestbase/nsk/jdi/TypeComponent/isSynthetic/issynthetic001/TestDescription.java Changes: https://git.openjdk.java.net/jdk/pull/6547/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6547=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277811 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6547.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6547/head:pull/6547 PR: https://git.openjdk.java.net/jdk/pull/6547
Re: RFR: 8266593: vmTestbase/nsk/jvmti/PopFrame/popframe011 fails with "assert(java_thread == _state->get_thread()) failed: Must be" [v3]
On Fri, 19 Nov 2021 10:14:23 GMT, Serguei Spitsyn wrote:
>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1401:
>>
>>> 1399: if (!self) {
>>> 1400: if (!java_thread->is_suspended()) {
>>> 1401: _result = JVMTI_ERROR_THREAD_NOT_SUSPENDED;
>>
>> I don't see an obvious reason for this `is_exiting()` check.
>
> Okay. I see similar check in the `force_early_return()` function:
>
> if (state == NULL) {
> return JVMTI_ERROR_THREAD_NOT_ALIVE;
> }
>
> Would it better to replace it with this check instead? :
>
> if (java_thread->is_exiting()) {
> return JVMTI_ERROR_THREAD_NOT_ALIVE;
> }
>
> Removing this check and keep the one inside the handshake would be even
> better.
>
> I would also add this line for symmetry with two other cases:
>
> + MutexLocker mu(JvmtiThreadState_lock);
> SetForceEarlyReturn op(state, value, tos);
My point is that I don't see why you added the `is_exiting()` check
since I don't see a race in that function, i.e., there's no `assert()` in
this function that you need to protect.
As for adding the `MutexLocker mu(JvmtiThreadState_lock)`, you'll
have to analyze and justify why you would need to add that lock grab
independent of this fix. I'm not seeing a bug there, but I haven't looked
very closely.
>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1533:
>>
>>> 1531: return; /* JVMTI_ERROR_THREAD_NOT_ALIVE (default) */
>>> 1532: }
>>> 1533: assert(java_thread == _state->get_thread(), "Must be");
>>
>> This `assert()` is the site of the original test failure. I haven't yet
>> looked at the locations of the other changes.
>>
>> The `is_exiting()` check is made under the protection of the
>> `JvmtiThreadState_lock` so an unsuspended target thread that is
>> exiting cannot reach the point where the `_state` is updated to
>> clear the `JavaThread*` so we can't fail the `assert()` if the
>> `is_exiting()` check has returned `false`.
>
> Dan,
> Thank you for reviewing this!
> I'm not sure, I correctly understand you here.
> Are you saying that you agree with this change?
> In fact, the thread state can not be changed (and the assert fired) after the
> `is_exiting()` check is made even without `JvmtiThreadState_lock` protection
> because it is inside of a handshake execution.
I agree with the `is_exiting()` check addition.
I forgot that we're executing a Handshake `doit()` function. So we have a couple
of reasons why an unsuspended target thread can't change from `!is_exiting()`
to `is_exiting()` while we are in this function.
-
PR: https://git.openjdk.java.net/jdk/pull/6440
Re: RFR: 8266593: vmTestbase/nsk/jvmti/PopFrame/popframe011 fails with "assert(java_thread == _state->get_thread()) failed: Must be" [v3]
On Thu, 18 Nov 2021 09:34:13 GMT, Serguei Spitsyn wrote: >> The test fails when the target JavaThread has is_exiting() status. In such a >> case the JvmtiExport::cleanup_thread(this) has already made a clean up of >> its jvmtiThreadState, so the JavaThread address returned by >> _state->get_thread() is 0xbabababababababa. >> The fix is to add a check for is_exiting() status into handshake closure >> do_thread() early. >> There following handshake closures are fixed by this update: >> - UpdateForPopTopFrameClosure >> - SetForceEarlyReturn >> - SetFramePopClosure > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > get rid of the checks in jvmti handshakes: java_thread->threadObj() == NULL Marked as reviewed by dcubed (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6440
Re: RFR: 8266593: vmTestbase/nsk/jvmti/PopFrame/popframe011 fails with "assert(java_thread == _state->get_thread()) failed: Must be" [v3]
On Thu, 18 Nov 2021 09:34:13 GMT, Serguei Spitsyn wrote:
>> The test fails when the target JavaThread has is_exiting() status. In such a
>> case the JvmtiExport::cleanup_thread(this) has already made a clean up of
>> its jvmtiThreadState, so the JavaThread address returned by
>> _state->get_thread() is 0xbabababababababa.
>> The fix is to add a check for is_exiting() status into handshake closure
>> do_thread() early.
>> There following handshake closures are fixed by this update:
>> - UpdateForPopTopFrameClosure
>> - SetForceEarlyReturn
>> - SetFramePopClosure
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> get rid of the checks in jvmti handshakes: java_thread->threadObj() == NULL
I don't see a reason for the change in `SetForceEarlyReturn::doit()`,
but I'm okay with the other changes.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1401:
> 1399: if (!self) {
> 1400: if (!java_thread->is_suspended()) {
> 1401: _result = JVMTI_ERROR_THREAD_NOT_SUSPENDED;
I don't see an obvious reason for this `is_exiting()` check.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1625:
> 1623: return; /* JVMTI_ERROR_THREAD_NOT_ALIVE (default) */
> 1624: }
> 1625: assert(_state->get_thread() == java_thread, "Must be");
The `assert()` on L1625 is subject to the same race as the original site.
This `is_exiting()` check is made under the protection of the
`JvmtiThreadState_lock` so it is sufficient to protect that `assert()`.
-
Changes requested by dcubed (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/6440
Re: RFR: 8266593: vmTestbase/nsk/jvmti/PopFrame/popframe011 fails with "assert(java_thread == _state->get_thread()) failed: Must be" [v3]
On Thu, 18 Nov 2021 09:34:13 GMT, Serguei Spitsyn wrote: >> The test fails when the target JavaThread has is_exiting() status. In such a >> case the JvmtiExport::cleanup_thread(this) has already made a clean up of >> its jvmtiThreadState, so the JavaThread address returned by >> _state->get_thread() is 0xbabababababababa. >> The fix is to add a check for is_exiting() status into handshake closure >> do_thread() early. >> There following handshake closures are fixed by this update: >> - UpdateForPopTopFrameClosure >> - SetForceEarlyReturn >> - SetFramePopClosure > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > get rid of the checks in jvmti handshakes: java_thread->threadObj() == NULL src/hotspot/share/prims/jvmtiEnvBase.cpp line 1533: > 1531: return; /* JVMTI_ERROR_THREAD_NOT_ALIVE (default) */ > 1532: } > 1533: assert(java_thread == _state->get_thread(), "Must be"); This `assert()` is the site of the original test failure. I haven't yet looked at the locations of the other changes. The `is_exiting()` check is made under the protection of the `JvmtiThreadState_lock` so an unsuspended target thread that is exiting cannot reach the point where the `_state` is updated to clear the `JavaThread*` so we can't fail the `assert()` if the `is_exiting()` check has returned `false`. - PR: https://git.openjdk.java.net/jdk/pull/6440
Integrated: 8277346: ProblemList 7 serviceability/sa tests on macosx-x64
On Wed, 17 Nov 2021 20:43:46 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList 7 serviceability/sa tests on macosx-x64. This pull request has now been integrated. Changeset: ce4471f8 Author: Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk/commit/ce4471f806e51bc9f9ad746b69ba490443947110 Stats: 8 lines in 1 file changed: 4 ins; 0 del; 4 mod 8277346: ProblemList 7 serviceability/sa tests on macosx-x64 8277351: ProblemList runtime/jni/checked/TestPrimitiveArrayCriticalWithBadParam.java on macosx-x64 Reviewed-by: tschatzl, bpb - PR: https://git.openjdk.java.net/jdk/pull/6438
Re: RFR: 8277346: ProblemList 7 serviceability/sa tests on macosx-x64
On Wed, 17 Nov 2021 21:16:18 GMT, Thomas Schatzl wrote: >> A trivial fix to ProblemList 7 serviceability/sa tests on macosx-x64. > > Lgtm and trivial @tschatzl and @bplb - Thanks for the fast review! - PR: https://git.openjdk.java.net/jdk/pull/6438
RFR: 8277346: ProblemList 7 serviceability/sa tests on macosx-x64
A trivial fix to ProblemList 7 serviceability/sa tests on macosx-x64. - Commit messages: - 8277351: ProblemList runtime/jni/checked/TestPrimitiveArrayCriticalWithBadParam.java on macosx-x64 - 8277346: ProblemList 7 serviceability/sa tests on macosx-x64 Changes: https://git.openjdk.java.net/jdk/pull/6438/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6438=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277346 Stats: 8 lines in 1 file changed: 4 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/6438.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6438/head:pull/6438 PR: https://git.openjdk.java.net/jdk/pull/6438
Integrated: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes
On Sat, 3 Jul 2021 14:15:54 GMT, Daniel D. Daugherty wrote: > A fix to reduce ThreadsListHandle overhead in relation to handshakes and > we add sanity checks for ThreadsListHandles higher in the call stack. > > This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. This pull request has now been integrated. Changeset: ea23e733 Author: Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk/commit/ea23e7333e03abb4aca3e9f3854bab418a4b70e2 Stats: 121 lines in 7 files changed: 52 ins; 6 del; 63 mod 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes Reviewed-by: coleenp, sspitsyn, dholmes, rehn - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes
On Sun, 4 Jul 2021 23:39:00 GMT, David Holmes wrote: >> A fix to reduce ThreadsListHandle overhead in relation to handshakes and >> we add sanity checks for ThreadsListHandles higher in the call stack. >> >> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. > > Hi Dan, > > I just updated the bug report. This really isn't addressing the reasons the > RFE was filed. > > David @dholmes-ora and @robehn - Thanks for the re-reviews! - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 5 Nov 2021 21:54:01 GMT, Coleen Phillimore wrote: >> I've written up a rather long analysis about how the use of >> `JvmtiThreadState_lock` >> in `JvmtiEventControllerPrivate::recompute_enabled()` means that we can >> safely >> traverse the JvmtiThreadState list returned by `JvmtiThreadState::first()` >> without >> racing with an exiting JavaThread. I've sent it to you via direct email. >> >> I could amend the comment above the ThreadsListHandle like this: >> >> // If we have a JvmtiThreadState, then we've reached the point where >> // threads can exist so create a ThreadsListHandle to protect them. >> // The held JvmtiThreadState_lock prevents exiting JavaThreads from >> // being removed from the JvmtiThreadState list we're about to walk >> // so this ThreadsListHandle exists just to satisfy the lower level >> sanity >> // checks that the target JavaThreads are protected. >> ThreadsListHandle tlh; > > Yes, this comment is good and it does explain why it's safe and why the TLH > is there. Thanks for doing the deep dive and explanation. Updated comment has been pushed to this PR. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v13]
On Fri, 5 Nov 2021 23:42:07 GMT, Daniel D. Daugherty wrote: >> A fix to reduce ThreadsListHandle overhead in relation to handshakes and >> we add sanity checks for ThreadsListHandles higher in the call stack. >> >> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. > > Daniel D. Daugherty has updated the pull request incrementally with one > additional commit since the last revision: > > coleenp CR - clarify need for ThreadsListHandle in > JvmtiEventControllerPrivate::recompute_enabled(). Updated comment has been pushed to this PR. For some reason, the comments that @coleenp and I are posting in the "Files changed" view are not showing up here. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v13]
> A fix to reduce ThreadsListHandle overhead in relation to handshakes and > we add sanity checks for ThreadsListHandles higher in the call stack. > > This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision: coleenp CR - clarify need for ThreadsListHandle in JvmtiEventControllerPrivate::recompute_enabled(). - Changes: - all: https://git.openjdk.java.net/jdk/pull/4677/files - new: https://git.openjdk.java.net/jdk/pull/4677/files/91173506..fe28cbe9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4677=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4677=11-12 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4677.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4677/head:pull/4677 PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v12]
On Fri, 5 Nov 2021 15:43:37 GMT, Daniel D. Daugherty wrote: >> A fix to reduce ThreadsListHandle overhead in relation to handshakes and >> we add sanity checks for ThreadsListHandles higher in the call stack. >> >> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. > > Daniel D. Daugherty has updated the pull request incrementally with one > additional commit since the last revision: > > 8249004.cr4.patch > JvmtiThreadState objects point to JavaThread and vice versa, so I still don't > see why you don't protect the first element. I've written up a rather long analysis about how the use of JvmtiThreadState_lock in JvmtiEventControllerPrivate::recompute_enabled() means that we can safely traverse the JvmtiThreadState list returned by JvmtiThreadState::first() without racing with an exiting JavaThread. I've sent it to you via direct email. I could amend the comment above the ThreadsListHandle like this: // If we have a JvmtiThreadState, then we've reached the point where // threads can exist so create a ThreadsListHandle to protect them. // The held JvmtiThreadState_lock prevents exiting JavaThreads from // being removed from the JvmtiThreadState list we're about to walk // so this ThreadsListHandle exists just to satisfy the lower level sanity // checks that the target JavaThreads are protected. ThreadsListHandle tlh; - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 5 Nov 2021 16:43:04 GMT, Coleen Phillimore wrote: >> The `ThreadsListHandle` protects `JavaThread` objects not `JvmtiThreadState` >> objects. >> `JvmtiThreadState::first()` returns the head of the global list of >> `JvmtiThreadState` >> objects for the system. Each `JvmtiThreadState` object contains a >> `JavaThread*` and >> we have to protect use of the `JavaThread*` which can happen in the >> `recompute_thread_enabled(state)` call below. > > JvmtiThreadState objects point to JavaThread and vice versa, so I still don't > see why you don't protect the first element. I've written up a rather long analysis about how the use of `JvmtiThreadState_lock` in `JvmtiEventControllerPrivate::recompute_enabled()` means that we can safely traverse the JvmtiThreadState list returned by `JvmtiThreadState::first()` without racing with an exiting JavaThread. I've sent it to you via direct email. I could amend the comment above the ThreadsListHandle like this: // If we have a JvmtiThreadState, then we've reached the point where // threads can exist so create a ThreadsListHandle to protect them. // The held JvmtiThreadState_lock prevents exiting JavaThreads from // being removed from the JvmtiThreadState list we're about to walk // so this ThreadsListHandle exists just to satisfy the lower level sanity // checks that the target JavaThreads are protected. ThreadsListHandle tlh; - PR: https://git.openjdk.java.net/jdk/pull/4677
Integrated: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Mon, 1 Nov 2021 15:32:54 GMT, Daniel D. Daugherty wrote: > macOS12 has changed the dladdr() function to accept "-1" as a valid address > and > we have functions that use dladdr() to convert DLL addresses into function or > library names. We also have a gtest that verifies that "-1" is not a valid > value to use > as a symbol address. > > As you might imagine, existing code that uses > `os::dll_address_to_function_name()` > or `os::dll_address_to_library_name()` can get quite confused (and sometimes > crash) > if an `addr` parameter of `-1` was allowed to be used. > > I've also made two cleanup changes as part of this fix: > > 1) In `src/hotspot/os/bsd/os_bsd.cpp` there is some macOS specific code that > should >be properly `#ifdef`'ed. There is also some code that makes sense for ELF > format >files, but not for Mach-O format files so that code needs to be excluded > on macOS. > > 2) In `src/hotspot/share/runtime/os.cpp` I noticed a simple typo in a comment > on an > `#endif` that I fixed. That typo does not appear anywhere else in the > HotSpot code > base so I'd like to fix it with this bug ID since I'm in related areas. > > This fix has been tested with Mach5 Tier[1-6]. This pull request has now been integrated. Changeset: 92d21763 Author:Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk/commit/92d2176362954a7093894057748056610eeafe4b Stats: 46 lines in 4 files changed: 34 ins; 8 del; 4 mod 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12 Reviewed-by: stuefe, gziemski - PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes
On Sun, 4 Jul 2021 23:39:00 GMT, David Holmes wrote: >> A fix to reduce ThreadsListHandle overhead in relation to handshakes and >> we add sanity checks for ThreadsListHandles higher in the call stack. >> >> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. > > Hi Dan, > > I just updated the bug report. This really isn't addressing the reasons the > RFE was filed. > > David @dholmes-ora and @sspitsyn - I still need a re-review from you guys. @robehn - A re-review from you would be good, but I think I cleanly addressed your last comments... - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Thu, 4 Nov 2021 21:45:16 GMT, Coleen Phillimore wrote: >> Daniel D. Daugherty has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8249004.cr2.patch > > Sorry for such a long delay looking at this. I had a couple questions and a > suggestion. @coleenp - Thanks for the re-review! - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Fri, 5 Nov 2021 15:15:14 GMT, Daniel D. Daugherty wrote: >> Yes, ^ that might make the logic easier to follow. I can't figure out what >> checkTLSOnly means. Could it be refactored into a different function like >> check_TLS() and then call it in the place where you pass true instead of >> is_JavaThread_protected? Does checkTLSOnly skip a lot of these checks? > > I've changed `is_JavaThread_protected()` to materialize current_thread when > it is declared > to simplify the code paths. > I can't figure out what checkTLSOnly means. See the function's header comment: // ... If checkTLHOnly is true (default is false), then we only check // if the target JavaThread is protected by a ThreadsList (if any) associated // with the calling Thread. > Could it be refactored into a different function like check_TLS() and then > call it in the place where you pass true instead of is_JavaThread_protected? I had a copy of this logic in a separate function and @dholmes-ora suggested that I add the `checkTLHOnly` parameter to this function and get rid of the separate copy so I've been there and done that. I could *move* the logic into a separate function and then call that new function from here and from the other call sites that pass `true`, but then I'd have to materialize current_thread in that new function and then on this code path we would be materializing current_thread twice. I don't want to do that. > Does checkTLSOnly skip a lot of these checks? Please read the header comment to see what `checkTLHOnly` does. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Thu, 4 Nov 2021 19:33:33 GMT, Coleen Phillimore wrote: >> It wasn't quite missing from the baseline code. This version of execute(): >> >> `Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target)` >> >> used to always create a ThreadsListHandle. I added a `ThreadsListHandle*` >> parameter to that version and created a wrapper with the existing signature >> to pass `nullptr` to the execute() version with the `ThreadsListHandle*` >> parameter. What that means is that all existing callers of: >> >> `Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target)` >> >> no longer had a ThreadsListHandle created for them. With the new sanity >> check in place, I shook the trees to make sure that we had explicit >> ThreadsListHandles in place for the locations that needed them. >> >> `JvmtiEventControllerPrivate::recompute_enabled()` happened to be >> one of the places where the ThreadsListHandle created by execute() >> was hiding the fact that `recompute_enabled()` needed one. > > Should the ThreadsListHandle protect JvmtiThreadState::first also? If > state->next() needs it why doesn't the first entry need this? There's no > atomic load on the _head field. The `ThreadsListHandle` protects `JavaThread` objects not `JvmtiThreadState` objects. `JvmtiThreadState::first()` returns the head of the global list of `JvmtiThreadState` objects for the system. Each `JvmtiThreadState` object contains a `JavaThread*` and we have to protect use of the `JavaThread*` which can happen in the `recompute_thread_enabled(state)` call below. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Thu, 4 Nov 2021 21:44:45 GMT, Coleen Phillimore wrote: >> I'm really tempted to go ahead and change it to always set >> current thread when it is declared and then clean things up a bit. > > Yes, ^ that might make the logic easier to follow. I can't figure out what > checkTLSOnly means. Could it be refactored into a different function like > check_TLS() and then call it in the place where you pass true instead of > is_JavaThread_protected? Does checkTLSOnly skip a lot of these checks? I've changed `is_JavaThread_protected()` to materialize current_thread when it is declared to simplify the code paths. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v12]
> A fix to reduce ThreadsListHandle overhead in relation to handshakes and > we add sanity checks for ThreadsListHandles higher in the call stack. > > This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision: 8249004.cr4.patch - Changes: - all: https://git.openjdk.java.net/jdk/pull/4677/files - new: https://git.openjdk.java.net/jdk/pull/4677/files/86fcdfbe..91173506 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4677=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4677=10-11 Stats: 5 lines in 1 file changed: 0 ins; 3 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/4677.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4677/head:pull/4677 PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12 [v2]
On Fri, 5 Nov 2021 14:49:45 GMT, Gerard Ziemski wrote: >> Daniel D. Daugherty has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8273967.cr1.patch > > Thank you Dan for the fix! @gerard-ziemski - Thanks for the re-review! - PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12 [v2]
On Fri, 5 Nov 2021 05:16:28 GMT, Thomas Stuefe wrote: >> Daniel D. Daugherty has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8273967.cr1.patch > > Looks good! @tstuefe - Thanks for the re-review! - PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12 [v2]
> macOS12 has changed the dladdr() function to accept "-1" as a valid address > and > we have functions that use dladdr() to convert DLL addresses into function or > library names. We also have a gtest that verifies that "-1" is not a valid > value to use > as a symbol address. > > As you might imagine, existing code that uses > `os::dll_address_to_function_name()` > or `os::dll_address_to_library_name()` can get quite confused (and sometimes > crash) > if an `addr` parameter of `-1` was allowed to be used. > > I've also made two cleanup changes as part of this fix: > > 1) In `src/hotspot/os/bsd/os_bsd.cpp` there is some macOS specific code that > should >be properly `#ifdef`'ed. There is also some code that makes sense for ELF > format >files, but not for Mach-O format files so that code needs to be excluded > on macOS. > > 2) In `src/hotspot/share/runtime/os.cpp` I noticed a simple typo in a comment > on an > `#endif` that I fixed. That typo does not appear anywhere else in the > HotSpot code > base so I'd like to fix it with this bug ID since I'm in related areas. > > This fix has been tested with Mach5 Tier[1-6]. Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision: 8273967.cr1.patch - Changes: - all: https://git.openjdk.java.net/jdk/pull/6193/files - new: https://git.openjdk.java.net/jdk/pull/6193/files/f57d7f0d..ecb230d6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6193=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6193=00-01 Stats: 48 lines in 1 file changed: 16 ins; 28 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/6193.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6193/head:pull/6193 PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Mon, 1 Nov 2021 15:32:54 GMT, Daniel D. Daugherty wrote: > macOS12 has changed the dladdr() function to accept "-1" as a valid address > and > we have functions that use dladdr() to convert DLL addresses into function or > library names. We also have a gtest that verifies that "-1" is not a valid > value to use > as a symbol address. > > As you might imagine, existing code that uses > `os::dll_address_to_function_name()` > or `os::dll_address_to_library_name()` can get quite confused (and sometimes > crash) > if an `addr` parameter of `-1` was allowed to be used. > > I've also made two cleanup changes as part of this fix: > > 1) In `src/hotspot/os/bsd/os_bsd.cpp` there is some macOS specific code that > should >be properly `#ifdef`'ed. There is also some code that makes sense for ELF > format >files, but not for Mach-O format files so that code needs to be excluded > on macOS. > > 2) In `src/hotspot/share/runtime/os.cpp` I noticed a simple typo in a comment > on an > `#endif` that I fixed. That typo does not appear anywhere else in the > HotSpot code > base so I'd like to fix it with this bug ID since I'm in related areas. > > This fix has been tested with Mach5 Tier[1-6]. This version has been tested with Mach5 Tier1 and with runs of the specific tests using release and debug bits on macosx-aarch64 and macosx-x64 test machines running macOS12. - PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12 [v2]
On Thu, 4 Nov 2021 05:13:29 GMT, Thomas Stuefe wrote: >> Daniel D. Daugherty has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8273967.cr1.patch > > Marked as reviewed by stuefe (Reviewer). @tstuefe and @gerard-ziemski - please re-review when you get the chance. - PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Wed, 3 Nov 2021 21:14:17 GMT, Gerard Ziemski wrote: >> macOS12 has changed the dladdr() function to accept "-1" as a valid address >> and >> we have functions that use dladdr() to convert DLL addresses into function or >> library names. We also have a gtest that verifies that "-1" is not a valid >> value to use >> as a symbol address. >> >> As you might imagine, existing code that uses >> `os::dll_address_to_function_name()` >> or `os::dll_address_to_library_name()` can get quite confused (and sometimes >> crash) >> if an `addr` parameter of `-1` was allowed to be used. >> >> I've also made two cleanup changes as part of this fix: >> >> 1) In `src/hotspot/os/bsd/os_bsd.cpp` there is some macOS specific code that >> should >>be properly `#ifdef`'ed. There is also some code that makes sense for ELF >> format >>files, but not for Mach-O format files so that code needs to be excluded >> on macOS. >> >> 2) In `src/hotspot/share/runtime/os.cpp` I noticed a simple typo in a >> comment on an >> `#endif` that I fixed. That typo does not appear anywhere else in the >> HotSpot code >> base so I'd like to fix it with this bug ID since I'm in related areas. >> >> This fix has been tested with Mach5 Tier[1-6]. > > src/hotspot/os/bsd/os_bsd.cpp line 929: > >> 927: #endif >> 928: >> 929: #if defined(__APPLE__) > > Why not just do: > > `#else` > > here instead and collapse these 3 lines into 1? Hmmm... I'll take a look at doing that. > src/hotspot/os/bsd/os_bsd.cpp line 930: > >> 928: >> 929: #if defined(__APPLE__) >> 930: char localbuf[MACH_MAXSYMLEN]; > > This `__APPLE__` section is the only one, that I can see, using > `MACH_MAXSYMLEN`, why not move: > > > #if defined(__APPLE__) > #define MACH_MAXSYMLEN 256 > #endif > > > here (i.e. just the `#define MACH_MAXSYMLEN 256` and minimize the need for` > __APPLE__` sections? Hmmm I'll take a look at cleaning this up a bit. > src/hotspot/os/bsd/os_bsd.cpp line 964: > >> 962: if (offset) *offset = -1; >> 963: return false; >> 964: } > > Do we need this here? Wouldn't the earlier call to `Decoder::decode(addr, > localbuf, MACH_MAXSYMLEN, offset, dlinfo.dli_fbase))` catch this with > `ShouldNotReachHere`? That's the 5-parameter version of decode() and it doesn't have `ShouldNotReachHere`. So if that code site is called and returns `false`, then we get into `dll_address_to_library_name()` and reach this `dladdr()` call which will accept the "-1"... - PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Thu, 4 Nov 2021 05:12:48 GMT, Thomas Stuefe wrote: >> I took a quick look at the other calls to `dladdr()` in >> src/hotspot/os/bsd/os_bsd.cpp >> and I'm not comfortable with changing those uses without having a specific >> test >> case that I can use to investigate those code paths. >> >> We are fairly early in our testing on macOS12 so I may run into a reason to >> revisit >> this choice down the road. > >> I took a quick look at the other calls to `dladdr()` in >> src/hotspot/os/bsd/os_bsd.cpp and I'm not comfortable with changing those >> uses without having a specific test case that I can use to investigate those >> code paths. >> >> We are fairly early in our testing on macOS12 so I may run into a reason to >> revisit this choice down the road. > > Okay! I've had a chance to think about this overnight and I'm not liking my duplication of code so I'm going to look at adding a wrapper that is called by the two calls sites where know I need the special handling. - PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Mon, 1 Nov 2021 15:32:54 GMT, Daniel D. Daugherty wrote: > macOS12 has changed the dladdr() function to accept "-1" as a valid address > and > we have functions that use dladdr() to convert DLL addresses into function or > library names. We also have a gtest that verifies that "-1" is not a valid > value to use > as a symbol address. > > As you might imagine, existing code that uses > `os::dll_address_to_function_name()` > or `os::dll_address_to_library_name()` can get quite confused (and sometimes > crash) > if an `addr` parameter of `-1` was allowed to be used. > > I've also made two cleanup changes as part of this fix: > > 1) In `src/hotspot/os/bsd/os_bsd.cpp` there is some macOS specific code that > should >be properly `#ifdef`'ed. There is also some code that makes sense for ELF > format >files, but not for Mach-O format files so that code needs to be excluded > on macOS. > > 2) In `src/hotspot/share/runtime/os.cpp` I noticed a simple typo in a comment > on an > `#endif` that I fixed. That typo does not appear anywhere else in the > HotSpot code > base so I'd like to fix it with this bug ID since I'm in related areas. > > This fix has been tested with Mach5 Tier[1-6]. @tstuefe - Thanks for closing the loop on my previous replies. @gerard-ziemski - Thanks for the review! I'm going to make more tweaks to this fix and will update the PR after my test cycle is complete. - PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Thu, 4 Nov 2021 05:12:32 GMT, Thomas Stuefe wrote: >> It's actually fairly common to have Mac-specific stuff in the BSD files. The >> macOS >> port was built on top of the BSD port and the BSD port was built by copying >> a LOT >> of code from Linux into BSD specific files with modifications as needed. >> >> If I pushed this change down into MachDecoder, then I would have to lose the >> `ShouldNotReachHere()` call in order to not assert in non-release bits. I >> don't >> think I want to do that since this may not be the only place that calls the >> 6-arg version of decode(). > >> It's actually fairly common to have Mac-specific stuff in the BSD files. The >> macOS port was built on top of the BSD port and the BSD port was built by >> copying a LOT of code from Linux into BSD specific files with modifications >> as needed. > > I always wondered whether anyone actually builds the BSDs in head. I assume > Oracle does not, right? I know there are downstream porters somewhere but > only for old releases, or? > >> >> If I pushed this change down into MachDecoder, then I would have to lose the >> `ShouldNotReachHere()` call in order to not assert in non-release bits. I >> don't think I want to do that since this may not be the only place that >> calls the 6-arg version of decode(). > > Fair enough, thanks for the clarification. Oracle does not build BSD in head. At one point, Dmitry Samersoff used to build BSD in his lab, but I don't know if he still does that. - PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Thu, 4 Nov 2021 01:16:03 GMT, David Holmes wrote: >> I suspect that the way that git is displaying the diffs is confusing you. >> >> We need `current_thread` set if we get to line 474 so we have to init >> `current_thread` on line 446 for the `checkTLHOnly == true` case and >> on line 463 for the `checkTLHOnly == false` case. >> >> I could simplify the logic by always setting current thread when it is >> declared on 444, but I was trying to avoid the call to `Thread::current()` >> until I actually needed it. I thought `Thread::current()` can be expensive. >> Is this no longer the case? > > Sorry I missed that line 463 is still within the else from line 447. > > Thread::current() is a compiler-defined thread-local access so should be > relatively cheap these days, but I have no numbers. I'm really tempted to go ahead and change it to always set current thread when it is declared and then clean things up a bit. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Thu, 4 Nov 2021 01:18:23 GMT, David Holmes wrote:
>> The rationale for removing the is_exiting() check from `java_suspend()` was
>> that it
>> was redundant because the handshake code detected and handled the
>> `is_exiting()`
>> case so we didn't need to do that work twice.
>>
>> If we look at `HandshakeState::resume()` there is no logic for detecting or
>> handling
>> the possibility of an exiting thread. That being said, we have to look
>> closer at what
>> `HandshakeState::resume()` does and whether that logic can be harmful if
>> executed
>> on an exiting thread.
>>
>> Here's the code:
>>
>> bool HandshakeState::resume() {
>> if (!is_suspended()) {
>> return false;
>> }
>> MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
>> if (!is_suspended()) {
>> assert(!_handshakee->is_suspended(), "cannot be suspended without a
>> suspend request");
>> return false;
>> }
>> // Resume the thread.
>> set_suspended(false);
>> _lock.notify();
>> return true;
>> }
>>
>>
>> I'm not seeing anything in `HandshakeState::resume()` that
>> worries me with respect to an exiting thread. Of course, the
>> proof is in the testing so I'll rerun the usual testing after
>> deleting that code.
>
> A suspended thread cannot be exiting - else the suspend logic is broken. So,
> given you can call `resume()` on a not-suspended thread, as long as the
> handshake code checks for `is_supended()` (which it does) then no explicit
> `is_exiting` check is needed.
Agreed! I have to keep reminding myself that with handshake based suspend
and resume, we just don't have the same races with exiting threads that we
used to have.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes
On Sun, 4 Jul 2021 23:39:00 GMT, David Holmes wrote: >> A fix to reduce ThreadsListHandle overhead in relation to handshakes and >> we add sanity checks for ThreadsListHandles higher in the call stack. >> >> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. > > Hi Dan, > > I just updated the bug report. This really isn't addressing the reasons the > RFE was filed. > > David @dholmes-ora and @robehn - Thanks for your reviews on the previous version (8249004.cr2.patch). Mach5 Tier[1-5] testing has finished and looks good; Mach5 Tier[678] are still running. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v11]
> A fix to reduce ThreadsListHandle overhead in relation to handshakes and > we add sanity checks for ThreadsListHandles higher in the call stack. > > This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision: 8249004.cr3.patch - Changes: - all: https://git.openjdk.java.net/jdk/pull/4677/files - new: https://git.openjdk.java.net/jdk/pull/4677/files/3e1d1b06..86fcdfbe Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4677=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4677=09-10 Stats: 10 lines in 3 files changed: 0 ins; 4 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/4677.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4677/head:pull/4677 PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Wed, 3 Nov 2021 19:08:38 GMT, Daniel D. Daugherty wrote:
>> src/hotspot/os/bsd/os_bsd.cpp line 902:
>>
>>> 900: return false;
>>> 901: }
>>> 902: #endif
>>
>> We use dladdr() in several places in this code. I wonder whether it would
>> make sense to fix all of those with a wrapper instead:
>>
>> static int my_dladdr(const void* addr, Dl_info* info) {
>> if (addr != (void*)-1) {
>> return dladdr(addr, info);
>> } else {
>> // add comment here
>> return 0;
>> }
>> }
>> #define dladdr my_dladdr
>
> I'll take a look at the other calls to dladdr(). I'm trying to limit what I
> change
> here to things that actually failed in our test on macOS12 on X64 and aarch64.
I took a quick look at the other calls to `dladdr()` in
src/hotspot/os/bsd/os_bsd.cpp
and I'm not comfortable with changing those uses without having a specific test
case that I can use to investigate those code paths.
We are fairly early in our testing on macOS12 so I may run into a reason to
revisit
this choice down the road.
-
PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Wed, 3 Nov 2021 17:49:40 GMT, Thomas Stuefe wrote:
>> macOS12 has changed the dladdr() function to accept "-1" as a valid address
>> and
>> we have functions that use dladdr() to convert DLL addresses into function or
>> library names. We also have a gtest that verifies that "-1" is not a valid
>> value to use
>> as a symbol address.
>>
>> As you might imagine, existing code that uses
>> `os::dll_address_to_function_name()`
>> or `os::dll_address_to_library_name()` can get quite confused (and sometimes
>> crash)
>> if an `addr` parameter of `-1` was allowed to be used.
>>
>> I've also made two cleanup changes as part of this fix:
>>
>> 1) In `src/hotspot/os/bsd/os_bsd.cpp` there is some macOS specific code that
>> should
>>be properly `#ifdef`'ed. There is also some code that makes sense for ELF
>> format
>>files, but not for Mach-O format files so that code needs to be excluded
>> on macOS.
>>
>> 2) In `src/hotspot/share/runtime/os.cpp` I noticed a simple typo in a
>> comment on an
>> `#endif` that I fixed. That typo does not appear anywhere else in the
>> HotSpot code
>> base so I'd like to fix it with this bug ID since I'm in related areas.
>>
>> This fix has been tested with Mach5 Tier[1-6].
>
> src/hotspot/os/bsd/os_bsd.cpp line 902:
>
>> 900: return false;
>> 901: }
>> 902: #endif
>
> We use dladdr() in several places in this code. I wonder whether it would
> make sense to fix all of those with a wrapper instead:
>
> static int my_dladdr(const void* addr, Dl_info* info) {
> if (addr != (void*)-1) {
> return dladdr(addr, info);
> } else {
> // add comment here
> return 0;
> }
> }
> #define dladdr my_dladdr
I'll take a look at the other calls to dladdr(). I'm trying to limit what I
change
here to things that actually failed in our test on macOS12 on X64 and aarch64.
> src/hotspot/os/bsd/os_bsd.cpp line 918:
>
>> 916: // ranges like ELF. That makes sense since Mach-O can contain
>> binaries for
>> 917: // than one instruction set so there can be more than one address
>> range for
>> 918: // each "file".
>
> Small nit, it seems confusing to have a Mac-specific comment in the BSD
> section.
>
> Maybe this would be better in MachDecoder? E.g. implement the 6-arg version
> of decode() but stubbed out returning false, and with your comment there.
It's actually fairly common to have Mac-specific stuff in the BSD files. The
macOS
port was built on top of the BSD port and the BSD port was built by copying a
LOT
of code from Linux into BSD specific files with modifications as needed.
If I pushed this change down into MachDecoder, then I would have to lose the
`ShouldNotReachHere()` call in order to not assert in non-release bits. I don't
think I want to do that since this may not be the only place that calls the
6-arg version of decode().
-
PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Mon, 1 Nov 2021 15:32:54 GMT, Daniel D. Daugherty wrote: > macOS12 has changed the dladdr() function to accept "-1" as a valid address > and > we have functions that use dladdr() to convert DLL addresses into function or > library names. We also have a gtest that verifies that "-1" is not a valid > value to use > as a symbol address. > > As you might imagine, existing code that uses > `os::dll_address_to_function_name()` > or `os::dll_address_to_library_name()` can get quite confused (and sometimes > crash) > if an `addr` parameter of `-1` was allowed to be used. > > I've also made two cleanup changes as part of this fix: > > 1) In `src/hotspot/os/bsd/os_bsd.cpp` there is some macOS specific code that > should >be properly `#ifdef`'ed. There is also some code that makes sense for ELF > format >files, but not for Mach-O format files so that code needs to be excluded > on macOS. > > 2) In `src/hotspot/share/runtime/os.cpp` I noticed a simple typo in a comment > on an > `#endif` that I fixed. That typo does not appear anywhere else in the > HotSpot code > base so I'd like to fix it with this bug ID since I'm in related areas. > > This fix has been tested with Mach5 Tier[1-6]. @tstuefe - Thanks for your review. - PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Mon, 1 Nov 2021 15:32:54 GMT, Daniel D. Daugherty wrote: > macOS12 has changed the dladdr() function to accept "-1" as a valid address > and > we have functions that use dladdr() to convert DLL addresses into function or > library names. We also have a gtest that verifies that "-1" is not a valid > value to use > as a symbol address. > > As you might imagine, existing code that uses > `os::dll_address_to_function_name()` > or `os::dll_address_to_library_name()` can get quite confused (and sometimes > crash) > if an `addr` parameter of `-1` was allowed to be used. > > I've also made two cleanup changes as part of this fix: > > 1) In `src/hotspot/os/bsd/os_bsd.cpp` there is some macOS specific code that > should >be properly `#ifdef`'ed. There is also some code that makes sense for ELF > format >files, but not for Mach-O format files so that code needs to be excluded > on macOS. > > 2) In `src/hotspot/share/runtime/os.cpp` I noticed a simple typo in a comment > on an > `#endif` that I fixed. That typo does not appear anywhere else in the > HotSpot code > base so I'd like to fix it with this bug ID since I'm in related areas. > > This fix has been tested with Mach5 Tier[1-6]. Ping! Any takers? - PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Wed, 3 Nov 2021 09:50:08 GMT, Robbin Ehn wrote:
>> src/hotspot/share/runtime/handshake.cpp line 350:
>>
>>> 348: }
>>> 349:
>>> 350: void Handshake::execute(HandshakeClosure* hs_cl, ThreadsListHandle*
>>> tlh_p, JavaThread* target) {
>>
>> Nit: can we drop the `_p` part of `tlh_p` please.
>
> Yes, please.
Fixed in handshake.[ch]pp.
>> src/hotspot/share/runtime/thread.cpp line 1764:
>>
>>> 1762: guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */
>>> true),
>>> 1763: "missing ThreadsListHandle in calling context.");
>>> 1764: if (is_exiting()) {
>>
>> Can't we remove this the same as we did for `java_suspend()`?
>
> Yes, please
The rationale for removing the is_exiting() check from `java_suspend()` was
that it
was redundant because the handshake code detected and handled the `is_exiting()`
case so we didn't need to do that work twice.
If we look at `HandshakeState::resume()` there is no logic for detecting or
handling
the possibility of an exiting thread. That being said, we have to look closer
at what
`HandshakeState::resume()` does and whether that logic can be harmful if
executed
on an exiting thread.
Here's the code:
bool HandshakeState::resume() {
if (!is_suspended()) {
return false;
}
MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
if (!is_suspended()) {
assert(!_handshakee->is_suspended(), "cannot be suspended without a suspend
request");
return false;
}
// Resume the thread.
set_suspended(false);
_lock.notify();
return true;
}
I'm not seeing anything in `HandshakeState::resume()` that
worries me with respect to an exiting thread. Of course, the
proof is in the testing so I'll rerun the usual testing after
deleting that code.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Wed, 3 Nov 2021 01:21:50 GMT, David Holmes wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8249004.cr2.patch
>
> src/hotspot/share/runtime/thread.cpp line 446:
>
>> 444: Thread* current_thread = nullptr;
>> 445: if (checkTLHOnly) {
>> 446: current_thread = Thread::current();
>
> This seems redundant due to line 463. You can just have a `if
> (!checkTLHOnly)` block here.
I suspect that the way that git is displaying the diffs is confusing you.
We need `current_thread` set if we get to line 474 so we have to init
`current_thread` on line 446 for the `checkTLHOnly == true` case and
on line 463 for the `checkTLHOnly == false` case.
I could simplify the logic by always setting current thread when it is
declared on 444, but I was trying to avoid the call to `Thread::current()`
until I actually needed it. I thought `Thread::current()` can be expensive.
Is this no longer the case?
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8276208: vmTestbase/nsk/jdb/repeat/repeat001/repeat001.java fails with "AssertionError: Unexpected output" [v2]
On Tue, 2 Nov 2021 01:07:30 GMT, Jakob Cornell wrote: >> This will fix a few issues with the tests added in #5290: >> >> - [x] intermittent failures >> - [x] tests should use `failure` method to report problems rather than >> throwing `AssertionError` > > Jakob Cornell has updated the pull request incrementally with one additional > commit since the last revision: > > Fix incorrect use of `receiveReplyFor' causing test failures @iklam - Can you review this fix? You were one of the original reviewers on: JDK-8271356 Modify jdb to treat an empty command as a repeat of the previous command Thanks! - PR: https://git.openjdk.java.net/jdk/pull/6182
