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

[Y.S.Ramakrishna at Sun.COM]

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.
See inline below...

On 09/08/09 15:06, Y.S.Ramakrishna at Sun.COM wrote:
> I've started looking at the entire webrev (the
> second of the two links you provided below).
> 
> I am not done yet, but had two small comments:
> 
> (1) the "DetectScavengeRoot" closure appears to check for the
>     presence of any non-perm oop, whereas your interest is really
>     in something that is in a space subject to a partial collection
>     (i.e. a scavenge in our current collectors). So your current
>     check is too loose and will often identify nmethods to have
>     "scavengable" oops when in fact they do not (their references
>     having moved into the old gen which is only collected during
>     a full collection). I think you need to change the check:-
> 
> 1651   virtual void do_oop(oop* p) {
> 1652     if ((*p) != NULL && !(*p)->is_perm()) {
>                               ^^^^^^^^^^^^^^^^
>                              !(*p)->is_is_young()) or equivalent
> 
> 1653       NOT_PRODUCT(maybe_print(p));
> 1654       _detected_scavenge_root = true;
> 1655     }
> 1656   }

So I came across this hunk:-

> --- old/src/share/vm/gc_interface/collectedHeap.hpp	2009-09-08 01:01:02.000000000 -0700
> +++ new/src/share/vm/gc_interface/collectedHeap.hpp	2009-09-08 01:01:02.000000000 -0700
> @@ -256,6 +256,14 @@
>      return p == NULL || is_in_permanent(p);
>    }
>  
> +  // An object is scavengable if its location may move during a scavenge.
> +  // (A scavenge is a GC which is not a full GC.)
> +  // Currently, this just means it is not perm (and not null).
> +  // This could change if we rethink what's in perm-gen.
> +  bool is_scavengable(const void *p) const {
> +    return !is_in_permanent_or_null(p);
> +  }
> +

where you clearly seem to have considered and postponed the
more refined version of is_scavengable(), and in so doing,
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:-

> 
> (2) Could the removal of nmethods from the scavengable list following
>     flush get potentially expensive as you walk down the list to find the
>     appropriate nmethod (could the list get long, at least with
>     large young gens and with certain kinds of dynamic language 
> applications?).
>     Would it be worthwhile to make this scavengable list doubly linked
>     so as to allow constant time deletion?
>     Depends on how frequently nmethod flushing occurs in the new
>     world of scavengable oops in nmethods, i suppose...
> 
> Next I'll go and check that the refs from the scavengable
> list of nmethods act (in general, unless activations are present),
> as weak roots, not strong roots (otherwise they would constitute a
> temporary "leak" until the references got promoted to the old generation,
> modulo my remark (1) above), but wanted to get these comments to
> you first.

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

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? (By the way,
please make sure to merge yr changes with my recent push of some
related changes for 4957990.)

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.

over and out.
-- ramki