Hi Dan, I'd be very happy about a review from somebody on the Serviceability team. I have asked for reviews many times (kindly I hope). And the change is for review for more than a year now.
According to [1] I'd think all requirements to push are met already. But maybe I missed something? After renaming of methods in SafepointMechanism the change needs to be rebased (already done). I'll publish a pull request as soon as possible. Thanks, Richard. [1] https://wiki.openjdk.java.net/display/HotSpot/Pushing+a+HotSpot+change -----Original Message----- From: Daniel D. Daugherty <daniel.daughe...@oracle.com> Sent: Dienstag, 8. September 2020 18:16 To: Reingruber, Richard <richard.reingru...@sap.com>; serviceability-dev <serviceability-dev@openjdk.java.net>; hotspot-compiler-...@openjdk.java.net; Hotspot dev runtime <hotspot-runtime-...@openjdk.java.net> Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, I haven't seen a review from anyone on the Serviceability team and I think you should get a review from them since JVM/TI is involved. Perhaps I missed it... Dan On 9/7/20 10:09 AM, Reingruber, Richard wrote: > Hi, > > I would like to close the review of this change. > > It has received a lot of helpful feedback during the process and 2 full > Reviews. Thanks everybody! > > I'm planning to push it this week on Thursday as solution for JBS items: > > https://bugs.openjdk.java.net/browse/JDK-8227745 > https://bugs.openjdk.java.net/browse/JDK-8233915 > > Version to be pushed: > > http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/ > > Hope to get my GIT/Skara setup going until then... :) > > Thanks, Richard. > > -----Original Message----- > From: hotspot-compiler-dev <hotspot-compiler-dev-r...@openjdk.java.net> On > Behalf Of Reingruber, Richard > Sent: Mittwoch, 2. September 2020 23:27 > To: Robbin Ehn <robbin....@oracle.com>; serviceability-dev > <serviceability-dev@openjdk.java.net>; hotspot-compiler-...@openjdk.java.net; > Hotspot dev runtime <hotspot-runtime-...@openjdk.java.net> > Subject: [CAUTION] RE: RFR(L) 8227745: Enable Escape Analysis for Better > Performance in the Presence of JVMTI Agents > > Hi Robin, > >> On 2020-09-02 15:48, Reingruber, Richard wrote: >>> Hi Robbin, >>> >>> // taking the discussion back to the mailing lists >>> >>> > I still don't understand why you don't deoptimize the objects inside >>> the >>> > handshake/safepoint instead? >> So for handshakes using asynch handshake and allowing blocking inside >> would fix that. (future fix, I'm working on that now) > Just to make it clear: I'm not fond of the extra suspension mechanism > currently > used for JDK-8227745 either. I want to get rid of it and I will work on it. > Asynch > handshakes (JDK-8238761) could be a replacement for it. At least I think they > can be used to suspend the target thread. > >> For safepoint, since we have suspended all threads, ~'safepointed them' >> with a JavaThread, you _could_ just execute the action directly (e.g. >> skipping VM_HeapWalkOperation safepoint) since they are suppose to be >> safely suspended until the destructor of EB, no? > Yes, this should be possible. This would be an advanced change though. I would > like EscapeBarriers to be a no-op and fall back to current implementation, if > C2-EscapeAnalysis/Graal are disabled. > >> So I suggest future work to instead just execute the safepoint with the >> requesting JT instead of having a this special safepoiting mechanism. >> Since you are missing above functionality I see why you went this way. >> If you need to push it, it's fine by me. > We will work on further improvements. Top of the list would > be eliminating the extra suspend mechanism. > > The implementation has matured for more than 12 months now [1]. It's been > tested > extensively at SAP over that time and passed also extended testing at Oracle > kindly conducted by Vladimir Kozlov. We've got two full Reviews and > incorporated > extensive feedback from a number of OpenJDK Reviewers (including you, > thanks!). Based on that I reckon we're good to push the change as enhancement > (JDK-8227745) and bug fix (JDK-8233915). > >> Thanks for explaining once again :) > Pleasure :) > > Thanks, Richard. > > [1] > http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-July/028729.html > > -----Original Message----- > From: Robbin Ehn <robbin....@oracle.com> > Sent: Mittwoch, 2. September 2020 16:54 > To: Reingruber, Richard <richard.reingru...@sap.com>; serviceability-dev > <serviceability-dev@openjdk.java.net>; hotspot-compiler-...@openjdk.java.net; > Hotspot dev runtime <hotspot-runtime-...@openjdk.java.net> > Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in > the Presence of JVMTI Agents > > Hi Richard, > > On 2020-09-02 15:48, Reingruber, Richard wrote: >> Hi Robbin, >> >> // taking the discussion back to the mailing lists >> >> > I still don't understand why you don't deoptimize the objects inside >> the >> > handshake/safepoint instead? > So for handshakes using asynch handshake and allowing blocking inside > would fix that. (future fix, I'm working on that now) > > For safepoint, since we have suspended all threads, ~'safepointed them' > with a JavaThread, you _could_ just execute the action directly (e.g. > skipping VM_HeapWalkOperation safepoint) since they are suppose to be > safely suspended until the destructor of EB, no? > > So I suggest future work to instead just execute the safepoint with the > requesting JT instead of having a this special safepoiting mechanism. > > Since you are missing above functionality I see why you went this way. > If you need to push it, it's fine by me. > > Thanks for explaining once again :) > > /Robbin > >> This is unfortunately not possible. Deoptimizing objects includes >> reallocating >> scalar replaced objects, i.e. calling Deoptimization::realloc_objects(). This >> cannot be done at a safepoint or handshake. >> >> 1. The vm thread is not allowed to allocate on the java heap >> See for instance assertions in ParallelScavengeHeap::mem_allocate() >> >> https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/4c73e045ce815d52abcdc99499266ccf2e6e9b4c/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp*L258__;Iw!!GqivPVa7Brio!K0f5chjtePI6MKBSBOoBKya9YZTJlVhsExQYMDO96v3Af_Klc_E4R26_dSyowotF$ >> >> This is not easy to change, I suppose, because it will be difficult to >> gc if >> necessary. >> >> 2. Using a direct handshake would not work either. The problem there is again >> gc. Let J be the JavaThread that is executing the direct handshake. The >> vm >> would deadlock if the vm thread waits for J to execute the closure of a >> handshake-all and J waits for the vm thread to execute a gc vm >> operation. >> Patricio Chilano made me aware of this: >> https://bugs.openjdk.java.net/browse/JDK-8230594 >> >> Cheers, Richard. >> >> -----Original Message----- >> From: Robbin Ehn <robbin....@oracle.com> >> Sent: Mittwoch, 2. September 2020 13:56 >> To: Reingruber, Richard <richard.reingru...@sap.com> >> Cc: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; Vladimir Kozlov >> <vladimir.koz...@oracle.com>; David Holmes <david.hol...@oracle.com> >> Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance >> in the Presence of JVMTI Agents >> >> Hi, >> >> I still don't understand why you don't deoptimize the objects inside the >> handshake/safepoint instead? >> >> E.g. >> >> JvmtiEnv::GetOwnedMonitorInfo you only should need the execute the code >> from: >> eb.deoptimize_objects(MaxJavaStackTraceDepth)) before looping over the >> stack, so: >> >> void >> GetOwnedMonitorInfoClosure::do_thread(Thread *target) { >> assert(target->is_Java_thread(), "just checking"); >> JavaThread *jt = (JavaThread *)target; >> >> if (!jt->is_exiting() && (jt->threadObj() != NULL)) { >> + if (EscapeBarrier::deoptimize_objects(jt, MaxJavaStackTraceDepth)) { >> _result = >> ((JvmtiEnvBase*)_env)->get_owned_monitors(_calling_thread, jt, >> _owned_monitors_list); >> } else { >> _result = JVMTI_ERROR_OUT_OF_MEMORY; >> } >> } >> } >> >> Why try 'suspend' the thread first? >> >> >> When we de-optimize all threads why not just in the following safepoint? >> E.g. >> VM_HeapWalkOperation::doit() { >> + EscapeBarrier::deoptimize_objects_all_threads(); >> ... >> } >> >> Thanks, Robbin >> >>