RFR: 8204540: Automatic oop closure devirtualization

[Stefan Karlsson]

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

Comments below:

On 2018-06-21 03:52, Kim Barrett wrote:
>> 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.

Before patch: 22833008 bytes
After patch:  22790160 bytes

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

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

Removed.

> 
> ------------------------------------------------------------------------------
>    58 #define DO_OOP_WORK_NV_IMPL(cls)                                  \
> 
> This is no longer defining "_nv" functions, so the "_NV" in the name
> seems odd.

OK. I merged DO_OOP_WORK_NV_IMPL and DO_OOP_WORK_IMPL.

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

Fixed.

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

Yes. Fixed.

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

> 
> ------------------------------------------------------------------------------
> src/hotspot/share/oops/instanceMirrorKlass.hpp
>   111  public:
> 
> Unnecessary, we're already in public section.

Removed.

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

I was thinking about that as well.

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


Done.


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

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

OK. Updated the text.

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

Updated.

Thanks,
StefanK

> 
> ------------------------------------------------------------------------------
>