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

Tue Sep 1 12:19:33 UTC 2015

Hi,

On 2015-09-01 14:05, Per Liden wrote:
> 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.
Nice catch, I also change follow_array() to take objArrayOop instead of 
oop to avoid to be more inline with follow_array_chunk.

Thanks for the review!

Stefan
>
> 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
>>>>>>
>>>>
>>>
>>