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

[John Rose]

To be complete, I am suggesting overloading the existing M::_code field not adding a new one, and not right now. 

-- John  (on my iPhone)

On Oct 10, 2013, at 12:46 PM, Albert Noll <albert.noll at oracle.com> wrote:

> 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
>