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

[John Cuthbertson]

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.

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

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

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

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

JohnC