request for review (m): 4957990: Perm heap bloat in JVM

[Y.S.Ramakrishna at Sun.COM]

On 08/06/09 14:28, Tom Rodriguez wrote:
> 
> On Aug 5, 2009, at 11:10 PM, Y. Srinivas Ramakrishna wrote:
> 
>> Tom Rodriguez wrote:
>>> After I wrote this I realized that this is a natural out growth of 
>>> your fix.  Without profiling information an nmethod should only refer 
>>> to types which are statically reachable.  Having the MDO act as a 
>>> strong root would keep the nmethod from getting unloaded when using 
>>> predicted call sites.  Once the MDO is a weak root then the nmethod 
>>> could want to be unloaded.  I guess without your fix CHA could 
>>> produce a case where an OSR nmethod would be unloaded without being 
>>> unlinked from the OSR nmethod list but I'd have to think about that 
>>> more.
>>
>> So, Tom, is my understanding correct that the criteria for unloading 
>> of the nmethod are not too
>> weak. Do you suggest that make_unloaded should just go unlink the 
>> nmethod from the osr list?
>> I'll give that a try tonight and update later on how that looks wrt 
>> the current test,
>> but let me know if i am misunderstanding and that would not  be the 
>> correct way to
>> fix this problem,

Bummer! The "fix", of removing the osr method (and marking it non-entrant)
when GC decides to unload it, was working really nicely with this test!
(I was in the midst of running more extensive tests.)
I was hoping you'd bless it!

> 
> We can't allow the sweeper to free an nmethod unless we are certain that 
> it's impossible for new activations of that nmethod to form.  Note this 
> comment:
> 
> void nmethod::make_not_entrant_or_zombie(int state) {
>   assert(state == zombie || state == not_entrant, "must be zombie or 
> not_entrant");
> 
>   // 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.
> 
> So I don't think reclamation of OSR nmethods in the current code is 
> safe.  I do believe that OSR nmethods could be reclaimed like other 
> normal nmethods and I have an old webrev that illustrates this at 
> http://javaweb.sfbay.sun.com/~never/webrev/osr.  I think that the 
> changes in the assembly code are unneeded since the requirement we're 
> trying to enforce is that if an OSR nmethod becomes unloaded at a 
> safepoint it won't be used for form a new activation which is 
> accomplished by the interpeterRuntime.cpp changes.  I'm not sure we want 
> to include the reclaiming of OSR nmethods into hs16 at this point.  So 
> maybe OSR nmethods need to act as strong roots for now?

I agree that it is too close to hs16b01 for this. However,
we have a few weeks more for hs16, so i think this is a good
opportunity to include the long-term fix of reclaiming OSR nmethods along the
lines of yr webrev above and to roll it in with 4957990.
If it passes muster we could consider putting it in hs16b2.
I'll take this up with you offline.

IMHO, I see little point in the interim step of making OSR methods
always act as strong roots and never become eligible for reclamation.
Naively, that would see to me to be almost worse than the original
problem we were trying to fix in 4957990.

-- ramki

> 
> tom
> 
>>
>> thanks!
>> -- ramki
>>>
>>> tom
>>>
>>> On Aug 5, 2009, at 6:15 PM, Tom Rodriguez wrote:
>>>
>>>> My understanding was that OSR nmethods could/should only be 
>>>> reclaimed once the klass of their holder was unloaded.  The reason 
>>>> for this is that the normal not_entrant sweeping logic isn't 
>>>> implemented safely for OSR methods so the sweeper can't safely 
>>>> reclaim them.  If the klass itself is being reclaimed then it should 
>>>> be safe to reclaim the OSR nmethods.  It's possible that we have a 
>>>> hole in the logic which doesn't account for dealing with unloaded 
>>>> OSR methods properly and you changes just make it more likely to 
>>>> happen.  We either need to close the not_entrant hole for OSR 
>>>> nmethods, which isn't that hard, and then correct 
>>>> nmethod::make_unloaded to unlink the OSR nmethod or we have to make 
>>>> do_unloading disallow unloading for OSR nmethods is the methodOop is 
>>>> strongly reachable.
>>>>
>>>> tom
>>>>
>>>> On Aug 5, 2009, at 4:57 PM, Y.S.Ramakrishna at Sun.COM wrote:
>>>>
>>>>>
>>>>> Can there be a situation where an osr nmethod associated
>>>>> with a method oop gets unloaded while the method and its
>>>>> class are still alive? I see that happening with my workspace
>>>>> with the changes for 4957990 (yet to figure out why the osr
>>>>> nmethod was unloaded).
>>>>>
>>>>> Shortly after said unload, the sweeper flushes the nmethod,
>>>>> but the nmethod is still left linked off of the klass's 
>>>>> _osr_nmethod_head
>>>>> list, and a subsequent invocation counter overflow at a bci does
>>>>> an osr nmethod lookup and falls foul of the flushed nmethod left
>>>>> in the instanceKlass's osr nmethod head.
>>>>>
>>>>> So I have two questions:
>>>>>
>>>>> (a) can an osr nmethod be unloaded while its klass is still alive ?
>>>>> (b) if the answer to (a) is yes, then we need to take special steps
>>>>> (not present in current code) to unlink said nmethod from the
>>>>> list of osr nmethods in its klass.
>>>>>
>>>>> Am I making sense? Otherwise i can provide more direct detail.
>>>>> (I am still digging to find out why we decided to unload the
>>>>> nmethod and will have more follow-up info in a subsequent email.)
>>>>>
>>>>> thanks.
>>>>> -- ramki
>>>>
>>>
>>
>