RFR: 8204540: Automatic oop closure devirtualization

[Kim Barrett]

> On Jun 21, 2018, at 5:44 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
> 
> Hi Kim,
> 
> Thanks for reviewing this!
> 
> Updated webrevs:
> http://cr.openjdk.java.net/~stefank/8204540/webrev.02.delta
> http://cr.openjdk.java.net/~stefank/8204540/webrev.02

Looks good, other than a few tiny nits.  I don't need another webrev.

------------------------------------------------------------------------------
src/hotspot/share/memory/iterator.hpp

Devirtualizer and OopIteratorClosureDispatch should be AllStatic.
Sorry I missed this in the first round.

------------------------------------------------------------------------------
  50 const int KLASS_ID_COUNT = 6;

KLASS_ID_COUNT should be an unsigned type, like uint.

------------------------------------------------------------------------------
rc/hotspot/share/oops/klass.hpp
  47   ObjArrayKlassID,

Trailing comma for last enumerator in KlassId is a C99/C++11 feature
and not valid in C++98, though some compilers may allow it in some
modes.

------------------------------------------------------------------------------
src/hotspot/share/oops/typeArrayKlass.inline.hpp

I think the key point here is that these klasses are guaranteed to be
processed via the null class loader.  That klasses don't move is kind
of obvious, since they are not Java objects themselves.  How about
something like:

Performance tweak: We skip processing the klass pointer since all
TypeArrayKlasses are guaranteed processed via the null class loader.

[And now I wonder if this remains true with Value Types.]

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

A couple more inline comments below:

>> 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.
> 
> I did runs on our internal perf system, and the scores were the same. I looked at pause times for some of the runs and couldn't find a difference. I'll run more runs over the weekend.

I was hoping that merging the UseCompressedOops dispatch into the
other dispatches might provide some measurable benefit.  Oh well, the
code improvement is well worth the change, even if it’s performance
neutral.

> -----------------------------------------------------------------------------
>> 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.
> 
> I tried your proposal. It has the unfortunate effect that whenever you include genOopClosures.hpp you get a compile error, even when the functions are not used.
> 
> I think we can get what you are looking for by changing 'virtual void do_oop(oop* p)' to 'inline virtual void do_oop(oop* p)'. I'm not sure this should be done for this RFE, though?

I’m fine with deferring.  We can discuss offline.  I’m curious about the compile error.

>> I'm surprised it can be const, because of the no-arg constructor.
> 
> A dummy value is set in the non-arg Klass constructor. Klasses generated with this constructor is only used in CDS to copy the vtables.

Thanks for the explanation.