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

[John Rose]

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/


-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20090825/082139d1/attachment.html