RFR(XXS): 8244278: Excessive code cache flushes and sweeps

[Nils Eliasson]

Hi again,

I have now posted request for reviews for:

8244658: Remove dead code in code cache sweeper - 
http://cr.openjdk.java.net/~neliasso/8244658/webrev.01/
8244660: Code cache sweeper heuristics is broken - 
http://cr.openjdk.java.net/~neliasso/8244660/webrev.01/

Man - Your patch applies on top of these. When all have passed reviews, 
I will push your patch together with my patches so that they end up in 
the same build.

Please try this new heuristics out and give me feedback.

Best regards,
Nils Eliasson



On 2020-05-07 11:31, Nils Eliasson wrote:
>
>
> On 2020-05-06 22:20, Man Cao wrote:
>> Hi,
>>
>> [@Laurent]
>>> Thanks Man for your results.
>>> I will try your fix on jdk15 repo and run my Marlin tests & 
>>> benchmark to
>> see if there are any gain in these cases.
>> You are welcome.
>> I have run DaCapo at JDK tip, with default JVM options. I didn't see any
>> noticeable difference in performance with and without my bugfix.
>> This is probably due to significantly reduced code cache flushes with a
>> large default ReservedCodeCacheSize (240MB) for +TieredCompilation.
>> I checked the logs for tradesoap for runs without my bugfix, to count 
>> the
>> number of completed flushes (NMethodSweeper::sweep_code_cache()):
>> ~550 for runs with -XX:-TieredCompilation 
>> -XX:ReservedCodeCacheSize=40m on
>> JDK11
>> ~35 for runs with default options on JDK tip
>> (+TieredCompilation, ReservedCodeCacheSize=240m)
>> The flushes are reduced by more than 15X with the default options.
>>
>> [@Nils]
>>> Looking at sweeper.cpp I see something that looks wrong. The 
>>> _last_sweep
>> zzcounter is updated even if no sweep was done. In low code cache usage
>> scenarios that means will might never reach the threshold.
>>> Can you try it out and see if things improve?
>> The change makes sense to me. I can try it out together after 
>> resolving the
>> next issue.
>>
>>> The sweeper should wake up
>>> regularly, but now it is only awakened when hitting the SweepAggressive
>>> threshold. This is wrong.
>>> I suggest holding of the fix until all the problems have been ironed 
>>> out.
>> Could you elaborate what is the expected frequency to wake up the 
>> sweeper?
>> Should we increase the default value for StartAggressiveSweepingAt 
>> instead?
> The expected behaviour is that sweeper should be invoked depending on 
> how many sweeper-stack scan has been done - that is tracked by the 
> _time_counter field. In NMethodSweeper::possibly_sweep there is a 
> heuristics that triggers a sweep more often if the code cache is 
> getting full.
>
> The expected behaviour for StartAggressiveSweepingAt is that we 
> possibly start a sweep with a stack scan on every codeblob/nmethod 
> allocation when the code cache is getting full. That is an attempt to 
> quickly free up additional space in the code cache.
>
> The first bug I found is that the _last_sweep counter should only be 
> set when a sweep has been performed, otherwise the threshold might 
> never be reached.
>
> The second bug is that the sweeper thread sleeps. The only way to wake 
> it up is through code paths that can only be reached when it is awake, 
> or when StartAggressiveSweepingAt has been reached. This bug has gone 
> unnoticed because the bug you found made SweepAggressive trigger when 
> the codecache is 10% full, which most often happen fairly quickly.
>
> To fix this the sweeper thread needs to be awakened whenever 
> _time_counter has been updated.
>
> However - that can only be a temporary fix - the transition to using 
> mostly handshakes for synchronization can make the safepoints very 
> rare. And then the heuristics is broken again.
>
> Best regards,
> Nils Eliasson
>
>>
>> As you suggested before, currently the sweeper is awakened for every new
>> allocation in the code cache, after the usage is above 10%.
>> This is definitely too frequent that it hurts performance, especially 
>> for
>> the -XX:-TieredCompilation case.
>> We do find that turning off code cache flushing in JDK11
>> (-XX:-UseCodeCacheFlushing)
>> could significantly improve performance, by ~20% for an important
>> production workload configured with -XX:-TieredCompilation!
>> Thus, we strongly support keeping the default flushing frequency low.
>>
>> -Man
>