RFR(xxxs): 8245707: Increase Metaspace reserve alignment

[coleen.phillimore at oracle.com]

On 6/2/20 12:28 PM, Thomas Stüfe wrote:
> Hi  Colleen,
>
> On Tue, Jun 2, 2020 at 5:53 PM <coleen.phillimore at oracle.com 
> <mailto:coleen.phillimore at oracle.com>> wrote:
>
>
>     Yes, version 01 looks good.
>
>
> thanks for the review.
>
>       How do we tell if
>     Metaspace::reserve_alignment() is used instead of
>     os::allocation_granularity() accidently? 
>
>
> By tripping off asserts - hopefully - which test for the correct 
> alignment.
>
> This change was motivated by the difference between CDS alignment 
> (MetaspaceShared::reserve_alignment()) and metaspace alignment. I 
> originally wanted to do this as part of the remodel change - as a 
> success check - but Ioi asked be to do this separately.
>
> I checked all current usage of metaspace reserve alignment, all is 
> currently good. This is more of a future trap for new code using 
> metaspace reserve alignment. I know its far from perfect, but still 
> better than being page sized.
>
>     I find the use of
>     os::vm_page_size(), os::vm_allocation_granularity() and
>     Metaspace::reserve_alignment() seem to appear almost arbitrary in
>     places, for example in ReservedSpace::initialize().
>
>
> Well, I do not have the sources in front of me 
> but ReservedSpace::initialize() hopefully does not use metaspace 
> alignment! that would make no sense.
>
> As for os::vm_page_size() vs os::vm_allocation_granularity(), that is 
> an ongoing annoyance. As I wrote before, we may want to clean up code 
> which uses os::vm_allocation_granularity(). It should only be used for 
> memory attach addresses, I do not think it is even necessary for 
> reserve size.

Thank you for the future cleanups!
Coleen
>
> Thanks, Thomas
>
>     Thanks,
>     Coleen
>
>     On 5/28/20 2:03 AM, Thomas Stüfe wrote:
>     > Thanks Ioi.
>     >
>     > On Thu, May 28, 2020 at 7:17 AM Ioi Lam <ioi.lam at oracle.com
>     <mailto:ioi.lam at oracle.com>> wrote:
>     >
>     >> The new version looks fine to me.
>     >>
>     >> Thanks
>     >> - Ioi
>     >>
>     >> On 5/26/20 10:57 PM, Thomas Stüfe wrote:
>     >>
>     >> New version:
>     >>
>     >>
>     >>
>     http://cr.openjdk.java.net/~stuefe/webrevs/8245707-increase-metaspace-reserve-alignment/webrev.01/webrev/
>     >>
>     >> Thanks, Thomas
>     >>
>     >> On Wed, May 27, 2020 at 7:42 AM Thomas Stüfe
>     <thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>>
>     >> wrote:
>     >>
>     >>> Hi Ioi,
>     >>>
>     >>> On Wed, May 27, 2020 at 7:04 AM Ioi Lam <ioi.lam at oracle.com
>     <mailto:ioi.lam at oracle.com>> wrote:
>     >>>
>     >>>> Hi Thomas,
>     >>>>
>     >>>> If (UseLargePages && UseLargePagesInMetaspace) is true, then
>     >>>> _reserve_alignment will be 4*os::large_page_size(). Is this
>     what you
>     >>>> want?
>     >>>>
>     >>>>
>     >>> No :) That is why I limited this to cases where the page size
>     is 4K.
>     >>>
>     >>> However I see that on Windows this would still land us at
>     4*allocation
>     >>> granularity instead, so 256K. I'll change that.
>     >>>
>     >>>
>     >>>> Or, do you want to do the *4 only when large page is not used?
>     >>>>
>     >>>>
>     >>> Only if page size is 4K. The reason is that I want to trip over
>     >>> anyone asserting and calculating alignment (eg. CDS) but not
>     trigger any
>     >>> tests which do test things like memory counters - on some of
>     our platforms
>     >>> we have 64K and I don't want to adjust a bunch of tests.
>     >>>
>     >>>
>     >>>> Why did you choose *4? Would *2 also accomplish what you want?
>     >>>>
>     >>>>
>     >>> *2 would also be acceptable. I'll post an update.
>     >>>
>     >>> Thanks! ..Thomas
>     >>>
>     >>>
>     >>>> Thanks
>     >>>> - Ioi
>     >>>>
>     >>>> On 5/25/20 10:57 PM, Thomas Stüfe wrote:
>     >>>>> Hi all,
>     >>>>>
>     >>>>> may I have reviews for this tiny change:
>     >>>>>
>     >>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8245707
>     >>>>> webrev:
>     >>>>>
>     >>>>
>     http://cr.openjdk.java.net/~stuefe/webrevs/8245707-increase-metaspace-reserve-alignment/webrev.00/webrev/
>     >>>>> One of the motivations behind JDK-8243392 was to decouple CDS
>     >>>> reservation
>     >>>>> alignment from metaspace alignment and prevent misuse of
>     metaspace
>     >>>> reserve
>     >>>>> alignment outside metaspace. This is needed since with the
>     upcoming new
>     >>>>> metaspace (JDK-8221173) its reserve alignment requirement
>     will be much
>     >>>>> higher than CDS alignment.
>     >>>>>
>     >>>>> For the time being, until JDK-8221173 finds its way into jdk
>     mainline,
>     >>>> we
>     >>>>> should increase metaspace reserve alignment in jdk mainline
>     to trigger
>     >>>> any
>     >>>>> remaining illegal uses of metaspace reserve alignments and
>     to prevent
>     >>>> new
>     >>>>> ones from creeping in.
>     >>>>>
>     >>>>> All CI tests at SAP went through successfully, so at the
>     moment we
>     >>>> seem to
>     >>>>> be clean.
>     >>>>>
>     >>>>> Thanks, Thomas
>     >>>>
>