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

[Albert Noll]

Sure,

I was not aware that this is such a big deal. I'll run more tests and 
let you know.

Best,
Albert

On 10.10.2013 20:47, Vladimir Kozlov wrote:
> Albert,
>
> I don't think you should do such refactoring now. Runtime and Embedded 
> will put you on "fire" for adding a field to Method class :)
>
> Lets do your original changes only for now. And do more complete 
> solution suggested by John later for jdk9. File RFE.
>
> Thanks,
> Vladimir
>
> On 10/10/13 8:34 AM, Albert Noll wrote:
>> 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
>>