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

[John Cuthbertson]

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. :)

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

JohnC