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

[Albert Noll]

Thank you John, now I finally got it.
I'll file an RFE as proposed by Vladimir.

Best,
Albert

On 11.10.2013 00:04, John Rose wrote:
> 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