RFR(S): 8027593: performance drop with constrained codecache starting with hs25 b111

[Albert Noll]

The previous mail contains an error. See inline.

Albert


On 11/07/2013 12:20 PM, Albert Noll wrote:
> Vladimir, Igor, John, thanks for looking at the patch.
> Here is  the updated webrev:
>
> http://cr.openjdk.java.net/~anoll/8027593/webrev.01/
>
> Please see comments inline.
>
>
> On 11/06/2013 02:52 AM, John Rose wrote:
>> Good idea.
>>
>> -- John  (on my iPhone)
>>
>> On Nov 5, 2013, at 3:11 PM, Igor Veresov <igor.veresov at oracle.com> 
>> wrote:
>>
>>> Looks good. It’s not related to the change, but could you please 
>>> consider adding some printing under PrintMethodFlushing && Verbose 
>>> for the case when the method is made not entrant and include hotness 
>>> and threshold values?
>>>
>>> igor
> I also agree. I added the print.
>>> On Nov 5, 2013, at 10:09 AM, Vladimir Kozlov 
>>> <vladimir.kozlov at oracle.com> wrote:
>>>
>>>> On 11/5/13 6:44 AM, Albert Noll wrote:
>>>>> Hi,
>>>>>
>>>>> could I get reviews for this small patch?
>>>>>
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8027593
>>>>> webrev: http://cr.openjdk.java.net/~anoll/8027593/webrev.00/
>>>>>
>>>>> Problem: The implementation of the sweeper (8020151) causes a 
>>>>> performance regression for small code cache sizes. There
>>>>> are two issues that cause this regression:
>>>>>   1) NmethodSweepFraction is only adjusted according to the 
>>>>> ReservedCodecacheSize if TieredCompilation is enabled. As a
>>>>> result, NmethodSweepFraction remains 16 (if TieredCompilation is 
>>>>> not used). This is way too large for small code cache
>>>>> sizes (e.g., <5m).
>>>>> 2) _request_mark_phase (sweeper.cpp) is initialized to false. As a 
>>>>> result, mark_active_nmethods() did not set
>>>>> _invocations and _current, which results in not invoking the 
>>>>> sweeper (calling sweep_code_cache()) at all. When
>>>>> TieredCompilation is enabled this was not an issue, since 
>>>>> NmethodSweeper::notify() (which sets _request_mark_phase) is
>>>>> called much more frequently.
>>>>>
>>>>> Solution: 1) Move setting of NmethodSweepFraction so that it is 
>>>>> always executed.
>>>> Good.
>>>>
> The place where I moved the adaption of NmethodSweepFraction is not 
> good, since the
> the code cache size is adapted later for tiered. The current version 
> should be fine.
>>>>> Solution: 2) Remove need_marking_phase(), 
>>>>> request_nmethod_marking(), and reset_nmetod_marking().
>>>>>                    I think that these checks are not needed since 
>>>>> 8020151, since we do stack scanning of
>>>>>                    active nmethods irrespective of the value of 
>>>>> what need_marking_phase() returns. Since
>>>>>                    the patch removes need_marking_phase() printing 
>>>>> out the warning (line 327 in
>>>>>                    sweeper.cpp) is incorrect, i.e., we continue to 
>>>>> invoke the sweeper. I removed the warning
>>>>>                    and the associated code.
>>>> Don't put mark_active_nmethods() under !UseCodeCacheFlushing 
>>>> condition. We always want to reclaim space in codecache.
> Done.
>>>> To do nmethod marking at each safepoint is fine (we  have to do it 
>>>> to make sure we did not miss live nmethods). But with your changes 
>>>> each safepoint triggers sweep. Do we really need sweep so 
>>>> frequently? Why to sweep if there were no nmethods state change and 
>>>> there is enough space in CodeCache. So I am not sure about removing 
>>>> _request_mark_phase, init it with 'true' is okay.
> I agree. The current patch contains a more sophisticated logic to 
> determine when we call the
> sweeper. The bottom line is that we do not invoke the sweeper only if:

!!!!We DO INVOKE the sweeper only if:
> (1) The code cache is getting full and/or
> (2) There are sufficient state changes in the last sweep.
!!!!!(3) We have not been sweeping for 'some time'
>
> The patch contains a detailed description + examples of the logic. I 
> tested the patch
> with small code cache sizes (specjvm98 + <4m code cache), medium-sized 
> code cache
> (128m + nashorn + octane), and large code cache (240m + nashorn + 
> octane). The measurements
> indicate that with the current logic in place, we can reduce the 
> number of sweeps by 50% for
> medium code cache sizes without increasing the maximum used code cache 
> size. The difference
> is more or less not significant.
>
> Please let me know what you think about it. The main disadvantage I 
> see with this change is that
> it makes reasoning about the sweeper harder than it was before.
>
>
>
>
>>>> The warning was useless so it is okay to remove it and 
>>>> corresponding code.
>>>>
>>>>>
>>>>> Also, I think that we can either remove -XX:MethodFlushing or 
>>>>> -XX:UseCodeCacheFlushing. Since 8020151, one of them is
>>>>> redundant and can be removed. I am not quite sure if we should do 
>>>>> that now so it is not included in the patch.
>>>> It is for separate change.
>>>>
>>>> MethodFlushing is obsolete because we can not run VM without 
>>>> codecache sweeping because we loose performance since we go into 
>>>> Interpreter after codecache filled. Did you tried to run with it 
>>>> OFF? I think it is good candidate to go.
>>>>
>>>> The problem with UseCodeCacheFlushing is it becomes famous so you 
>>>> can't remove it easily. But don't replace MethodFlushing with it. I 
>>>> think code which currently guarded by MethodFlushing should be 
>>>> executed unconditionally.
>>>>
>>>> In arguments.cpp there is table for obsolete flags:
>>>> static ObsoleteFlag obsolete_jvm_flags[] = {
>>>>
>>>> jdk8 is major release so we can change flags. Add MethodFlushing 
>>>> there to remove it in jdk9:
>>>> { "MethodFlushing", JDK_Version::jdk(8), JDK_Version::jdk(9) },
>>>>
>>>> Thanks,
>>>> Vladimir
> I'll file a new bug to remove the flag. I guess this change will most 
> likely only make it into 8uXX.
>>>>> Testing
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8027593 also shows a 
>>>>> performance evaluation.
>>>>>
>>>>> Many thanks for looking at the patch.
>>>>> Best,
>>>>> Albert
>