RFR: 8204540: Automatic oop closure devirtualization

[Stefan Karlsson]

On 2018-06-21 18:33, Erik Osterlund wrote:
> Hi Stefan,
>
> Looks amazing. I have wanted this to happen in one way or another for years. Thank you for doing this.

Thanks for the review! And thanks to both you and Kim for teaching me 
some of the techniques used to implement this.

StefanK

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