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

[John Rose]

On Sep 8, 2009, at 4:06 PM, Y.S.Ramakrishna at Sun.COM wrote:

> Sorry about doing the review in bits and pieces ... I underdstand
> now that some of my comments below may have been misplaced,
> given what i see on reading further into the webrev.

It's not a problem; the early feedback is helpful, and I hope in turn  
you don't mind that the target is moving as I discover more bugs.

(Ideally, I'd like to push a change that will pass JPRT with the  
feature turned on to the maximum level... That might not be possible,  
in which case I'd like at least to pass current JRuby regression tests  
with the feature turned on to the "medium" level.)

> ...and after having seen the following hunk:-
>
> --- old/src/share/vm/memory/sharedHeap.cpp	2009-09-08  
> 01:01:08.000000000 -0700
> +++ new/src/share/vm/memory/sharedHeap.cpp	2009-09-08  
> 01:01:08.000000000 -0700
> ...
> @@ -157,10 +166,17 @@
>
>   if (!_process_strong_tasks- 
> >is_task_claimed(SH_PS_CodeCache_oops_do)) {
>      if (so & SO_CodeCache) {
> +       assert(collecting_perm_gen, "scanning all of code cache");
>        CodeCache::oops_do(roots);
> +     } else if (so & (SO_SystemClasses|SO_AllClasses)) {
> +       if (!collecting_perm_gen) {
> +         // if we are collecting from class statics, but we are not  
> going
> +         // to visit all of the CodeCache, collect from the non- 
> perm roots if any:
> +         CodeCache::scavenge_root_nmethods_oops_do(roots);
> +       }
>      }
>     // Verify if the code cache contents are in the perm gen
> -    NOT_PRODUCT(CodeCache::oops_do(&assert_is_perm_closure));
> +     
> NOT_PRODUCT 
> (CodeCache 
> ::asserted_non_scavengable_oops_do(&assert_is_perm_closure));
>   }
>
> I see that the following comment is misplaced:-
>
>>
>>    Would it be worthwhile to make this scavengable list doubly linked
>>    so as to allow constant time deletion?
>
> ... because your intention, in fact, appears to be to treat these
> scavengable list of nmethods as strong roots (even if they do not
> have activations on thread stacks) and to wait for a full GC to
> reclaim or flush them. Could there be situations where these nmethods
> might hold the sole references to an otherwise unreachable oop?
> Might that cause a visible heap bloat until such time as a full gc
> will be called which will reclaim them?

Actually, I think you've pointed out a design bug:  I want to treat  
those guys as weak roots (comparable to perm-gen references in the  
inactive but live nmethods within the code cache).

> Somewhat relatedly, you weakened the following assertion in  
> nmethod.cpp:-
>
> @@ -1350,7 +1372,10 @@
>       return false;
>     }
>   }
> -  assert(unloading_occurred, "Inconsistency in unloading");
> +  // If ScavengeRootsInCode is true, an nmethod might be unloaded
> +  // simply because one of its constant oops has gone dead.
> +  // No actual classes need to be unloaded in order for this to  
> occur.
> +  assert(unloading_occurred || ScavengeRootsInCode, "Inconsistency  
> in unloading");
>   make_unloaded(is_alive, obj);
>   return true;
> }
>
> Was this necessitated because of the above possibility?

No, I think it is simply an out-of-date assert that is incompatible  
with the logic of the new feature.  (Or a cue that my design is  
wrong.)  It says that the only way an nmethod's embedded weak oops can  
fail is if class unloading has just occurred.  But this is not true if  
we allow new sources of non-perm oops into the code.  If a random app- 
generated oop makes its way into the code, and the oops goes dead, the  
code needs to be thrown away.  At least, that's my current thinking.   
A nice-to-have add on would be for the compiler to say "null this  
constant out when it goes dead" but we don't have a way to say that now.

> (By the way,
> please make sure to merge yr changes with my recent push of some
> related changes for 4957990.)

Will do.  I see them in hotspot-gc.

> rest looks good (although as you hinted, a suitable structuring of
> the active nmethod "do_once" infrastructure so as to call the related
> prologue/epilogue in a more structured fashion might be worth thinking
> about). I'll go look at yr JPRT job later today and see if i have
> any ideas about the problems you are encountering.

I'm experimenting with abolishing the "do_nmethods" bit altogether,  
and having closures do the "do_nmethods" thing by default, and with  
test-and-set marking to ensure once-only processing.  I suspect it  
will be easier to fix bugs from that starting point, than from the  
webrev you are looking at.

-- John
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20090908/4ec574c6/attachment.htm>