RFR (S/M): 8012547: Reclamation of flushed methods can be slow

[Nils Eliasson]

On 2013-04-24 22:33, Vladimir Kozlov wrote:
> Nils,
>
> Yes, in general it is better.
>
> http://cr.openjdk.java.net/~neliasso/8012547/webrev.04/
>
> On 4/24/13 7:06 AM, Nils Eliasson wrote:
>> Hi again,
>>
>> Further testing revealed that not all issues was fully identified.
>> Flushing could still cause compiles to halt even though compilations was
>> on, and turning off compilations when the code cache was really full
>> made the sweeper run much less often, prolonging the problem.
>>
>> Change log:
>> * Fix the comment Christian mentioned, and some other comments too.
>> * find_and_remove_saved_code renamed to reanimate_saved_code to
>> distinguish it from remove_saved_code that actually removes it.
>
> Please, add comments to those CodeCache methods to describe what they do.

ok - fixed.

>> * dont bail out on need_flushing() in compile_method - that causes us to
>> stop compiling as soon as we hit CodeCacheFlushingMinimumFreeSpace - and
>> that wasn't the idea.
>
> Then you should not check UseCodeCacheFlushing flag and the comment 
> should be updated.

ok, I removed the flag and the capacity check. When we can hit it - 
compilation should already be turned off. And if it isn't it will be in 
a moment.

>
>> * New product flag CodeCacheFlushingFraction (defaults to 2)
>> * _was_full removed. Was a redundant way to say that we stopped
>> compilations.
>> * _last_was_full renamed to _last_full_flush_time, to emphasize that it
>> is the time when we hit the hard limit CodeCacheMinimumSpace and
>> compilations are stopped
>> * _was_full_traversal renamed to _last_flush_traversal_id. 'full' means
>> the hard CodeCacheMinimumFreeSpace limit in all other variable names. 
>> * introduced _dead_compile_ids that hold a count for the number of
>> compile ID that have been reclaimed. Gives a more accurate way to flush
>> the cache.
>
> Remove space in "jint       _flush_token;" to align it. Also align 
> comments.
Fixed
>
>> * Simplified scan_stacks
>>    - restore flush token when min interval has passed
>>    - start compilations again if flushing is no longer needed
>> * Speed up sweeper when compilations is turned off. Sweeping is slowed
>> down when compiler threads are sleeping on the CodeCache lock, and we
>> want to speed up the reclaim process so that we can turn on compiler 
>> again.
>> * Let sweeper make disconnected methods non_entrant even if we don't
>> need flushing any more. Makes us less likely to hit a new flush.
>
> You removed compilation enabling code in 
> speculative_disconnect_nmethods() so we need to wait next safepoint to 
> enable it (in scan_stacks()). Should we do enabling compilation during 
> sweep may be in an other place when we know that some space is 
> reclaimed to avoid waiting safepoint?

Yes, my though was that !CodeCache::needs_flushing() will never be true 
since this code is only called when we need flushing. And the only place 
where code cache is actually freed is in scan_stacks when nmethods are 
flushed. So it should be enough to check for turning on the compiler as 
the last thing there.

>
>> * Refactored NMethodSweeper::handle_full_code_cache
>>    - flushed and full flushes now uses the same token to synchronize -
>> otherwise we might get two back-to-back flushes. And that doesn't help
>> anyone.
>>    - remove time check - flush_token is restored in scan_stacks where
>> the time is already checked.
>> * set _resweep flag when flushing.
>>
>> The code is much easier to read now and the behavior should be less
>> surprising.
>
> Is that '/* */' leftover from your debugging?:
>
> 238   if (PrintMethodFlushing /*&& Verbose*/) {
>
Yes, I forgot to qrefresh my patch before doing the webrev.

> Thanks,
> Vladimir

New webrev with your and Christians feedback here:

http://cr.openjdk.java.net/~neliasso/8012547/webrev.07 
<http://cr.openjdk.java.net/%7Eneliasso/8012547/webrev.07>

Thanks for all your help!
//Nils
>
>>
>> http://cr.openjdk.java.net/~neliasso/8012547/webrev.03/
>> <http://cr.openjdk.java.net/%7Eneliasso/8012547/webrev.03/>
>>
>> Thanks,
>> Nils Eliasson
>>
>>
>> On 2013-04-18 15:09, Nils Eliasson wrote:
>>> Hi!
>>>
>>> I have another fix to the code cache sweeper and flushing that needs a
>>> review.
>>>
>>> The major change is to remove a check in scan_stacks that stops the
>>> sweeper when the cache is getting  full. The normal mode is to sweep
>>> and record if any change has happened that require another sweep. This
>>> check stops the sweeper early causing some methods that are
>>> speculatively disconnected to stay so for an unnecessary long time
>>> sometimes causing unnecessary new flushes.
>>>
>>> Also some refactoring
>>> - remove state variable _do_sweep that was unnecessary. It marked if a
>>> sweep was active or not, but just a duplicate way of checking if any
>>> methods are being sweept (nmethodsweeper::current != NULL).
>>> - rename _rescan to _resweep. When _resweep is set there will be
>>> another sweep started when the current ends. That sweep will start
>>> with a scan, but it is not only a scan.
>>> - rename _advise_to_sweep to _flush_token. Is CASed by the first
>>> thread that wants to flush and reset by scan_stacks when the flush is
>>> finished and a proper time has passed.
>>>
>>> http://cr.openjdk.java.net/~neliasso/8012547/
>>> <http://cr.openjdk.java.net/%7Eneliasso/8012547/>
>>> https://jbs.oracle.com/bugs/browse/JDK-8012547
>>>
>>> Thanks,
>>> Nils E.
>>