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

Albert Noll albert.noll at oracle.com
Thu Nov 7 03:20:00 PST 2013

Vladimir, Igor, John, thanks for looking at the patch.
Here is  the updated webrev:


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.
>>> 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:

(1) The code cache is getting full and/or
(2) There are sufficient state changes in the last sweep.

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