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

Fri Jul 3 12:48:07 UTC 2015

Hi all,

Please disregard:
http://cr.openjdk.java.net/~sjohanss/8129417/2-add-oop_iterate_size/hotspot.00-01/ 

This fix was incorrect and will not be part of the change.

Stefan

On 2015-07-02 16:52, Stefan Johansson wrote:
> 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
>