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

Mon Jun 23 13:27:02 UTC 2014

On 2014-06-23 11:54, Lindenmaier, Goetz wrote:
> Hi Stefan,
>
> thanks for the review!
>
> I still need a sponsor for this.
> Could you please push this change?

Sure. Your change has been pushed to hs-gc:
http://hg.openjdk.java.net/jdk9/hs-gc/hotspot/rev/b596a1063e90

Cheers,
StefanK

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