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

Thu Jul 2 14:52:16 UTC 2015

Thansk for reviewing this Per,

New full webrev:
http://cr.openjdk.java.net/~sjohanss/8129417/hotspot.01

Incremental webrevs:
http://cr.openjdk.java.net/~sjohanss/8129417/1-remove-oop_ms_follow_contents/hotspot.00-01/
http://cr.openjdk.java.net/~sjohanss/8129417/2-add-oop_iterate_size/hotspot.00-01/
http://cr.openjdk.java.net/~sjohanss/8129417/3-re-structure-mark-sweep/hotspot.00-01/
http://cr.openjdk.java.net/~sjohanss/8129417/4-compiler-hints-for-inlining/hotspot.00-01/

See comments inline.

On 2015-06-30 17:42, Per Liden wrote:
> 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?
I think this is cleanup that we also want to do, especially the ms_*. 
Haven't looked as much at the pc_* parts but I guess they cloud be 
handled in a similar way.

>
> 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.
>
Moved and also did some re-arranging in iterator.hpp.

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

> - 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");
>
Added back.

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

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

>
>>
>>
>> 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);
>
Made all but follow_stack private (it's used by PSMarkSweep).

>
> - In MarkSweep::follow_stack/follow_root, could we move the 
> is_objArray() check into follow_object() instead and void the 
> duplication?
>
Did this, but it gives a small regression on linux_x64. Not entirely 
sure what to do with this, will do yet another re-run with all changes.

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

Thanks,
Stefan

> 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