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

Mikael Gerdin mikael.gerdin at oracle.com
Wed Jun 19 20:29:25 UTC 2013


Hi John,

On 06/19/2013 12:21 AM, John Cuthbertson wrote:
> Hi Mikael,
>
> Sorry the late response. I didn't expect my response to Thomas to take
> so long.
>
> Replies inline...
>
> On 6/14/2013 5:27 AM, Mikael Gerdin wrote:
>
>>
>> Just to make sure I understand this change, for the G1 class unloading
>> work I don't want the nmethods to act as strong roots. So I guess I'll
>> need to remove the call to mark_strong_code_roots.
>> In the class unloading case only nmethods on the execution stack
>> should be treated as strong roots and they are taken care of by
>> Threads::possibly_parallel_oops_do (from process_strong_roots).
>
> Skipping the call is reasonable and should work.
>
> Here's the way I've always looked at things. For non initial mark pauses
> (including mixed), the code roots are strong (i.e. we don't unload any
> nmethods during evacuation pauses). This is similar to the young GCs of
> the other collectors. Since we currently don't unload nmethods at the
> end of marking they are strong roots for marking. That will change with
> the G1 class unloading work. During an initial mark pause the nmethods
> associated with activations on the thread stacks are the strong roots.
> An nmethod that points into the collection set of an initial mark will
> have it's cset oops evacuated and root region scanning will mark the
> objects directly reachable from those evacuated oops.
>
>>
>>
>>>
>>> To verify the integrity of these nmethod lists I've extended the heap
>>> verification. When scanning the code cache we check that an nmethod is
>>> attached to heap regions that it points into. When verifying the heap
>>> regions we ensure that the nmethods attached to a region contain at
>>> least one pointer into that region. This additional verification can add
>>> some time so I recently put it under control of a diagnostic flag
>>> (testing was performed with it enabled).
>>
>> Did you write some sort of verification code to make sure we don't
>> miss any nmethods which we used to scan with SO_CodeCache but don't
>> right now? I'm not sure we need that in the code right now I'm just
>> curious.
>
> The existing verification walks the code cache just as it's always done
> with the addition that I check that each nmethod with oops is on the
> code root list of each region it points into. That should ensure we
> don't miss any nmethods. The whole idea is that we do not want to scan
> the entire scavengable list.

Ok.

>
>>
>>>
>>> As part of these changes I moved some code around (specifically the
>>> verification code) in g1CollectedHeap.[ch]pp and heapRegion.[ch]pp. The
>>> purely clean up changes can be found at:
>>> http://cr.openjdk.java.net/~johnc/7145569/webrev.0.code-movement/
>>
>> I looked through this. I can't say that I understand your exact
>> reasons for this change but I trust you on this.
>>
>
> Thanks. I just moved some of the verification code and the keep alive
> closure declaration. It kind of looked silly being in the middle of the
> alloc region code.
>
>>
>>>
>>> The webrev containing just the functionality can be found at:
>>> http://cr.openjdk.java.net/~johnc/7145569/webrev.1.optimize-nmethod-scanning/
>>>
>>
>> g1CollectedHeap.cpp:
>> I'm curious as to why you make G1VerifyCodeRootBlobClosure inherit
>> CodeBlobToOopClosure instead of just replacing
>> +      CodeBlobToOopClosure::do_code_blob(cb);
>> with "nm->oops_do(_oop_cl);"
>
> 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.


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

>
>>
>> heapRegion.cpp:
>> +void HeapRegion::migrate_strong_code_roots() {
>> (...)
>> +  GrowableArray<nmethod*> to_be_retained(10);
>> The backing array for this object is resource-allocated.
>> Are we fine with only having a ResourceMark all the way up in
>> evacuate_collection_set()?
>
> Good point. I'll add the ResourceMark to HeapRegion::
>
>>
>> heapRegion.cpp (nits):
>> It seems strange to add _strong_code_root_list as a new line given
>> that the other entries in the initializer list are not on their own
>> lines.
>>      _rem_set(NULL), _recorded_rs_length(0),
>> _predicted_elapsed_time_ms(0),
>> -    _predicted_bytes_to_copy(0)
>> +    _predicted_bytes_to_copy(0),
>> +    _strong_code_root_list(NULL)
>
> OK. Done.
>
>>
>> It seems to be more common to have the asterisk next to the type in
>> HotSpot, just as you did two lines up.
>> +    nmethod *nm = _strong_code_root_list->at(i);
>
> Done.
>
>> Did you do any performance testing?
>
> I haven't yet. Once it's in a respectable state I'll engage PSR. I'll
> also ask Monica if she thinks SPECjbb2013 would good for validating and
> measuring the performance. Let me know if you still want me to change
> G1VerifyCodeRootBlobClosure. I'll wait for your response before sending
> out another webrev.

As I said above, I'm ok with this change as of webrev.3.mikaelg-comments

/Mikael

>
> JohnC
>




More information about the hotspot-gc-dev mailing list