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

[Thomas Schatzl]

On Tue, 2017-07-25 at 13:29 +0200, Erik Österlund wrote:
> Hi,
> 
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8185141
> 
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8185141/webrev.00/

(looked at .01):

- I agree with Roman about empty methods being the better default.
- method comments from g1CollectedHeap should be moved to
genCollectedHeap.
- the comment for codeCache.cpp at line 694 talks about "non-perm"
oops. Maybe, since we already removed perm gen a long time ago, fix
that comment ("contain oops into the java heap")

- there are no comments about testing of that change in the RFR.

- parallelScavengeHeap.cpp needs copyright update

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

- undecided on whether instead of adding the assert_at_safepoint* to G1
code it would not be better to call the super class code. The codecache
methods also duplicate it

> 
> There seems to be different ways of handling scavengeable nmethod
> roots in hotspot.
> 
> The primary way of dealing with them is to use the CodeCache
> scavenge 
> root nmethod list that maintains a list of all nmethods with 
> scavengeable nmethods.
> However, G1 does not use this list as it has its own mechanism of 
> keeping track of nmethods with scavengeable roots pointing into the
> heap.
> To handle this, the current CodeCache code is full of special cases
> for G1. In multiple cases we check if (UseG1GC) and then return.
> ,m
> We seemingly need a better way of communicating to the GC what 
> scavengeable nmethod roots there are to be able to get rid of the if 
> (UseG1GC)... code.
> 
> As a solution, I propose to make CollectedHeap::register_nmethod the 
> primary way of registering to the GC that there might be a new
> nmethod to keep track of. It is then up to the specific GC to take
> appropriate action. The default appropriate action of CollectedHeap
> is to add the nmethod to the shared scavenge root nmethod list if it
> is not already on the list and it detected the existence of a
> scavengeable root oop in the nmethod. G1 on the other hand, will use
> its closures to figure out what remembered set it should be added to.
> 
> When using G1, the CodeCache scavenge list will be empty, and so a
> lot of G1-centric code for exiting before we walk the list of
> nmethods on the list can be removed where the list is processed in a
> for loop. Because since the list is empty, it does not matter that G1
> runs this code too - it will just iterate 0 times in the loop since
> it is empty. 
> But that's because the list was empty, not because we are using G1 -
> it just happens to be that the list is always empty when we use G1.
> 

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.

Thanks,
  Thomas