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

[John Rose]

On Sep 8, 2009, at 3:06 PM, 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).

Good catch; thanks.  That should have been in my introduction of  
is_scavengable.
In those last two cases, I changed x->is_perm() => !x->is_scavengable().

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

Nmethods are never removed one at a time.  Instead, in each  
gc_epilogue, the list is traversed by prune_scavenge_root_nmethods.  I  
suppose this could become expensive if there are many short GCs and  
the list is very long.  (OTOH, that list must be traversed on every GC  
anyway, since it is part of the weak root set.)  If this turns out to  
be a problem, we could do as you suggest, and have the sweeper perform  
the pruning logic incrementally.

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

Yes, thanks; that's an important thing to verify.

Here's a complication to that assertion: Any nmethods (on the scavenge  
list or not!) which are being used by active stack frames function as  
strong roots.  So those nmethods which are in *both* categories need a  
little extra thought.

For this reason, among others, I'm now experimenting with having *all*  
OopClosures perform the nmethod mark check, so that between any pair  
of oops_do_marking_prologue/epilogue brackets, an nmethod will be  
processed at most once.

-- John