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

[Stefan Johansson]

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/

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
>