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

Fri Oct 6 11:33:41 UTC 2017

Thanks for the review.

/Erik

On 2017-10-06 13:30, Per Lidén wrote:
> 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
>>>>>