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

[Stefan Karlsson]

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.

--------------------------------------------------------------------------------
+  int oop_oop_iterate_range##nv_suffix(oop obj, OopClosureType* blk, \
+                                     int start, int end);

Could you combine these two lines.

--------------------------------------------------------------------------------
The indentation of the ending backslashes are inconsistent.

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

--------------------------------------------------------------------------------

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

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

--------------------------------------------------------------------------------

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