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

[Tony Printezis]

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.

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

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
>