RFR(S): 8027593: performance drop with constrained codecache starting with hs25 b111
Albert Noll
albert.noll at oracle.com
Fri Nov 8 11:04:56 PST 2013
Thank you, Vladimir.
Best,
Albert
On 11/08/2013 06:38 PM, Vladimir Kozlov wrote:
> 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