RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents

Tue Sep 8 16:45:15 UTC 2020

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.daugherty at oracle.com> 
Sent: Dienstag, 8. September 2020 18:16
To: Reingruber, Richard <richard.reingruber at sap.com>; serviceability-dev <serviceability-dev at openjdk.java.net>; hotspot-compiler-dev at openjdk.java.net; Hotspot dev runtime <hotspot-runtime-dev at 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-retn at openjdk.java.net> On Behalf Of Reingruber, Richard
> Sent: Mittwoch, 2. September 2020 23:27
> To: Robbin Ehn <robbin.ehn at oracle.com>; serviceability-dev <serviceability-dev at openjdk.java.net>; hotspot-compiler-dev at openjdk.java.net; Hotspot dev runtime <hotspot-runtime-dev at 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.ehn at oracle.com>
> Sent: Mittwoch, 2. September 2020 16:54
> To: Reingruber, Richard <richard.reingruber at sap.com>; serviceability-dev <serviceability-dev at openjdk.java.net>; hotspot-compiler-dev at openjdk.java.net; Hotspot dev runtime <hotspot-runtime-dev at 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.ehn at oracle.com>
>> Sent: Mittwoch, 2. September 2020 13:56
>> To: Reingruber, Richard <richard.reingruber at sap.com>
>> Cc: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Vladimir Kozlov <vladimir.kozlov at oracle.com>; David Holmes <david.holmes at 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
>>    
>>