RFR(M/L): 7145569: G1: optimize nmethods scanning

[Mikael Gerdin]

John,

On 06/20/2013 01:39 AM, John Cuthbertson wrote:
> Hi Mikael,
>
> On 6/19/2013 1:29 PM, Mikael Gerdin wrote:
>>
>>> I'm not sure I get what you mean. Do you mean change the OopClosure
>>> passed into CodeBlobToOopClosure to perform the verification of the root
>>> and then to verify that the region containing the referenced object has
>>> the nmethod in it's code root list? If so then how do you get the
>>> nmethod. You need a specialized do_code_blob routine to record the code
>>> blob before calling the oops_do. G1VerifyCodeRootBlobClosure could
>>> inherit directly from CodeBlobClosure but I decided to reuse (rather
>>> than replicate and specialize) the functionality of
>>> CodeBlobToOopClosure.
>>
>> I just meant that you could just change
>> class G1VerifyCodeRootBlobClosure: public CodeBlobToOopClosure {
>> to
>> class G1VerifyCodeRootBlobClosure: public CodeBlobClosure {
>>
>> and change do_code_blob() accordingly.
>> There would be less code and a reader of the code wouldn't need to
>> worry about figuring out how CodeBlobToOopClosure works.
>>
>> Anyway, if you don't feel like changing this I'm fine with it the way
>> it is currently.
>>
>
> OK. Thanks for the clarification. It's what I thought I don't mind
> making the change.
>
>
>>>
>>>>
>>>> CodeBlobToOopClosure has all sorts of complex code to handle
>>>> "claiming" nmethods to only scan them once but you bypass that by
>>>> setting do_marking = false.
>>>> The claiming is done in MarkingCodeBlobClosure::do_code_blob and it
>>>> then calls back up to do_newly_marked_nmethod.
>>>> This complexity makes the code difficult to follow.
>>>
>>> Verification doesn't claim nmethods. That's why we pass false in for the
>>> "marking" parameter. I went with the values in the existing code. I've
>>> seen the StrongRootsScope destructor take quite a time clearing the
>>> "marks" from nmethods but I would bet it's done for safety so that
>>> claiming an nmethod during verification doesn't interfere with the
>>> actual work of the GC and vice versa.
>>
>> Yes. I was just trying to provide an argument for why I wanted you to
>> do the change I described above.
>>
>>>
>>>>
>>>> It would be nice if you could move some of the nmethod scanning
>>>> functionality to a new file instead of growing g1CollectedHeap.cpp
>>>> even more. Several of the functions you added to G1CollectedHeap
>>>> (together with their closures) could be factored out into a a
>>>> G1NMethodManager class or something. It would be nice to keep
>>>> g1CollectedHeap.cpp from growing to 7000 lines...
>>>
>>> I'm OK with moving the new code into its own file. But there's only 3
>>> additional routines being added (the other two are overrides of routines
>>> in CollectedHeap so have to be there) so I don't think it needs it's own
>>> class.  The copyright,  includes, and class boiler plate would come out
>>> to around 100 lines. That seems excessive for three routines. If you are
>>> that concerned (and I'm not disagreeing with the sentiment) then I would
>>> suggest a larger effort and really split up g1CollectedHeap.cpp into the
>>> following:
>>>
>>> * allocation
>>> * full GC
>>> * incremntal GC
>>> * verification
>>>
>>> but I think it should done after this.
>>
>> Ok. That's fine.
>
> Thank you. I've actually been itching to split this file up. :)

That's good. We definitely don't want it to grow to the size of 
concurrentMarkSweepGeneration.cpp :)

>
>>
>> As I said above, I'm ok with this change as of webrev.3.mikaelg-comments
>
> Now that I understand what you were suggesting. I have no problem making
> the change to the verification code.

Great!
Thanks
/Mikael

>
> JohnC
>