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