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

[Tom Rodriguez]

There's a bug in these changes that Christian just tripped across.  In  
drop_scavenge_root_nmethod the next == nm case doesn't does call  
clear_on_scavenge_root_list or set the link to NULL.  You should move  
that case into the body of the loop so you don't have to duplicate the  
code.

tom

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".
>
>> Maybe scavengeable or some other more descriptive but less specific  
>> term would be better.  If the GC interface provided a  
>> BoolObjectClosure to identify scavengable oops then we could use  
>> that to easily distinguish which ones need to be scanned.
>
> Since it's a global property, a simple subroutine would do as well  
> as a closure.  This makes sense.
>
> I'll change "non-perm" to "scavengable", and put in a note what they  
> term is trying to indicate.  That will decouple the policy we might  
> want to extend from the old non-perm category.  To do this, I'll add  
> parallel definitions to oopDesc::is_perm and SharedHeap::is_permanent.
>
> Some of the renames are:
>   non_perm_{nmethods,link,state} => scavenge_root_{nmethods,...}
>   {add,drop,prune,...}_non_perm_nmethod =>  
> {add,...}_scavenge_root_nmethod
>   NonPermCodeRefs => ScavengeRootsInCode
>
>> Minor nit: In the ad files could you write all the asserts in the  
>> same way:
>>
>> + assert(oop(d64)->is_oop() && (NonPermCodeRefs || oop(d64)- 
>> >is_perm()),
>
> Yes, will do.
>
>> ciObject.cpp:
>> I don't think I get the distinction between has_encoding and  
>> should_be_constant.  I always thought has_encoding was a stupid  
>> name.  Maybe has_encoding should be can_be_embedded and  
>> should_be_constant -> should_be_embedded.  I guess  
>> should_be_constant is ok but then has_encoding should be  
>> can_be_constant.  The relationship between these two should be  
>> clearer.
>
> I'll change has_encoding to can_be_constant.
>
> I'll also rename ciObject::encoding to constant_encoding, to keep  
> that correspondence clear.  It will also make the term "encoding" a  
> little less overloaded; it also refers to machine register names and  
> maybe other instruction operands.
>
> BTW, I found during testing that some non-perm oops were getting  
> into the code cache even if the config flag was set to zero.  The  
> bug fix is in type.cpp, with a little extra "can vs. should" logic  
> in TypeOopPtr::make_from_constant:
>   http://cr.openjdk.java.net/~jrose/6863023/webrev.02/src/share/vm/opto/type.cpp.udiff.html
>
>> What's the plan for this?  I think we want to pick a default and  
>> stick with it.
>
> You mean the plan for the setting of NonPermCodeRefs?  We'll need to  
> set it to the middle option "1" by default when JSR 292 stuff is  
> turned on by default.  We need some such constants for invokedynamic  
> targets.  We'll want to put more such constants into code, I think,  
> when we improve our constant-finding to look through final fields at  
> run-time.  (The ball's in your court on that one, with the  
> "effectively final" work.)  Whether or not we should have the  
> compiler put all possible constants into the code cache will need  
> some experimentation; for now the conservative thing is to allow  
> them but have the compiler be selective about which to put in.   
> That's the middle option of NonPermCodeRefs.
>
>> codecache.cpp:
>> why isn't the logic for drop_non_perm_nmethod part of  
>> nmethod::flush instead of being buried in CodeCache::free?
>
> Oops; thanks!
>
>> nmethod.hpp:
>> The enum names shouldn't look like variable declarations.  Remove  
>> the underscore.
>> + enum { _npl_on_list = 0x01, _npl_marked = 0x10 };
>
> OK.
>
>> nmethod.cpp:
>> There are quite a few returns here...
>> +bool nmethod::detect_non_perm_oops() {
>> +  DetectNonPerm detect_non_perm;
>> +  if (PrintRelocations)  NOT_PRODUCT(detect_non_perm._print_nm =  
>> this);
>> +  oops_do(&detect_non_perm);
>> +  return detect_non_perm.detected_non_perm();
>> +    return false;
>> +  return true;
>> +}
>
> Yes; an incomplete edit.  Fixed.  (JPRT kicked it out at me also.)
>
>> 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.
>
> 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/
>
>