[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-dev
Subject: RE: RFR(M): 8044775: Improve usage of umbrella header atomic.inline.hpp.
From: "Lindenmaier, Goetz" <goetz.lindenmaier () sap ! com>
Date: 2014-06-23 14:06:38
Message-ID: 4295855A5C1DE049A61835A1887419CC2CED734D () DEWDFEMB12A ! global ! corp ! sap
[Download RAW message or body]
Thanks, that was fast!
Best regards,
Goetz
-----Original Message-----
From: Stefan Karlsson [mailto:stefan.karlsson@oracle.com]
Sent: Montag, 23. Juni 2014 15:27
To: Lindenmaier, Goetz; hotspot-dev@openjdk.java.net
Subject: Re: RFR(M): 8044775: Improve usage of umbrella header atomic.inline.hpp.
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@oracle.com]
> Sent: Dienstag, 10. Juni 2014 10:21
> To: Lindenmaier, Goetz; hotspot-dev@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@oracle.com]
> > Sent: Donnerstag, 5. Juni 2014 10:38
> > To: Lindenmaier, Goetz; hotspot-dev@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.
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic