RFR: 8204540: Automatic oop closure devirtualization

[Erik Osterlund]

Hi Stefan,

Looks amazing. I have wanted this to happen in one way or another for years. Thank you for doing this.

Thanks,
/Erik

> On 21 Jun 2018, at 11:44, 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
> 
> 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
> 
>> ------------------------------------------------------------------------------