RFR: 8129417: Oop iteration clean-up to remove oop_ms_follow_contents
per.liden at oracle.com
Tue Jun 30 15:42:20 UTC 2015
I think this looks good in general, some comments below. Question: any
plans to address the remaining oopDesc::ms_* and oopDesc::pc_* parts, or
do you see anything that would stop us from converting those too in the
On 2015-06-22 16:53, Stefan Johansson wrote:
> Please review these changes for RFE:
> Webrev for the full change:
> 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:
- I can't see any reason for MetadataAwareOopClosure::do_klass() to be
in iterator.inline.hpp, so I'd suggest we move it to iterator.hpp.
- A little typo in comment on line 64, "sowhen ..."
- Looks like we lost this assert when code was moved from markSweep.cpp:
// If klass is NULL then this a mirror for a primitive type.
// We don't have to follow them, since they are handled as strong
// roots in Universe::oops_do.
assert(java_lang_Class::is_primitive(obj), "Sanity check");
- I'd prefer if the forwarding declaration comes after the DefNew part
rather then in the middle of the ParNew/CMS parts.
> This is more or less what the RFE is all about, the other changes are
> needed to avoid introducing a regression. To be able to remove the
> mark-sweep specific visitor oop_ms_follow_contents the generic visitor
> oop_oop_iterate for all *Klass classes needs to be able to handle the
> mark-sweep case correctly. This is done by adding more functionality to
> the Devirtualizer and make use of it in the generic visitors.
> The MarkAndPushClosure is also added to the set of closures that are
> specialized to avoid using virtual calls during the iteration.
> Part 2 - introducing oop_iterate_size:
- In ContiguousSpaceDCTOC::walk_mem_region_with_cl() we should be using
oop_iterate_size() in the loop as well and get rid of the oop->size() call.
> As stated above the cleanup to remove oop_ms_follow_content give a
> slight regression. One reason for this regression was the fact the
> oop_ms_follow_content didn't care about calculating the size for the
> object it iterated, but the generic visitors do. When looking through
> the code base it is clear that only a subset of the users of
> oop_iterate() really cares about the size and to solve this regression
> and make it possible to iterate without calculating the size I've added
> oop_iterate_size(). It is implemented using oop_iterate() (now void) but
> also calculates the size.
> Part 3 - restructure mark-sweep:
- It looks like MarkSweep has a lot of functions that should be private
now. I didn't check them all, but here's some that I think could be made
static void mark_object(oop obj);
template <class T> static inline void follow_root(T* p);
static inline void push_objarray(oop obj, size_t index);
static void follow_stack();
static void follow_object(oop obj);
static void follow_array(objArrayOop array, int index);
- In MarkSweep::follow_stack/follow_root, could we move the
is_objArray() check into follow_object() instead and void the duplication?
> Both a cleanup and a way to avoid part of the regression on some
> platforms. We want to avoid having too much code in the inline.hpp
> files, but we also want all compilers to be able to inline call in a
> good manor. This part of the change moves a lot of code from
> markSweep.inline.hpp to markSweep.cpp and also try to structure it to
> allow good inlining.
> Part 4 - compiler hints for inlining:
- Please #undef NOINLINE somewhere in the end of that file.
> To avoid having regressions it seems to be very important that certain
> code-paths are correctly inline. To achieve this we need to hint the
> compiler in some parts of the code.
> The change does two things:
> * Avoid inlining the slow-path for Stack::push() when using gcc
> * Add always_inline for Solaris in instanceKlass.inline.hpp (same as
> what is already present for Windows compiler)
> * 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.
More information about the hotspot-gc-dev