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

[Erik Österlund]

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/

Incremental webrev:
http://cr.openjdk.java.net/~eosterlund/8185141/webrev.01_02/

Testing: mach5 hs-tier3

I also got some feedback from Per. He thought that if the 
register/unregister methods on CollectedHeap are empty and assume that 
more specific heaps put in precisely what they want to do here, then the 
same should be the case for the verify_nmethod_roots() call, so that 
their expectations are symmetric. Therefore, all three nmethod calls are 
now empty on CollectedHeap.

I also decided to change the is_scavengable member function to take an 
oop rather than a const void* that actually always gets sent an oop and 
is subsequently casted to back to an oop in every single implementation 
of it. Since it is arguably part of this interface, I hope nobody 
objects to that.

On 2017-08-03 15:35, Thomas Schatzl wrote:
> 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.

Fixed, and additionally also done for verify_nmethod.

> - method comments from g1CollectedHeap should be moved to
> genCollectedHeap.

Fixed.

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

Fixed.

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

Fixed. (mach5 hs-tier3)

> - parallelScavengeHeap.cpp needs copyright update

Fixed by somebody else it seems.

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

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

I decided to remove these asserts as they are duplicated asserts. G1 
already checks for this in HeapRegionRemSet::remove_strong_code_root and 
HeapRegionRemSet::add_strong_code_root. GenCollectedHeap and 
ParallelScavengeHeap  call CodeCache::register_scavenge_root_nmethod, 
which performs the same assertion.
It seems like a new GC may or may not depend on that lock being taken to 
be able to perform the relevant bookkeeping to remember the oops 
pointing into the java heap, and I do not want to force that 
requirement. So instead, the CollectedHeap implementations are now 
completely empty, and the lock asserts are performed anyway where 
necessary for each collector.

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

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 
to add a boolean method on CollectedHeap and ask it if it expects the 
scavenge root list code to be used or not and let G1 return false. But I 
really do not think this is necessary or desired. If a hypothetical new 
collector accidentally calls this shared code for the scavenge root 
list, and that actually causes incorrect behaviour, then testing would 
certainly break anyway and the mistake would be relatively easy to 
catch. So I guess I am not as inclined to add an interface for 
supporting such asserts only (checking that you are not running shared 
code you do not want to call from your GC), if the code would break 
either way without the asserts requiring this new interface catching it.

I hope I persuaded you to agree about this.

Thanks,
/Erik

> Thanks,
>    Thomas
>