Request for review (S): 7130974 G1: Remove G1ParCopyHelper

[Bengt Rutisson]

Hi Tony,

Inline.

On 2012-03-15 15:51, Tony Printezis wrote:
> Hi Bengt,
>
> Inline.
>
> On 03/15/2012 07:10 AM, Bengt Rutisson wrote:
>>
>> Tony,
>>
>> Thanks for looking at this!
>>
>> On 2012-03-14 20:59, Tony Printezis wrote:
>>> Bengt,
>>>
>>> Hi, the change looks good. The only concern I have is that by moving 
>>> copy_to_survivor_space() into G1ParCopyClosure, it will be compiled 
>>> several times (and it's not a small method), once for every 
>>> instantiation of the template (of which we have quite a few IIRC). 
>>> On the other hand, this way we'll also get more more accurate 
>>> profiles. E.g., we'll get a better idea where 
>>> copy_to_survivor_space() is called from, i.e., when scanning roots? 
>>> RSets? etc. This could be very valuable.. Additionally, I've looked 
>>> at collect/analyze G1 profiles in the past and I've seen cases where 
>>> copy_to_survivor_space() was being inlined in the caller, so maybe 
>>> the code is already replicated. Anyway, I'm thinking aloud. I think 
>>> it's OK but I thought that I should bring this up.
>>
>> Good point. I don't think copy_to_survivor_space() is such a huge 
>> method, so hopefully this will not have any significant impact. And 
>> you are right about the nice side effect of increased debugability.
>>
>> I'll run this through ref workload and see if it detects anything. Or 
>> do you think it is worth doing some more targeted performance testing.
>
> Well, I was more worried about compiled code size, not performance 
> (even though in the end I don't think it will be an issue for the 
> reasons I mentioned below). I doubt we'll see any performance impact 
> (even though I'm willing to be surprised!). Also, even if there is an 
> impact, it will probably be very small and will only affect GC so I 
> doubt refworkload will catch it (famous last words!!!). You'd probably 
> need to compare before/after GC times.

I see your point about compiled size. Thanks for clarifying it. I got 
off in kind of the wrong direction. Anyway, RefWorkLoad didn't find 
anything - just as expected.

>> It is hardly worth engaging the performance team for this small clean 
>> up. If we feel unsure about the impact of the change I think it is 
>> better that we just skip doing it. Personally I am not too worried 
>> about any regression from this. But you can of course never be 
>> sure...;-)
>
> I'm not worried either.

:-)

>
>>> Cosmetic suggestion: maybe you want to split some of the lines that 
>>> got quite long with the introduction of the template parameters?
>>>
>>> 4391 template<bool do_gen_barrier, G1Barrier barrier, bool 
>>> do_mark_object>
>>> 4392 oop G1ParCopyClosure<do_gen_barrier, barrier, 
>>> do_mark_object>::copy_to_survivor_space(oop old) {
>>
>> I agree with you about breaking those lines. However, I am not sure 
>> exactly where to break them. 
>
> I know, I've always find breaking lines like this a royal pain!
>
>> I found two occurrences of these long method declarations. Here is a 
>> suggestion for how to line break them:
>>
>> diff --git a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp 
>> b/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
>> --- a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
>> +++ b/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
>> @@ -4366,7 +4366,8 @@
>>  }
>>
>>  template <bool do_gen_barrier, G1Barrier barrier, bool do_mark_object>
>> -void G1ParCopyClosure<do_gen_barrier, barrier, 
>> do_mark_object>::mark_forwarded_object(oop from_obj, oop to_obj) {
>> +void G1ParCopyClosure<do_gen_barrier, barrier, do_mark_object>
>> +  ::mark_forwarded_object(oop from_obj, oop to_obj) {
>>  #ifdef ASSERT
>>    assert(from_obj->is_forwarded(), "from obj should be forwarded");
>>    assert(from_obj->forwardee() == to_obj, "to obj should be the 
>> forwardee");
>> @@ -4389,7 +4390,8 @@
>>  }
>>
>>  template <bool do_gen_barrier, G1Barrier barrier, bool do_mark_object>
>> -oop G1ParCopyClosure<do_gen_barrier, barrier, 
>> do_mark_object>::copy_to_survivor_space(oop old) {
>> +oop G1ParCopyClosure<do_gen_barrier, barrier, do_mark_object>
>> +  ::copy_to_survivor_space(oop old) {
>>    size_t    word_sz = old->size();
>>    HeapRegion* from_region = _g1->heap_region_containing_raw(old);
>>    // +1 to make the -1 indexes valid...
>
> This looks reasonable to me. Thanks!

Good. I'll go with this then.

Thanks again for reviewing this!
Bengt

>
> Tony
>
>> Thanks again for reviewing this!
>> Bengt
>>
>>
>>>
>>>
>>> Tony
>>>
>>> On 03/13/2012 07:49 AM, Bengt Rutisson wrote:
>>>>
>>>> Hi all,
>>>>
>>>> Could I have a couple of reviews of this small code clean up?
>>>>
>>>> http://cr.openjdk.java.net/~brutisso/7130974/webrev.00/
>>>>
>>>> Using templates it is possible to get rid of the extra inheritance 
>>>> level introduced by G1ParCopyHelper. This also removes a dependency 
>>>> between G1ParCopyHelper and G1ParCopyClosure regarding how the 
>>>> _scanner field is initialized.
>>>>
>>>> Bug report:
>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7130974
>>>>
>>>> Thanks,
>>>> Bengt
>>