for review (M): 6863023: need non-perm oops in code cache for JSR 292

[Tom Rodriguez]

On Aug 25, 2009, at 1:42 AM, John Rose wrote:

> On Aug 11, 2009, at 11:31 AM, Tom Rodriguez wrote:
>
>> I don't really like the non_perm naming being baked into this.  I  
>> expect at some point we'll want/need to fix this to have a more  
>> refined notion of whether the GC needs to scan the nmethods that  
>> won't be based on is_perm.
>
> OK, that's fair.
>
>> I expect that a lot of nmethods will start ending up on the list  
>> and scanning a significant portion of the code cache twice for  
>> every scavenge seems like an overhead we'd want to avoid.
>
> The new root list is used instead of walking the whole code cache.   
> Both scans don't need to happen in the same operation.  So I don't  
> get what you mean here by "scanning twice".

Once for marking and once to update any pointers.

>> I don't think your marking changes are right.  I think the new rule  
>> has to be that during a scavenge a the non-perm subset of nmethods  
>> are scanned as strong roots and during a full collection all  
>> nmethods are treated as weak roots as they are right now.  We could  
>> make them be weak during a scavenge but then we'd need to perform  
>> the do_unloading/make_unloaded logic during a scavenge and it seems  
>> like we should avoid that.  Given this I think the call to  
>> non_perm_nmethod_oops_do in psMarkSweep is wrong since it makes  
>> them into strong roots.  As far as I can tell the others are  
>> probably right though I don't claim to understand the layering in  
>> our gcs.
>
> That's a good catch.  Thanks for explaining it.  I agree that making  
> those nmethods strong roots will inhibit class unloading for the  
> affected code (that which uses invokedynamic).
>
> Most of the extra oops we're putting into the nmethods are also  
> rooted in the method's constant pool, so they won't go dead until  
> the enclosing nmethod also needs to get unloaded.  Some of the extra  
> oops might be rooted uniquely in the nmethod, because they are  
> optimistic tests on cases that no longer exist.  This is similar to  
> what happens to our type-predicted virtual calls, when the predicted  
> type disappears.  At that point, the nmethod holds the last  
> remaining reference to the predicted type, and needs to be flushed.   
> There is a lighter-weight case where an inline cache has a stale  
> pointer; the IC gets reset, instead of the whole nmethod getting  
> unloaded.  In the case of a predicted invokedynamic target, the  
> whole nmethod needs to be flushed.
>
> Does this sound reasonable?  If so, I just need to remove the call  
> in phase 1 of psMarkSweep to non_perm_nmethod_oops_do, and let  
> nmethods with stale oops get flushed.

Yes that sounds right.

tom

>
> Thanks for the review.  I've updated the webrev:
>   http://cr.openjdk.java.net/~jrose/6863023/webrev.02
>
> -- John
>
>> On Jul 22, 2009, at 1:53 AM, John Rose wrote:
>>
>>> In order to inline method handles at invokedynamic instructions,  
>>> it is necessary to manipulate MethodHandle and CallSite constants  
>>> from generated code.  Since these objects are created by ordinary  
>>> user code and subject to usual GC, they are not preallocated in  
>>> the perm gen.
>>>
>>> Currently compiled code requires that any oop embedded as an  
>>> instruction constant or any other nmethod part must be  
>>> OopDesc::is_perm.  For example, internal method objects and  
>>> classes and interned strings are permanent so that they can be  
>>> manipulated from compiled code.
>>>
>>> This restriction is a bug for JSR 292.  Luckily, there is a simple  
>>> fix.
>>>
>>> http://cr.openjdk.java.net/~jrose/6862576/webrev.00/
>
>