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

Tue Sep 1 12:05:27 UTC 2015

Hi Stefan,

On 2015-08-31 18:09, Stefan Johansson wrote:
> Hi,
>
> After some offline discussions with Erik and Stefan I've done some small
> changes to avoid unnecessary calls to do_metadata from
> oop_iterate_range(). I also modified the handling of object arrays
> slightly and made sure not push empty arrays to avoid extra unnecessary
> calls into the iteration code.

Looks good. One minor thing, the index argument to follow_array() should 
be removed as it's not used. I don't need to see a new webrev for that 
though.

cheers,
/Per

>
> Webrev of these changes:
> http://cr.openjdk.java.net/~sjohanss/8129417/5-no-range-metadata/hotspot.00/
>
>
> Full webrev:
> http://cr.openjdk.java.net/~sjohanss/8129417/hotspot.03/
>
> Did a sanity RBT run and also re-did some of the benchmarking to make
> sure these changes are on par with previous versions, and no problems
> were found.
>
> Thanks,
> Stefan
>
> On 2015-08-25 13:29, Per Liden wrote:
>> 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
>>>>>
>>>
>>
>