RFR (S): 8133470: Uses of Atomic methods in plab.hpp should be moved to .inline.hpp file

Erik Helin erik.helin at oracle.com
Fri Aug 14 09:34:12 UTC 2015


On 2015-08-13, Thomas Schatzl wrote:
> Hi all,
> 
> On Thu, 2015-08-13 at 12:49 +0200, Thomas Schatzl wrote:
> > Hi all,
> > 
> >   in the review for 8073013 one of the review questions was why plab.hpp
> > added atomic.inline.hpp, and that that is not good practice.
> > 
> > This has been a pre-existing bug, i.e. plab.hpp already used the Atomic
> > functions that reside in .inline.hpp.
> > 
> > This CR fixes this issue by moving the problematic calls into
> > plab.inline.hpp.
> > 
> > The only problem is that changing includes from plab.hpp to
> > plab.inline.hpp in cpp files and adding plab.hpp in header files opens
> > up dependency issues and other missing includes.
> > 
> > That's why this change is larger than it should.
> > 
> > Webrev:
> > https://bugs.openjdk.java.net/browse/JDK-8133470
> > CR:
> > http://cr.openjdk.java.net/~tschatzl/8133470/webrev/
> > 
> > Testing:
> > jprt
> 
>   Mikael and Erik reminded me that it is not a good idea to move
> PLABAllocator::allocate_aligned() into the .cpp file because CMS uses
> only allocate_aligned(). This may result in performance degradation for
> CMS, which is unwanted.
> 
> This required the introduction of a parNewGeneration.inline.hpp for the
> method calling it. And some other code shuffling.
> 
> Here are new webrevs:
> http://cr.openjdk.java.net/~tschatzl/8133470/webrev.0_to_1
> http://cr.openjdk.java.net/~tschatzl/8133470/webrev.1

Thanks for fixing this! Looks good, Reviewed.

Thanks,
Erik

> Testing:
> jprt
> 
> Thanks,
>   Thomas
> 
> 


More information about the hotspot-gc-dev mailing list