RFR: 8204540: Automatic oop closure devirtualization

[Stefan Karlsson]

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

Thanks. Here's one anyway:
  http://cr.openjdk.java.net/~stefank/8204540/webrev.03.delta
  http://cr.openjdk.java.net/~stefank/8204540/webrev.03

Inlined:

>
> ------------------------------------------------------------------------------
> src/hotspot/share/memory/iterator.hpp
>
> Devirtualizer and OopIteratorClosureDispatch should be AllStatic.
> Sorry I missed this in the first round.

Done.

>
> ------------------------------------------------------------------------------
>    50 const int KLASS_ID_COUNT = 6;
>
> KLASS_ID_COUNT should be an unsigned type, like uint.

OK.

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

Right. This was unintentional left-overs from some intermediate code. 
Removed.

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

Right.

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

Copy-n-pasted your comment.

>
> [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.
With:
diff --git a/src/hotspot/share/gc/shared/aaaa.cpp 
b/src/hotspot/share/gc/shared/aaaa.cpp
new file mode 100644
--- /dev/null
+++ b/src/hotspot/share/gc/shared/aaaa.cpp
@@ -0,0 +1,2 @@
+#include "precompiled.hpp"
+#include "genOopClosures.hpp"
diff --git a/src/hotspot/share/gc/shared/genOopClosures.hpp 
b/src/hotspot/share/gc/shared/genOopClosures.hpp
--- a/src/hotspot/share/gc/shared/genOopClosures.hpp
+++ b/src/hotspot/share/gc/shared/genOopClosures.hpp
@@ -118,8 +118,8 @@
    template <class T> inline void do_oop_work(T* p);
   public:
    ScanClosure(DefNewGeneration* g, bool gc_barrier);
-  virtual void do_oop(oop* p);
-  virtual void do_oop(narrowOop* p);
+  virtual void do_oop(oop* p) { do_oop_work(p); }
+  virtual void do_oop(narrowOop* p) { do_oop_work(p); }
  };

  // Closure for scanning DefNewGeneration.
diff --git a/src/hotspot/share/gc/shared/genOopClosures.inline.hpp 
b/src/hotspot/share/gc/shared/genOopClosures.inline.hpp
--- a/src/hotspot/share/gc/shared/genOopClosures.inline.hpp
+++ b/src/hotspot/share/gc/shared/genOopClosures.inline.hpp
@@ -108,8 +108,10 @@
    }
  }

+/*
  inline void ScanClosure::do_oop(oop* p)       { 
ScanClosure::do_oop_work(p); }
  inline void ScanClosure::do_oop(narrowOop* p) { 
ScanClosure::do_oop_work(p); }
+*/

  // NOTE! Any changes made here should also be made
  // in ScanClosure::do_oop_work()

and without PCH, I get:

$ bash 
/home/stefank/hg/jdk/jdk/build/slowdebug/hotspot/variant-server/libjvm/objs/aaa.o.cmdline
In file included from 
/home/stefank/hg/jdk/jdk/open/src/hotspot/share/gc/z/aaa.cpp:2:0:
/home/stefank/hg/jdk/jdk/open/src/hotspot/share/gc/shared/genOopClosures.hpp:118:34: 
error: inline function 'void ScanClosure::do_oop_work(T*) [with T = 
oopDesc*]' used but never defined [-Werror]
    template <class T> inline void do_oop_work(T* p);
                                   ^~~~~~~~~~~
/home/stefank/hg/jdk/jdk/open/src/hotspot/share/gc/shared/genOopClosures.hpp:118:34: 
error: inline function 'void ScanClosure::do_oop_work(T*) [with T = 
unsigned int]' used but never defined [-Werror]
cc1plus: all warnings being treated as errors

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