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

Tue Sep 1 13:55:04 UTC 2015

On 2015-09-01, Stefan Johansson wrote:
> 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.

Looks good pending your own and Per's comments.

Thanks,
Erik

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