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

[David Holmes]

On 13/12/2019 9:02 am, Reingruber, Richard wrote:
> Hello Vladimir,
> 
> thanks for having a look.
> 
>    > Use vm.compMode == "Xmixed" instead of vm.compMode != "Xcomp" to skip
>    > test from running in Interpreter mode too.
> 
> Done.
> 
>    > You don't need vm.opt.TieredCompilation != true in @requires because you
>    > specified -XX:-TieredCompilation in @run command.
> 
> Ok.
> 
>    > The test is specifically written for C2 only (not for C1 or Graal) to
>    > verify its Escape Analysis optimization.
>    > I did not look in great details into test's code but its analysis may be
>    > affected if C1 compiler is also used.
>    >
>    > Richard may clarify this.
> 
> The test cases aim to get their testmethod 'dontinline_testMethod' compiled by C2. If they get C1
> compiled before doesn't matter all that much. I've got a slight preference to disabled tiered
> compilation for simplicity.

My concern - perhaps unfounded - is that this seems to be being tested 
only in a pure C2 environment when the actual changes will have to 
operate correctly in a tiered environment (and JVMCI).

Thanks,
David

> Thanks, Richard.
> 
> -----Original Message-----
> From: Vladimir Kozlov <vladimir.kozlov at oracle.com>
> Sent: Donnerstag, 12. Dezember 2019 19:20
> To: David Holmes <david.holmes at oracle.com>; hotspot-runtime-dev at openjdk.java.net; hotspot-compiler-dev at openjdk.java.net; serviceability-dev at openjdk.java.net; Reingruber, Richard <richard.reingruber at sap.com>
> Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
> 
> Hi David,
> 
> Tiered is disabled because we don't want to see compilations and outputs
> from C1 compiler which does not have EA.
> 
> The test is specifically written for C2 only (not for C1 or Graal) to
> verify its Escape Analysis optimization.
> I did not look in great details into test's code but its analysis may be
> affected if C1 compiler is also used.
> 
> Richard may clarify this.
> 
> thanks,
> Vladimir
> 
> On 12/11/19 1:04 PM, David Holmes wrote:
>> On 12/12/2019 5:21 am, Vladimir Kozlov wrote:
>>> I will do full review later. I want to comment about test command line.
>>>
>>> You don't need vm.opt.TieredCompilation != true in @requires because
>>> you specified -XX:-TieredCompilation in @run command.
>>
>> And per my comment this should be being tested with tiered as well.
>>
>> David
>>
>>> Use vm.compMode == "Xmixed" instead of vm.compMode != "Xcomp" to skip
>>> test from running in Interpreter mode too.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 12/11/19 7:07 AM, Reingruber, Richard wrote:
>>>> Hi David,
>>>>
>>>>     > Most of the details here are in areas I can comment on in
>>>> detail, but I
>>>>     > did take an initial general look at things.
>>>>
>>>> Thanks for taking the time!
>>>>
>>>>     > The only thing that jumped out at me is that I think the
>>>>     > DeoptimizeObjectsALotThread should be a hidden thread.
>>>>     >
>>>>     > +  bool is_hidden_from_external_view() const { return true; }
>>>>
>>>> Yes, it should. Will add the method like above.
>>>>
>>>>     > Also I don't see any testing of the DeoptimizeObjectsALotThread.
>>>> Without
>>>>     > active testing this will just bit-rot.
>>>>
>>>> DeoptimizeObjectsALot is meant for stress testing with a larger
>>>> workload. I will add a minimal test
>>>> to keep it fresh.
>>>>
>>>>     > Also on the tests I don't understand your @requires clause:
>>>>     >
>>>>     >   @requires ((vm.compMode != "Xcomp") & vm.compiler2.enabled &
>>>>     > (vm.opt.TieredCompilation != true))
>>>>     >
>>>>     > This seems to require that TieredCompilation is disabled, but
>>>> tiered is
>>>>     > our normal mode of operation. ??
>>>>     >
>>>>
>>>> I removed the clause. I guess I wanted to target the tests towards
>>>> the code they are supposed to
>>>> test, and it's easier to analyze failures w/o tiered compilation and
>>>> with just one compiler thread.
>>>>
>>>> Additionally I will make use of
>>>> compiler.whitebox.CompilerWhiteBoxTest.THRESHOLD in the tests.
>>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>> -----Original Message-----
>>>> From: David Holmes <david.holmes at oracle.com>
>>>> Sent: Mittwoch, 11. Dezember 2019 08:03
>>>> To: Reingruber, Richard <richard.reingruber at sap.com>;
>>>> serviceability-dev at openjdk.java.net;
>>>> hotspot-compiler-dev at openjdk.java.net;
>>>> 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 11/12/2019 7:45 am, Reingruber, Richard wrote:
>>>>> Hi,
>>>>>
>>>>> I would like to get reviews please for
>>>>>
>>>>> http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.3/
>>>>>
>>>>> Corresponding RFE:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8227745
>>>>>
>>>>> Fixes also https://bugs.openjdk.java.net/browse/JDK-8233915
>>>>> And potentially https://bugs.openjdk.java.net/browse/JDK-8214584 [1]
>>>>>
>>>>> Vladimir Kozlov kindly put webrev.3 through tier1-8 testing without
>>>>> issues (thanks!). In addition the
>>>>> change is being tested at SAP since I posted the first RFR some
>>>>> months ago.
>>>>>
>>>>> The intention of this enhancement is to benefit performance wise
>>>>> from escape analysis even if JVMTI
>>>>> agents request capabilities that allow them to access local variable
>>>>> values. E.g. if you start-up
>>>>> with -agentlib:jdwp=transport=dt_socket,server=y,suspend=n, then
>>>>> escape analysis is disabled right
>>>>> from the beginning, well before a debugger attaches -- if ever one
>>>>> should do so. With the
>>>>> enhancement, escape analysis will remain enabled until and after a
>>>>> debugger attaches. EA based
>>>>> optimizations are reverted just before an agent acquires the
>>>>> reference to an object. In the JBS item
>>>>> you'll find more details.
>>>>
>>>> Most of the details here are in areas I can comment on in detail, but I
>>>> did take an initial general look at things.
>>>>
>>>> The only thing that jumped out at me is that I think the
>>>> DeoptimizeObjectsALotThread should be a hidden thread.
>>>>
>>>> +  bool is_hidden_from_external_view() const { return true; }
>>>>
>>>> Also I don't see any testing of the DeoptimizeObjectsALotThread. Without
>>>> active testing this will just bit-rot.
>>>>
>>>> Also on the tests I don't understand your @requires clause:
>>>>
>>>>     @requires ((vm.compMode != "Xcomp") & vm.compiler2.enabled &
>>>> (vm.opt.TieredCompilation != true))
>>>>
>>>> This seems to require that TieredCompilation is disabled, but tiered is
>>>> our normal mode of operation. ??
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Thanks,
>>>>> Richard.
>>>>>
>>>>> [1] Experimental fix for JDK-8214584 based on JDK-8227745
>>>>> http://cr.openjdk.java.net/~rrich/webrevs/2019/8214584/experiment_v1.patch
>>>>>
>>>>>