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

Thomas Schatzl thomas.schatzl at oracle.com
Thu Aug 3 13:35:37 UTC 2017

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

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.


More information about the hotspot-gc-dev mailing list