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

Mon Jun 23 09:54:30 UTC 2014

Hi Stefan, 

thanks for the review!

I still need a sponsor for this.
Could you please push this change?  

Thanks,
  Goetz.

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

On 2014-06-05 22:23, Lindenmaier, Goetz wrote:
> 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

Thanks.

Looks good to me.

StefanK

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