RFR(S): 8027593: performance drop with constrained codecache starting with hs25 b111
Vladimir Kozlov
vladimir.kozlov at oracle.com
Fri Nov 8 09:38:38 PST 2013
This is really good.
Thanks,
Vladimir
On 11/8/13 7:32 AM, Albert Noll wrote:
> Hi Vladimir,
>
> thanks for looking at the patch. I hope that this version addresses all issues that
> you brought up. Here is a high-level overview of the changes since the last version:
>
> * We keep track of code cache state changes that also happen outside the sweeper.
> I re-installed notify, which is now called report_state_change() and is doing the job.
> report_state_change() checks if enough state has changed and enables the sweeper
> (it sets _should_sweep) to true. We reset _bytes_changed only once we have finished
> a while sweep cycle. That seems to make sense to me.
>
> * I added code that prints out every 10th warning that the compiler has been disabled.
> I also added a warning that compilation has been enabled again. I think if we print a message
> that compilation has been disabled, we should also print a message (maybe not a warning)
> that was enabled again.
>
> Here is the new webrev:
> http://cr.openjdk.java.net/~anoll/8027593/webrev.02/
>
> Best,
> Albert
>
> On 11/07/2013 11:04 PM, Vladimir Kozlov wrote:
>> On 11/7/13 1:36 PM, Vladimir Kozlov wrote:
>>> Nice work, Albert
>>>
>>> One concern is transition "alive -> not_entrant" is counted only when
>>> the nmethod needs to be flushed because you removed in notify() in
>>> nmethod::make_not_entrant_or_zombie(). Also make_zombie() is called from
>>> other places.
>>> I think _bytes_changed should be updated by NMethodSweeper::notify() so
>>> don't remove it from nmethod.cpp. _bytes_changed should be reset when we
>>> finished sweep instead of before each sweep.
>>
>> May be reset in both places actually. One to check that there were updates between sweeps and an other during sweep
>> (as you do currently).
>>
>> Thanks,
>> Vladimir
>>
>>>
>>> Can you keep comments in code which initialize static variables (first
>>> changes in sweeper.cpp)?
>>>
>>> Please narrow your main comment, chars 90 chars per line.
>>>
>>> What is the second place? (initialization should not be count):
>>>
>>> + // of the two places where should_sweep can be set to true.
>>>
>>> You need to clear state in the comment conditions when we sweep. Like
>>> you did in the replay:
>>>
>>> (1) The code cache is getting full
>>> (2) There are sufficient state changes in the last sweep.
>>> (3) We have not been sweeping for 'some time'
>>>
>>> Split into 2 lines:
>>>
>>> + int wait_until_next_sweep = (ReservedCodeCacheSize / (16 * M)) -
>>> time_since_last_sweep - CodeCache::reverse_free_ratio();
>>>
>>> In the print format do not use %p, use PTR_FORMAT for 'nm'.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 11/7/13 3:27 AM, Albert Noll wrote:
>>>> 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
>>>>>
>>>>
>
More information about the hotspot-compiler-dev
mailing list