RFR (S): JDK-6991197 G1: specialize deal_with_reference() for narrowOop*

[Bengt Rutisson]

Hi Thomas,

On 1/27/14 12:51 PM, Thomas Schatzl wrote:
> Hi all,
>
>    can I have your opinion and optionally a review for the following
> small change? Like the bug title reads, the change manually specializes
> G1ParScanThreadState::deal_with_reference() for the narrowOop*, allowing
> some additional specialization.
>
> Measurements indicate a small improvement (~1%) on object copy time for
> specjbb, however not significant.
>
> As I have the change on hand (I had to do it anyway for measurements) I
> would like to ask your opinion whether to keep it or not. If so, ask for
> review.
>
> I am actually undecided - personally I tend to like it (as it trims fat)
> and it seems small enough to keep it and does not distract too much, but
> it does not really show up on measurements.

I share your indecisiveness about this. In a way it is clearer, but it 
also gives code duplication. It would be possible to wrap the three 
duplicated lines in a separate (templified) method, but I don't think 
that would be much better. I also can't really come up with a nice way 
to have the current template parameter control if we call 
has_partial_array_mask() or not. That would have been pretty nice, but I 
am not sure how to do that.

If we do go with your proposed patch I think you should also remove the 
templatisation of set_partial_array_mask(). That would make it clearer 
that only oop* can have the partial array mask set. Not sure if it would 
be a good idea to remove the narrowOop* version of 
has_partial_array_mask(). That might just be confusing as it is now used 
"negatively" in asserts.

Sorry for not having a strong opinion on this.

Bengt

>
> Testing:
> jprt, specjbb2005/13, specjvm98/2008, CRM Fuse
>
> Bug entry:
> https://bugs.openjdk.java.net/browse/JDK-6991197
>
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/6991197/webrev/
>
> Thanks,
> Thomas
>