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

[Tom Rodriguez]

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

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()),
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.  What's the plan for this?  I think we  
want to pick a default and stick with it.
codecache.cpp:
why isn't the logic for drop_non_perm_nmethod part of nmethod::flush  
instead of being buried in CodeCache::free?
nmethod.hpp:
The enum names shouldn't look like variable declarations.  Remove the  
underscore.
+ enum { _npl_on_list = 0x01, _npl_marked = 0x10 };

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;
+}
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.

tom


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/
>