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

Bengt Rutisson bengt.rutisson at oracle.com
Mon Jan 27 13:09:39 UTC 2014

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::dealwithreference() 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

More information about the hotspot-gc-dev mailing list