RFR (XXS): 8023191: OSR nmethods should be flushed to free space in CodeCache

[Albert Noll]

Vladimir, Chris, John, many thanks for your feedback.

I'll run more tests to show that removing OSR methods in the sweeper is 
stable (or not).
So far, I have not run into problems.

@Chris, maybe you ran into 8024008 when you ran nashorn with a small 
code cache size.

There is one thing I do not understand in the current implementation. 
What is the reason
that OSR nmethods are maintained in InstanceKlass and not in Method?
I do not see a reason but maybe I am missing something.

Another thing I do not understand: When we remove an OSR method from the 
list (in InstanceKlass),
we search for the max_comp level of all OSR nmethods of that class and 
then set this max_level as
the max_level of the Method. Why would we set max_comp_level of Method X 
to max_comp_level of
Method Y?

I've an updated webrev, which moves the maintaining of the list of OSR 
methods from InstanceKlass to
Method. This seems more logical to me. Moving the code to Method, also 
fixes the problem (if it is a
problem) mentioned in the previous paragraph.

Here is the webrev:
http://cr.openjdk.java.net/~anoll/8023191/webrev.01/ 
<http://cr.openjdk.java.net/%7Eanoll/8023191/webrev.01/>

Tests are still running but so far everything looks good.
Thanks in advance for your help.

Best,
Albert

On 10.10.2013 00:18, Vladimir Kozlov wrote:
> On 10/9/13 10:21 AM, Vladimir Kozlov wrote:
>> Albert,
>>
>> Changes are fine but you need more testing.
>>
>> Run full CTW too and all jtreg tests from Hotspot and jdk (run jdk tests
>> with -Xcomp and without it).
>>
>> Also the bug report has link to test (jittest). Can you build and run it
>> too to see if it solves the problem with small codecache? It fills
>> codecache with osr nmethods.
>>
>> It is very delicate matter. There was a reason we did not remove OSR
>> nmethod until corresponding klass is unloaded (yes, deoptimization can
>> make it non-entrant). Unfortunately I don't remember it. May be John
>> know. Non of inline caches are pointing to OSR nmethods since it is
>> called only from Interpreter. There is transition phase at the start of
>> OSR nmethod when it calls runtime to convert interpreter frame to
>> compiled frame. And method has separate field for osr nmethods.
>
> I think I dig out the answer.
>
> First, the last statement in previous mail is wrong. The method object 
> does not have reference to osr nmethods. Each java method can have 
> several alive OSR nmethods for different bcis. Instead method's holder 
> class has list of all OSR nmethods associated with all methods of this 
> class. Each time we do osr lookup we go through this list.
> Since method does not have reference to OSR the original flushing 
> (speculative disconnection) could not be implemented for OSR nmethods.
>
> Second. Originally osr nmethod were not cleaned up from codecache. 
> Here is the comment and the code in 
> nmethod::make_not_entrant_or_zombie() from long time ago:
>
>   // Code for an on-stack-replacement nmethod is removed when a class 
> gets unloaded.
>   // They never become zombie/non-entrant, so the nmethod sweeper will 
> never remove
>   // them. Instead the entry_bci is set to InvalidOSREntryBci, so the 
> osr nmethod
>   // will never be used anymore. That the nmethods only gets removed 
> when class unloading
>   // happens, make life much simpler, since the nmethods are not just 
> going to disappear
>   // out of the blue.
>   if (is_osr_only_method()) {
>     if (osr_entry_bci() != InvalidOSREntryBci) {
>       // only log this once
>       log_state_change(state);
>     }
>     invalidate_osr_method();
>     return;
>   }
>
> This code was changed by Tom for 5057818 4 years ago to allow cleanup 
> unused OSR nmethods from codecache:
>
> http://hg.openjdk.java.net/hsx/hotspot-comp/hotspot/rev/032260830071
>
> Third, I think those changes are not complete. 
> nmethod::invalidate_osr_method() should check for _entry_bci == 
> InvalidOSREntryBci to avoid lookup through osr list again after 
> nmethod change from non_entrant to zombie (invalidate_osr_method() is 
> called each time make_not_entrant_or_zombie() is called). May be other 
> places. Note, when we process osr nmethod::_code should not be touched 
> since it is pointing to normal compiled nmethod which could still be 
> alive.
>
> Thanks,
> Vladimir
>
>>
>> Thanks,
>> Vladimir
>>
>> On 10/8/13 10:18 PM, Albert Noll wrote:
>>> Hi,
>>>
>>> could I have a review for this small patch?
>>>
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8023191
>>> webrev: http://cr.openjdk.java.net/~anoll/8023191/webrev.00/
>>> <http://cr.openjdk.java.net/%7Eanoll/8023191/webrev.00/>
>>>
>>> Problem: OSR methods are not removed from code cache to free space.
>>> Solution: Remove OSR methods from code cache. OSR methods c an be made
>>>                 not-entrant and are then removed from the code cache
>>> likenon-OSR
>>>                 methods. Other parts in the code (e.g.,
>>> deoptimization.cpp:1547) already make
>>>                 OSR methods not-entrant.
>>>
>>> Testing: Passed JPRT; I'll also run specjvm and nashorn to see if there
>>> is an impact on performance.
>>>
>>> Many thanks in advance,
>>> Albert

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20131010/aec6770a/attachment-0001.html