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

Tue Aug 25 11:29:42 UTC 2015

Hi Stefan,

On 2015-08-21 14:04, Stefan Johansson wrote:
> Hi Per,
>
> On 2015-08-18 13:11, Per Liden wrote:
>> 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.
>>
> Thanks for looking at this again. I agree that your proposed change 
> would make the code nicer and I think I had it structured that way 
> once during this work but had to revert it back to this to keep 
> performance.
>
> I went ahead and did the changes and some new measurements, and this 
> change leads to a small regression for some platforms. I haven't done 
> any deep analysis but I guess it leads to worse inlining decisions on 
> these platforms.
>
> I prefer to leave the code as is. The other possible solution would be 
> to do further analysis and give even more special inlining hints to 
> the different compilers.

Ok, I'm fine with leaving it as is if the other approach leads to 
regressions.

/Per

>
> Thanks,
> Stefan
>
>> /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
>>>
>