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

[Thomas Schatzl]

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

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

Thanks,
  Thomas