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

[Bengt Rutisson]

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.

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

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


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