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

[Vladimir Kozlov]

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
>