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

Tom Rodriguez Thomas.Rodriguez at Sun.COM
Thu Aug 27 09:24:53 PDT 2009

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  


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:
>> 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:
> -- 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.

More information about the hotspot-gc-dev mailing list