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

[Per Liden]

Looks good.

/Per

On 2015-05-25 10:42, Stefan Johansson wrote:
> Thanks Per for looking at this,
>
> New webrevs:
> http://cr.openjdk.java.net/~sjohanss/8080746/hotspot.02/
> http://cr.openjdk.java.net/~sjohanss/8080746/hotspot.01-02/
>
> On 2015-05-22 15:45, Per Liden wrote:
>> 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()
>>
> Fixed.
>
> Thanks,
> Stefan
>> 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
>>>>
>>>
>