RFR: 8080746: Refactor oop iteration macros to be more general

[Per Liden]

On 2015-05-20 14:57, Stefan Johansson wrote:
> Thanks for looking at this Stefan,
>
> New webrevs:
> Full: http://cr.openjdk.java.net/~sjohanss/8080746/hotspot.01/
> Inc: http://cr.openjdk.java.net/~sjohanss/8080746/hotspot.00-01/


Looks good, just one minor thing. We now get:

   oop_oop_iterate_range##nv_suffix()
   oop_oop_iterate##nv_suffix##_m()
   oop_oop_iterate_range##nv_suffix()
   oop_oop_iterate_backwards##nv_suffix()

Could align the _m version into this:

   oop_oop_iterate_range##nv_suffix()
   oop_oop_iterate_bounded##nv_suffix()
   oop_oop_iterate_range##nv_suffix()
   oop_oop_iterate_backwards##nv_suffix()

cheers,
/Per

>
> On 2015-05-20 11:44, Stefan Karlsson wrote:
>> Hi Stefan,
>>
>> On 2015-05-20 10:58, Stefan Johansson wrote:
>>> Hi,
>>>
>>> Please review this change to generalize the oop iteration macros:
>>> https://bugs.openjdk.java.net/browse/JDK-8080746
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~sjohanss/8080746/hotspot.00/
>>
>> This looks great!
>>
>> Here's a couple of cleanup/style comments:
>>
>> ========================================================================
>> http://cr.openjdk.java.net/~sjohanss/8080746/hotspot.00/src/share/vm/oops/klass.hpp.udiff.html
>>
>> http://cr.openjdk.java.net/~sjohanss/8080746/hotspot.00/src/share/vm/oops/arrayKlass.hpp.udiff.html
>>
>>
>> --------------------------------------------------------------------------------
>>
>> Could you visually separate the DEFN and DECL defines so that it's
>> more obvious that they serve different purposes. It might be worth
>> adding a comment describing how the DEFN definitions are used.
>>
> Fixed, added an extra new line and extended the comments.
>> --------------------------------------------------------------------------------
>>
>> +  int oop_oop_iterate_range##nv_suffix(oop obj, OopClosureType* blk, \
>> +                                     int start, int end);
>>
>> Could you combine these two lines.
>>
> Fixed.
>> --------------------------------------------------------------------------------
>>
>> The indentation of the ending backslashes are inconsistent.
>>
> Fixed.
>> --------------------------------------------------------------------------------
>>
>> Pre-existing naming issue:
>>
>> +  int oop_oop_iterate_backwards##nv_suffix(oop obj, OopClosureType*
>> blk);
>>
>> +int KlassType::oop_oop_iterate_backwards##nv_suffix(oop obj,
>> OopClosureType* closure) {      \
>>
>> Could you change parameter name blk to closure?
>>
> Fixed.
>> --------------------------------------------------------------------------------
>>
>>
>> ========================================================================
>> http://cr.openjdk.java.net/~sjohanss/8080746/hotspot.00/src/share/vm/oops/objArrayKlass.inline.hpp.frames.html
>>
>>
>> --------------------------------------------------------------------------------
>>
>>  155   OOP_OOP_ITERATE_DEFN(             ObjArrayKlass,
>> OopClosureType, nv_suffix) \
>>  156   OOP_OOP_ITERATE_DEFN_m(           ObjArrayKlass,
>> OopClosureType, nv_suffix) \
>>  157   OOP_OOP_ITERATE_RANGE_DEFN(       ObjArrayKlass,
>> OopClosureType, nv_suffix) \
>>  158   OOP_OOP_ITERATE_NO_BACKWARDS_DEFN(ObjArrayKlass,
>> OopClosureType, nv_suffix)
>>
>> It would be nice to prefix all these macros with OOP_OOP_ITERATE_DEFN
> Fixed, did the same for OOP_OOP_ITERATE_DECL. Change _m to BOUNDED.
>> --------------------------------------------------------------------------------
>>
>>
>> ========================================================================
>> http://cr.openjdk.java.net/~sjohanss/8080746/hotspot.00/src/share/vm/oops/typeArrayKlass.inline.hpp.frames.html
>>
>>
>> --------------------------------------------------------------------------------
>>
>>   44 template <bool nv, typename OopClosureType>
>>   45 int TypeArrayKlass::oop_oop_iterate(oop obj, OopClosureType*
>> closure) {
>>   46   return oop_oop_iterate_impl(obj, closure);
>>   47 }
>>   48
>>   49 template <bool nv, typename OopClosureType>
>>   50 int TypeArrayKlass::oop_oop_iterate_bounded(oop obj,
>> OopClosureType* closure, MemRegion mr) {
>>   51   return oop_oop_iterate_impl(obj, closure);
>>   52 }
>>
>> I think you should add the inline keyword to these functions.
> Skipped this, does not seem to be needed and leaving it out matches how
> objArrayKlass.inline.hpp is handled.
>>
>> --------------------------------------------------------------------------------
>>
>>
> Thanks for the review,
> StefanJ
>> Thanks,
>> StefanK
>>
>>>
>>> Summary:
>>> The macros for the oop_oop_iterate functions were defined for all
>>> *Klass types even though they were very similar. This change extracts
>>> and generalizes the macros to klass.hpp and arrayKlass.hpp.
>>>
>>> For the arrays the *_OOP_OOP_ITERATE_BACKWARDS_* macros is now called
>>> OOP_OOP_ITERATE_NO_BACKWARDS_* to reflect that for arrays we
>>> currently don't have a reverse implementation.
>>>
>>> Thanks,
>>> Stefan
>>
>