RFR: 8129417: Oop iteration clean-up to remove oop_ms_follow_contents

[Erik Helin]

Hi Stefan,

thanks a lot for taking on this work. Please see comments inline.

On 2015-06-22, Stefan Johansson wrote:
> Hi,
> 
> Please review these changes for RFE:
> https://bugs.openjdk.java.net/browse/JDK-8129417
> 
> Webrev for the full change:
> http://cr.openjdk.java.net/~sjohanss/8129417/hotspot.00/
> 
> Summary:
> To allow further cleanups and later remove G1 specific code from mark-sweep,
> we want to remove the mark-sweep specific visitor oop_ms_follow_contents.
> Doing this cleanup has proven to be a little more complicated than I first
> anticipated and to make the change easier to review I've split it into four
> different parts.
> 
> Part 1 - removing oop_ms_follow_contents:
> http://cr.openjdk.java.net/~sjohanss/8129417/1-remove-oop_ms_follow_contents/hotspot.00/

- Looking in specialized_oop_closures.hpp, the comment
   71 // This is split into several because of a Visual C++ 6.0 compiler bug
   72 // where very long macros cause the compiler to crash
  seems a bit dated, given that Visual C++ 6.0 was released in 1998. I
  think we should try to merge ALL_OOP_OOP_ITERATE_CLOSURES_1 and 2 (and
  their corresponding macros). Do you think like doing that in this
  patch or do you want to file a follow-up bug?

- The part of the patch that changes instanceMirrorKlass.inline.hpp
  might impact more GCs than just any of the marksweep ones. Have you
  seen any performance improvements/regressions with G1 or CMS?

> Part 2 - introducing oop_iterate_size:
> http://cr.openjdk.java.net/~sjohanss/8129417/2-add-oop_iterate_size/hotspot.00/

- In arrayKlass.hpp, why add a space to:
  +#define OOP_OOP_ITERATE_DECL_NO_BACKWARDS(OopClosureType, nv_suffix)         \

- In arrayKlass.hpp, the following lines seems aligned strangely:
  +void KlassType::oop_oop_iterate_range##nv_suffix(oop obj, OopClosureType* closure, int start, int end) {  \
  +  oop_oop_iterate_range<nvs_to_bool(nv_suffix)>(obj, closure, start, end);                               \

- In oop.inline.hpp, why must the size be computed before applying the
  closure? In the world of perm gen, this might have been important in
  order to not follow stale klass pointers, but with Metaspace the Klass*
  won't move.

- Can we remove the method MutableSpace::oop_iterate?

> Part 3 - restructure mark-sweep:
> http://cr.openjdk.java.net/~sjohanss/8129417/3-re-structure-mark-sweep/hotspot.00/

- Can you please remove the commented assert in mark_and_push:
   +//  assert(Universe::heap()->is_in_reserved(p), "should be in object space");
  or should it be enabled again?

- I agree with Per about moving the check for object arrays into
  follow_object.

> Part 4 - compiler hints for inlining:
> http://cr.openjdk.java.net/~sjohanss/8129417/4-compiler-hints-for-inlining/hotspot.00/

- I would like to see the definiton of FORCE_INLINE in the compiler
  specific globalDefinitions file. Then the code in
  instanceKlass.inline.hpp can look like:
   #if defined(TARGET_COMPILER_visCPP) || defined(TARGET_COMPILER_sparcWorks)
     #define INLINE FORCE_INLINE
   #else
     #define INLINE inline
   #endif

- The same comment for stack.inline.hpp, but with NO_INLINE:
  #if defined(TARGET_COMPILER_sparcWorks)
    #define STACK_NO_INLINE NO_INLINE
  #else
    #define STACK_NO_INLINE
  #endif

- I agree with Per about adding an #undef in stack.inline.hpp

Thanks!
Erik

> Testing:
> * Adhoc RBT run for functionality - no new failures.
> * Adhoc aurora.se run for performance - no obvious regression.
> * Many local runs tuned to do Full GCs a lot to verify that there is no
> obvious regression.
> 
> Thanks,
> Stefan