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

Fri Oct 6 11:30:58 UTC 2017

Looks good!

Thanks,
Per

> On 6 Oct 2017, at 10:50, Erik Österlund <erik.osterlund at oracle.com> wrote:
> 
> Hi Per,
> 
> Thanks for the review!
> 
> I liked your suggestions and have incorporated them.
> 
> Full webrev:
> http://cr.openjdk.java.net/~eosterlund/8185141/webrev.03/
> 
> Incremental webrev:
> http://cr.openjdk.java.net/~eosterlund/8185141/webrev.02_03/
> 
> Thanks,
> /Erik
> 
>> On 2017-10-06 08:51, Per Liden wrote:
>> 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
>>>> 
>>> 
> 