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

[Vladimir Kozlov]

On 11/7/13 1:37 PM, Albert Noll wrote:
> Hi,
>
>
> On 11/07/2013 08:39 PM, Vladimir Kozlov wrote:
>> On 11/7/13 11:04 AM, Igor Veresov wrote:
>>> I’d vote to put it under PrintCodeCache. And make the messages not
>>> warnings, but just “compiler disabled/enabled”. What do you think?
>>
>> Unfortunately there could be customer's tools which look for this
>> message. So changing it, at least now for jdk8, is not good. With
>> small codecache we will expect this message showing up. But with big
>> codecache it should not happen. I think we should keep it as warning
>> but throttle it when small codecache is used as Chris suggested.
>>
>> May be put it under combined check:
>>
>> if (PrintCodeCache || ReservedCodeCacheSize > X)
>>
>> Do we have a state now when we definitely will not compile any more?
>> Or we always making progress? I think it will be difficult to find
>> when it should be printed only once.
>>
> With the current version (when sweeper is enabled) we should not reach a
> state (unless the entire code cache is filled with OSR-methods or native
> methods) where we disable compilation and never enable it.
> As soon as we free memory from the code cache, we re-enable compilation.
> The message will be printed very frequently, if the code cache is
> significantly smaller than the application demands.
>
> We could solve the 'problem' also by adding code that prints the warning
> only if compilation is
> disabled for a certain time. The current patch (webrev.01) defines a
> virtual time for the sweeper (we increment time counter by one every
> time we call mark_active_nmethods), which we could use.

Or only print 10th (or whatever) message, first one must print.

Thanks,
Vladimir

>
> Best,
> Albert
>> Thanks,
>> Vladimir
>>
>>>
>>> igor
>>>
>>> On Nov 7, 2013, at 3:24 AM, Albert Noll <albert.noll at oracle.com> wrote:
>>>
>>>> Hi Chris,
>>>>
>>>> On 11/06/2013 03:18 AM, Chris Plummer wrote:
>>>>> BTW, one thing I forgot to mention is I now see a lot of messages
>>>>> for the codecache filling up. For example:
>>>>>
>>>>> Java HotSpot(TM) Client VM warning: CodeCache is full. Compiler has
>>>>> been disabled.
>>>>> Java HotSpot(TM) Client VM warning: Try increasing the code cache
>>>>> size using -XX:ReservedCodeCacheSize=
>>>>> CodeCache: size=2700Kb used=2196Kb max_used=2196Kb free=503Kb
>>>>>
>>>>> With b111, I was only seeing one message. I suspect with b111, once
>>>>> this message appeared compilation was never re-enabled so the
>>>>> message never appeared again. In that case seeing in many times now
>>>>> is actually a good indicator. However, it appears even when not
>>>>> using -XX:+PrintCodeCache, and I can see this output being a
>>>>> distraction for programs whose normal operation may involve
>>>>> constraining the codecache and having it constantly filling up.
>>>>> Perhaps this message should be off by default, or possibly only
>>>>> appear once.
>>>>>
>>>> You are right. The previous version just never re-enabled
>>>> compilation. I also agree that the
>>>> output is distracting. There are multiple ways to solve this issue.
>>>> I would go for a product -XX flag
>>>> which allows to turn this warning on/off. Would that be ok or do you
>>>> have a different solution in mind?
>>>>
>>>> Best,
>>>> Albert
>>>>
>>>>> cheers,
>>>>>
>>>>> Chris
>>>>>
>>>>> On 11/5/13 5:59 PM, Chris Plummer wrote:
>>>>>> Hi Albert,
>>>>>>
>>>>>> I applied your patch and got some new numbers. Performance is now
>>>>>> even better than it was with b110. See the chart I added to the bug.
>>>>>>
>>>>>> Nice work!
>>>>>>
>>>>>> Chris
>>>>>>
>>>>>> 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.
>>>>>>> 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.
>>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>>>>
>>>>>>> Testing
>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8027593 also shows
>>>>>>> a performance evaluation.
>>>>>>>
>>>>>>> Many thanks for looking at the patch.
>>>>>>> Best,
>>>>>>> Albert
>>>>>>
>>>>>
>>>>
>>>
>