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

[Per Liden]

Hi Stefan,

On 2015-07-07 14:58, Stefan Johansson wrote:
> Thanks Erik for reviewing,
>
> New webrevs:
> Part 2 - full:
> http://cr.openjdk.java.net/~sjohanss/8129417/2-add-oop_iterate_size/hotspot.01/
>
> Part 2 - inc:
> http://cr.openjdk.java.net/~sjohanss/8129417/2-add-oop_iterate_size/hotspot.00-01/
>
>
> Part 3 - full:
> http://cr.openjdk.java.net/~sjohanss/8129417/3-re-structure-mark-sweep/hotspot.02/
>
> Part 3 - inc:
> http://cr.openjdk.java.net/~sjohanss/8129417/3-re-structure-mark-sweep/hotspot.00-02/
>
> Also includes an updated assert, which was missed in previous webrev.
>
> Comments inline.

Just one thing that looks odd to me, but maybe you can clarify. 
Shouldn't MarkSweep::follow_object() call follow_array() instead of 
calling push_objarray()? That way the change made to 
ObjArrayTask::is_valid() wouldn't be needed.

/Per

>
> On 2015-07-06 17:15, Erik Helin wrote:
>> 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?
> I would like to do/investigate this as a separate RFE, I have a list of
> things that should follow this clean up and I plan to file those before
> pushing this change.
>
>>
>> - 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?
> No, but I have not done as many runs and done as much analysis of this
> as I've done on the serial full gc. I've not seen any regression in
> aurora.se performance runs.
>>> 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)         \
> I've followed '\' alignment "rule" for these macros, which seems to be
> two spaces after the longest line. I just realized that I should also
> update OOP_OOP_ITERATE_DECL_RANGE with one extra space to be consistent.
>
>> - 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);                               \
> Nice catch, missed fixing the alignment when making it void.
>
>> - 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.
> As we've discussed offline, this is most likely safe. I've done an
> ad-hoc RBT run with some random testing when I assert on the size being
> the same both before and after the call to oop_oop_iterate.
>
> I would like to do this change as a separate change as well, so it's
> been added to my list of RFEs to file.
>
>>
>> - Can we remove the method MutableSpace::oop_iterate?
> Looks like it, no one seems to be using it. I'll remove it instead of
> updating it to use oop_iterate_size().
>
>>
>>> 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?
> Removed.
>
>> - I agree with Per about moving the check for object arrays into
>>    follow_object.
> Already done, but gives a slight regression. I still think we can live
> with this small regression for now, to avoid people doing wrong and call
> follow_object on objArrays.
>
>>> 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, we should come up with a better and cleaner way to do this. I
> will file a follow up RFE.
>
>> - I agree with Per about adding an #undef in stack.inline.hpp
> Already fixed.
>
> Thanks again Erik,
> Stefan
>
>> 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
>