RFR: 8204540: Automatic oop closure devirtualization

[Kim Barrett]

> On Jun 20, 2018, at 6:50 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
> 
> Hi all,
> 
> Please review this patch to get rid of the macro based oop_iterate devirtualization layer, and replace it with a new implementation based on templates that automatically determines when the closure function calls can be devirtualized.
> 
> http://cr.openjdk.java.net/~stefank/8204540/webrev.01/
> https://bugs.openjdk.java.net/browse/JDK-8204540

Generally looks good, just a few minor comments and nits.

------------------------------------------------------------------------------

Any libjvm.so size comparison? We ought to get some benefit from not
generating code we don't use. OTOH, it's possible this change might
result in more inlining than previously.

Any performance comparisons?  My guess would be perhaps a small
improvement, as we might get some inlining we weren't previously
getting, in some (supposedly) not performance critical places.

------------------------------------------------------------------------------
src/hotspot/share/gc/cms/cmsOopClosures.inline.hpp
  60   inline void cls::do_oop(oop* p)       { cls::do_oop_work(p); }  \
  61   inline void cls::do_oop(narrowOop* p) { cls::do_oop_work(p); }

[pre-existing]
I think the "cls::" qualifiers in the body should be unnecessary.

------------------------------------------------------------------------------
  58 #define DO_OOP_WORK_NV_IMPL(cls)                                  \

This is no longer defining "_nv" functions, so the "_NV" in the name
seems odd.

------------------------------------------------------------------------------
src/hotspot/share/gc/cms/concurrentMarkSweepGeneration.cpp
3321                         MetadataVisitingOopIterateClosure(collector->ref_processor()),

Indentation messed up.

------------------------------------------------------------------------------
src/hotspot/share/memory/iterator.hpp
 370 class OopClosureDispatch {

OopIterateClosureDispatch?

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/genOopClosures.inline.hpp

I think it might be better to put the trivial forwarding do_oop
definitions that have been moved to here instead directly into the
class declarations.  I think doing so gives better / earlier error
messages when forgetting to include the associated .inline.hpp file by
callers. 

------------------------------------------------------------------------------
src/hotspot/share/oops/instanceMirrorKlass.hpp
 111  public:

Unnecessary, we're already in public section.

------------------------------------------------------------------------------
src/hotspot/share/memory/iterator.inline.hpp
 231     static const int NUM_KLASSES = 6;

The value 6 is derived from the number of entries in enum KlassId,
which is far away.  How about defining KLASS_ID_COUNT with that enum?
It might be that the enum and that constant need to be somewhere other
than in klass.hpp though, to avoid include circularities.  But see
next comment too.

------------------------------------------------------------------------------
src/hotspot/share/memory/iterator.inline.hpp
 106   const int   _id;

Maybe this should be of type KlassId?  And similarly the accessor.

Similarly the static const ID members in the various klass types.

I'm surprised it can be const, because of the no-arg constructor.

------------------------------------------------------------------------------
src/hotspot/share/oops/typeArrayKlass.inline.hpp
  38   // Performance tweak: We skip iterating over the klass pointer since we
  39   // know that Universe::TypeArrayKlass never moves.

[pre-existing]

The wording of this comment seems like it might be left-over from
permgen, and ought to be re-worded.

------------------------------------------------------------------------------
src/hotspot/share/memory/iterator.inline.hpp
  77 //  - If &OopClosureType::do_oop is resolved to &Base::do_oop, then there are no
  78 //    implementation of do_oop between Base and OopClosureType. However, there

Either "is no implementation of" or "are no implementations of"

------------------------------------------------------------------------------