RFR (S): 8185141: Generalize scavengeable nmethod root handling

[Erik Österlund]

Hi Thomas,

Thank you for having a look at this change.

On fre, 2017-10-13 at 11:10 +0200, Thomas Schatzl wrote:
> Hi,
> 
> On Wed, 2017-10-04 at 17:17 +0200, Erik Österlund wrote:
> > 
> > Hi,
> > 
> > Sorry for the delayed reply. I got the last review in this thread
> > when I 
> > was on vacation and then unfortunately forgot about this.
> > 
> > So first off, thank you for the review.
> > 
> > New full webrev:
> > http://cr.openjdk.java.net/~eosterlund/8185141/webrev.02/
> I looked at the later 03 change for this re-review....
> 
> > 
> > 
> > Incremental webrev:
> > http://cr.openjdk.java.net/~eosterlund/8185141/webrev.01_02/
> > 
> > Testing: mach5 hs-tier3
> > 
> > [.. lots of fixes...]
> > 
> Thanks.
> 
> > 
> > > 
> > > - probably the _scavenge_root_nmethods and related methods should
> > > be moved out of CodeCache - all this clearly seems to be GC code,
> > > not compiler one. Could be done as another change to not mix
> > > moving
> > > and generalizing the interface.
> > Perhaps. Although the scavangeable root nmethod list is embedded
> > into the nmethods themselves.
> > So it is not a 100% clear cut GC-only code.
> To me the main point of the scavangeable root nmethod list is one of
> optimization: instead of needing to look through all nmethods for
> roots, only the "interesting" nmethods need to be visited during GC.
> 
> Depending on internals of the particular GC, a simple list is not
> optimal any more.
> 
> It just happened that for all "generational framework" collectors a
> list has been sufficient: for other collectors (like G1 or probably
> Shenandoah) a simple linked list is not sufficient any more.
> Probably due to history this list ended up being located here.
> 
> Since the optimal data structure seems GC dependent, it seems
> straightforward to let all GCs be "owner" of that (optimization) data
> structure.
> 
> However, as I mentioned, I would be okay deferring any change here
> (if
> there is).

I agree with you. It would be nice to eventually build a better shared
data structure for this.

> 
> > 
> > > 
> > > Instead of removing the "if (!UseG1GC) { return; }" statements, I
> > > would really prefer to add asserts that these methods are not
> > > called by G1.
> > > Like the one the change removes in
> > > CodeCache::unlink_scavenge_root_nmethod().
> > > 
> [...]
> > 
> > > 
> > > If the method is still called, this just means that it should be
> > > added to the CollectedHeap nmethods-interface - the CR seems to
> > > be
> > > about generalizing the interface, and this seems to be within
> > > scope.
> > > 
> > > Otherwise the interface seems to be incomplete, and apparently
> > > not
> > > general enough - which is what the subject of the CR suggests.
> > I see what you are saying. However, I would really like to not
> > have 
> > asserts in the shared code that this shared code is not run by this
> > or that collector. The main goal with doing this is to modularize
> > the
> > GC implementations so that the shared code is not GC-specific.
> > That 
> > includes assertions that we are not using G1 in shared code.
> > 
> > 
> > If we really want to have such assertions, I guess it would be
> > possible [...]
> > 
> > I hope I persuaded you to agree about this.
> While I agree what you are saying here, given a constraint that
> CodeCache::_scavenge_root_nmethods needs to stay where it is, the
> alternative would be the suggestion to move out
> CodeCache::_scavenge_root_nmethods to GenCollectedHeap.
> 
> (Yeah, I know that G1 inherits from GenCollectedHeap right now...)

It would be unfortunate for collectors that do not want to be part of
the GenCollectedHeap framework to be unable to use the normal scavenge
root list I think.

Eventually, the list probably ought to be completely moved out to its
own class, possibly with a new better data structure.

Would you be okay with leaving the actual list in CodeCache for now?
For this change, my only intention was cleaning up the interface so
that GC-specific code is not scattered in the shared code, and a GC may
choose strategy for its scavengeable roots.

Thanks,
/Erik

> 
> Thanks,
>   Thomas
>