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

Fri Oct 6 08:50:45 UTC 2017

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