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

[Per Liden]

Hi Stefan,

I think this looks good in general, some comments below. Question: any 
plans to address the remaining oopDesc::ms_* and oopDesc::pc_* parts, or 
do you see anything that would stop us from converting those too in the 
future?

On 2015-06-22 16:53, 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/

iterator.hpp:
-------------
- I can't see any reason for MetadataAwareOopClosure::do_klass() to be 
in iterator.inline.hpp, so I'd suggest we move it to iterator.hpp.


instanceMirrorKlass.inline.hpp
------------------------------
- A little typo in comment on line 64, "sowhen ..."

- Looks like we lost this assert when code was moved from markSweep.cpp:

     // If klass is NULL then this a mirror for a primitive type.
     // We don't have to follow them, since they are handled as strong
     // roots in Universe::oops_do.
     assert(java_lang_Class::is_primitive(obj), "Sanity check");


specialized_oop_closures.hpp
----------------------------
- I'd prefer if the forwarding declaration comes after the DefNew part 
rather then in the middle of the ParNew/CMS parts.


>
>
> This is more or less what the RFE is all about, the other changes are
> needed to avoid introducing a regression. To be able to remove the
> mark-sweep specific visitor oop_ms_follow_contents the generic visitor
> oop_oop_iterate for all *Klass classes needs to be able to handle the
> mark-sweep case correctly. This is done by adding more functionality to
> the Devirtualizer and make use of it in the generic visitors.
>
> The MarkAndPushClosure is also added to the set of closures that are
> specialized to avoid using virtual calls during the iteration.
> ---
>
> Part 2 - introducing oop_iterate_size:
> http://cr.openjdk.java.net/~sjohanss/8129417/2-add-oop_iterate_size/hotspot.00/

space.cpp
---------
- In ContiguousSpaceDCTOC::walk_mem_region_with_cl() we should be using 
oop_iterate_size() in the loop as well and get rid of the oop->size() call.


>
>
> As stated above the cleanup to remove oop_ms_follow_content give a
> slight regression. One reason for this regression was the fact the
> oop_ms_follow_content didn't care about calculating the size for the
> object it iterated, but the generic visitors do. When looking through
> the code base it is clear that only a subset of the users of
> oop_iterate() really cares about the size and to solve this regression
> and make it possible to iterate without calculating the size I've added
> oop_iterate_size(). It is implemented using oop_iterate() (now void) but
> also calculates the size.
> ---
>
> Part 3 - restructure mark-sweep:
> http://cr.openjdk.java.net/~sjohanss/8129417/3-re-structure-mark-sweep/hotspot.00/

markSweep.hpp
-------------
- It looks like MarkSweep has a lot of functions that should be private 
now. I didn't check them all, but here's some that I think could be made 
private.

   static void mark_object(oop obj);
   template <class T> static inline void follow_root(T* p);
   static inline void push_objarray(oop obj, size_t index);
   static void follow_stack();
   static void follow_object(oop obj);
   static void follow_array(objArrayOop array, int index);


- In MarkSweep::follow_stack/follow_root, could we move the 
is_objArray() check into follow_object() instead and void the duplication?

>
>
> Both a cleanup and a way to avoid part of the regression on some
> platforms. We want to avoid having too much code in the inline.hpp
> files, but we also want all compilers to be able to inline call in a
> good manor. This part of the change moves a lot of code from
> markSweep.inline.hpp to markSweep.cpp and also try to structure it to
> allow good inlining.
> ---
>
> Part 4 - compiler hints for inlining:
> http://cr.openjdk.java.net/~sjohanss/8129417/4-compiler-hints-for-inlining/hotspot.00/

stack.inline.hpp
----------------
- Please #undef NOINLINE somewhere in the end of that file.

cheers,
/Per

>
>
> To avoid having regressions it seems to be very important that certain
> code-paths are correctly inline. To achieve this we need to hint the
> compiler in some parts of the code.
>
> The change does two things:
> * Avoid inlining the slow-path for Stack::push() when using gcc
> * Add always_inline for Solaris in instanceKlass.inline.hpp (same as
> what is already present for Windows compiler)
> ---
>
> 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