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

[Stefan Johansson]

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