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

Mon Aug 31 16:09:23 UTC 2015

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.

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