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

[Per Liden]

Thanks for fixing this Erik.

Change looks good! Just two minor suggestion.

First, how about adding a verify_scavenge_root_nmethod() to CodeCache, 
which would do:

void CodeCahe:verify_scavenge_root_nmethod(nmethod* nm) {
   nm->verify_scavenge_root_oops();
}

so that the call-sites in the collectors look symmetric/related, like this:

  670 void ParallelScavengeHeap::register_nmethod(nmethod* nm) {
  671   CodeCache::register_scavenge_root_nmethod(nm);
  672 }
  673
  674 void ParallelScavengeHeap::verify_nmethod_roots(nmethod* nmethod) {
  675   CodeCache::verify_scavenge_root_nmethod(nm);
  676 }

instead of:

  670 void ParallelScavengeHeap::register_nmethod(nmethod* nm) {
  671   CodeCache::register_scavenge_root_nmethod(nm);
  672 }
  673
  674 void ParallelScavengeHeap::verify_nmethod_roots(nmethod* nmethod) {
  675   nmethod->verify_scavenge_root_oops();
  676 }


Second, for symmetry in CollectedHeap, how about renaming 
verify_nmethod_roots() to verify_nmethod() so that we get:

  572   virtual void register_nmethod(nmethod* nm) {}
  573   virtual void unregister_nmethod(nmethod* nm) {}
  574   virtual void verify_nmethod(nmethod* nmethod) {}

instead of:

  572   virtual void register_nmethod(nmethod* nm) {}
  573   virtual void unregister_nmethod(nmethod* nm) {}
  574   virtual void verify_nmethod_roots(nmethod* nmethod) {}

cheers,
Per

On 2017-10-04 17:17, 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/
>
> 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
>>
>