RFR(M): 8044775: Improve usage of umbrella header atomic.inline.hpp.

[Lindenmaier, Goetz]

Hi Stefan, 

thanks for the thorough review!

> Thanks again for cleaning up the include files.
I checked the other includes, I would like to do this for os_<os>.inline.hpp, too.
That include cascade is shorter, but used in around 30 files.
Other more prominent cascades are 
bytes_<cpu>.hpp  (12 cascades), 
ad_<cpu>.hpp (10 cascades), 
nativeInst_<cpu>.hpp   (8)
vmreg.inline_<cpu>.hpp (7)
As we have another 3 cpus (zArch, parisc, ia64), that would clean up our code
nicely ;)

Back to this change:

I moved the Atomics in StringDedup.hpp as you requested.
I removed the include in markSweep.inline.hpp.
I moved the include in oop.pcgc.inline.hpp up. 
I moved the debug methods in gcLocker.hpp to the cpp file.

> This is a lot of noise just to get rid of a call to the the 
> CompiledICHolder constructor in product builds. Is it really worth it?
I think not.  Moved to cpp file.

About adding the header to precompiled.hpp: I don't really care, 
I just follow the current pattern.  As there were no other comments
on your request in the last change, I removed it this time.

Here is the new webrev:
http://cr.openjdk.java.net/~goetz/webrevs/8044775-atomInc/webrev.01

Best regards,
  Goetz.


-----Original Message-----
From: Stefan Karlsson [mailto:stefan.karlsson at oracle.com] 
Sent: Donnerstag, 5. Juni 2014 10:38
To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net
Subject: Re: RFR(M): 8044775: Improve usage of umbrella header atomic.inline.hpp.

Hi Goetz,

Thanks again for cleaning up the include files.

On 2014-06-04 15:59, Lindenmaier, Goetz wrote:
> Hi,
>
> I would like to do another change cleaning up the includes of
> .inline.hpp files from the os_cpu directories.
>
> Please review and test this change.  I please need a sponsor.
> http://cr.openjdk.java.net/~goetz/webrevs/8044775-atomInc/webrev.00/

http://cr.openjdk.java.net/~goetz/webrevs/8044775-atomInc/webrev.00/src/share/vm/gc_implementation/g1/g1StringDedup.hpp.udiff.html
http://cr.openjdk.java.net/~goetz/webrevs/8044775-atomInc/webrev.00/src/share/vm/gc_implementation/g1/g1StringDedup.cpp.udiff.html
http://cr.openjdk.java.net/~goetz/webrevs/8044775-atomInc/webrev.00/src/share/vm/gc_implementation/g1/g1StringDedupQueue.cpp.udiff.html
http://cr.openjdk.java.net/~goetz/webrevs/8044775-atomInc/webrev.00/src/share/vm/gc_implementation/g1/g1StringDedupThread.cpp.udiff.html

I think it would be good to move the Atomic calls out from the 
g1StringDedup.hpp to g1StringDedup.cpp. I talked to Per, the author of 
StringDedup, and he would be fine with that move.
I moved the debug methods in gcLocker.hpp to the cpp file.


http://cr.openjdk.java.net/~goetz/webrevs/8044775-atomInc/webrev.00/src/share/vm/gc_implementation/shared/markSweep.inline.hpp.frames.html

   32 #if INCLUDE_ALL_GCS
   33 #include "gc_implementation/g1/g1StringDedup.hpp"
   34 #include "gc_implementation/parallelScavenge/psParallelCompact.hpp"
   35 #include "runtime/atomic.inline.hpp"
   36 #endif // INCLUDE_ALL_GCS

Why was this include added? Was it becuase g1StringDedup.hpp was added?


http://cr.openjdk.java.net/~goetz/webrevs/8044775-atomInc/webrev.00/src/share/vm/gc_implementation/shared/mutableSpace.cpp.frames.html
http://cr.openjdk.java.net/~goetz/webrevs/8044775-atomInc/webrev.00/src/share/vm/oops/oop.pcgc.inline.hpp.frames.html

   25 #include "precompiled.hpp"
   26 #include "utilities/macros.hpp"
   27 #if INCLUDE_ALL_GCS
   28 #include "gc_implementation/shared/mutableSpace.hpp"
   29 #include "gc_implementation/shared/spaceDecorator.hpp"
   30 #include "oops/oop.inline.hpp"
   31 #include "runtime/safepoint.hpp"
   32 #include "runtime/thread.hpp"
   33 #endif // INCLUDE_ALL_GCS
   34 #include "runtime/atomic.inline.hpp"

Could you place the new includes together with the includes before the 
INCLUDE_ALL_GCS block?


http://cr.openjdk.java.net/~goetz/webrevs/8044775-atomInc/webrev.00/src/share/vm/memory/gcLocker.hpp.udiff.html
http://cr.openjdk.java.net/~goetz/webrevs/8044775-atomInc/webrev.00/src/share/vm/memory/gcLocker.inline.hpp.udiff.html
http://cr.openjdk.java.net/~goetz/webrevs/8044775-atomInc/webrev.00/src/share/vm/runtime/safepoint.cpp.udiff.html

+  inline static void increment_debug_jni_lock_count();
+  inline static void decrement_debug_jni_lock_count();

This is debugging code, so it would be better to move it to the .cpp file.


http://cr.openjdk.java.net/~goetz/webrevs/8044775-atomInc/webrev.00/src/share/vm/oops/compiledICHolder.hpp.udiff.html

   54 #ifdef ASSERT
   55   CompiledICHolder(Method* method, Klass* klass);
   56 #else
   57   CompiledICHolder(Method* method, Klass* klass)
   58     : _holder_method(method), _holder_klass(klass) {};
   59 #endif
   60
   61   ~CompiledICHolder() NOT_DEBUG_RETURN;

This is a lot of noise just to get rid of a call to the the 
CompiledICHolder constructor in product builds. Is it really worth it?


http://cr.openjdk.java.net/~goetz/webrevs/8044775-atomInc/webrev.00/src/share/vm/precompiled/precompiled.hpp.udiff.html

  # include "runtime/atomic.hpp"
+# include "runtime/atomic.inline.hpp"

I still think this is a bad idea. It will just make it more likely that 
someone building with precompiled headers will forget to include 
atomic.inline.hpp, and we would have to clean it up later when we found 
out that it breaks some platform outside of Oracle's test matrix.

thanks,
StefanK

>
> This change improves the usage of the umbrella header atomic.inline.hpp.
> It removes includes of this header in files where it's not needed,
> and adds it in all .cpp and .inline.hpp files where a method of
> Atomic declared 'inline' is used.
>
> Also, the change moves some calls to such methods from .hpp files to
> .inline.hpp files.  In case of ASSERT code it moves the calls
> to .cpp files.
>
> A row of headers still contain calls to inline methods of Atomic,
> which I don't want to move as no appropriate .inline.hpp file is existing:
>
> src/share/vm/compiler/compileBroker.hpp
> src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepThread.hpp
> src/share/vm/gc_implementation/g1/g1StringDedup.hpp
> src/share/vm/gc_implementation/g1/heapRegionRemSet.hpp
> src/share/vm/gc_implementation/parallelScavenge/psParallelCompact.hpp
> src/share/vm/gc_implementation/shared/parGCAllocBuffer.hpp
> src/share/vm/memory/specialized_oop_closures.hpp
> src/share/vm/oops/methodData.hpp
> src/share/vm/runtime/safepoint.hpp
> src/share/vm/services/lowMemoryDetector.hpp
> src/share/vm/services/memTracker.hpp
> src/share/vm/utilities/taskqueue.hpp
>
> I compiled and tested this without precompiled headers on
> linuxx86_64, linuxppc64, windowsx86_64, solaris_sparc64, solaris_sparc32, darwinx86_64, aixppc64
> in opt, dbg and fastdbg versions.
>
> Best regards,
>    Goetz.