Make reserved_size for compressed class space and metaspace respect the ergo-initialized CompressedClassSpaceSize flag value

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

Make reserved_size for compressed class space and metaspace respect the ergo-initialized CompressedClassSpaceSize flag value

Man Cao
Hello,

This patch is a follow-up fix for
https://bugs.openjdk.java.net/browse/JDK-8087291

This patch moves the call to set_compressed_class_space_size() after the
flag value for CompressedClassSpaceSize is ergo-initialized, fixing the
issue that the reserved size for compressed class space and metaspace is
excessively large when MaxMetaspaceSize is set to a small value. More
discussion about it is available here:
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2017-November/025200.html

This code patch is attached. Could anyone review and/or sponsor this patch?

Best,
Man

jdk10hs.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Make reserved_size for compressed class space and metaspace respect the ergo-initialized CompressedClassSpaceSize flag value

Man Cao
I realized that the email attachment is probably dropped by the mailing
list, so below is the inlined patch.

--- old/src/hotspot/share/memory/metaspace.cpp 2017-11-29
14:56:59.017118444 -0800
+++ new/src/hotspot/share/memory/metaspace.cpp 2017-11-29
14:56:58.657121375 -0800
@@ -3321,9 +3321,6 @@
   MinMetaspaceExpansion = align_down_bounded(MinMetaspaceExpansion,
_commit_alignment);
   MaxMetaspaceExpansion = align_down_bounded(MaxMetaspaceExpansion,
_commit_alignment);

-  CompressedClassSpaceSize = align_down_bounded(CompressedClassSpaceSize,
_reserve_alignment);
-  set_compressed_class_space_size(CompressedClassSpaceSize);
-
   // Initial virtual space size will be calculated at global_initialize()
   size_t min_metaspace_sz =
       VIRTUALSPACEMULTIPLIER * InitialBootClassLoaderMetaspaceSize;
@@ -3341,6 +3338,8 @@
                   min_metaspace_sz);
   }

+  CompressedClassSpaceSize = align_down_bounded(CompressedClassSpaceSize,
_reserve_alignment);
+  set_compressed_class_space_size(CompressedClassSpaceSize);
 }

 void Metaspace::global_initialize() {

Best,
Man

On Wed, Nov 29, 2017 at 3:21 PM, Man Cao <[hidden email]> wrote:

> Hello,
>
> This patch is a follow-up fix for https://bugs.openjdk.java.
> net/browse/JDK-8087291
>
> This patch moves the call to set_compressed_class_space_size() after the
> flag value for CompressedClassSpaceSize is ergo-initialized, fixing the
> issue that the reserved size for compressed class space and metaspace is
> excessively large when MaxMetaspaceSize is set to a small value. More
> discussion about it is available here:
> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/
> 2017-November/025200.html
>
> This code patch is attached. Could anyone review and/or sponsor this patch?
>
> Best,
> Man
>
Reply | Threaded
Open this post in threaded view
|

Re: Make reserved_size for compressed class space and metaspace respect the ergo-initialized CompressedClassSpaceSize flag value

Man Cao
Hello,

This is a friendly ping. Could anyone review or sponsor this change? It's
just a two-liner change.

-Man

On Thu, Nov 30, 2017 at 2:03 PM, Man Cao <[hidden email]> wrote:

> I realized that the email attachment is probably dropped by the mailing
> list, so below is the inlined patch.
>
> --- old/src/hotspot/share/memory/metaspace.cpp 2017-11-29
> 14:56:59.017118444 -0800
> +++ new/src/hotspot/share/memory/metaspace.cpp 2017-11-29
> 14:56:58.657121375 -0800
> @@ -3321,9 +3321,6 @@
>    MinMetaspaceExpansion = align_down_bounded(MinMetaspaceExpansion,
> _commit_alignment);
>    MaxMetaspaceExpansion = align_down_bounded(MaxMetaspaceExpansion,
> _commit_alignment);
>
> -  CompressedClassSpaceSize = align_down_bounded(CompressedClassSpaceSize,
> _reserve_alignment);
> -  set_compressed_class_space_size(CompressedClassSpaceSize);
> -
>    // Initial virtual space size will be calculated at global_initialize()
>    size_t min_metaspace_sz =
>        VIRTUALSPACEMULTIPLIER * InitialBootClassLoaderMetaspaceSize;
> @@ -3341,6 +3338,8 @@
>                    min_metaspace_sz);
>    }
>
> +  CompressedClassSpaceSize = align_down_bounded(CompressedClassSpaceSize,
> _reserve_alignment);
> +  set_compressed_class_space_size(CompressedClassSpaceSize);
>  }
>
>  void Metaspace::global_initialize() {
>
> Best,
> Man
>
> On Wed, Nov 29, 2017 at 3:21 PM, Man Cao <[hidden email]> wrote:
>
>> Hello,
>>
>> This patch is a follow-up fix for https://bugs.openjdk.java.
>> net/browse/JDK-8087291
>>
>> This patch moves the call to set_compressed_class_space_size() after the
>> flag value for CompressedClassSpaceSize is ergo-initialized, fixing the
>> issue that the reserved size for compressed class space and metaspace is
>> excessively large when MaxMetaspaceSize is set to a small value. More
>> discussion about it is available here:
>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2
>> 017-November/025200.html
>>
>> This code patch is attached. Could anyone review and/or sponsor this
>> patch?
>>
>> Best,
>> Man
>>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: Make reserved_size for compressed class space and metaspace respect the ergo-initialized CompressedClassSpaceSize flag value

Yasumasa Suenaga-4
Hi Man,

CompressedClassSpaceSize might be modified by FLAG_SET_ERGO.
So I think you need to move set_compressed_class_space_size() only.


Thanks,

Yasumasa



2017-12-08 3:42 GMT+09:00 Man Cao <[hidden email]>:

> Hello,
>
> This is a friendly ping. Could anyone review or sponsor this change? It's
> just a two-liner change.
>
> -Man
>
> On Thu, Nov 30, 2017 at 2:03 PM, Man Cao <[hidden email]> wrote:
>>
>> I realized that the email attachment is probably dropped by the mailing
>> list, so below is the inlined patch.
>>
>> --- old/src/hotspot/share/memory/metaspace.cpp 2017-11-29
>> 14:56:59.017118444 -0800
>> +++ new/src/hotspot/share/memory/metaspace.cpp 2017-11-29
>> 14:56:58.657121375 -0800
>> @@ -3321,9 +3321,6 @@
>>    MinMetaspaceExpansion = align_down_bounded(MinMetaspaceExpansion,
>> _commit_alignment);
>>    MaxMetaspaceExpansion = align_down_bounded(MaxMetaspaceExpansion,
>> _commit_alignment);
>>
>> -  CompressedClassSpaceSize = align_down_bounded(CompressedClassSpaceSize,
>> _reserve_alignment);
>> -  set_compressed_class_space_size(CompressedClassSpaceSize);
>> -
>>    // Initial virtual space size will be calculated at global_initialize()
>>    size_t min_metaspace_sz =
>>        VIRTUALSPACEMULTIPLIER * InitialBootClassLoaderMetaspaceSize;
>> @@ -3341,6 +3338,8 @@
>>                    min_metaspace_sz);
>>    }
>>
>> +  CompressedClassSpaceSize = align_down_bounded(CompressedClassSpaceSize,
>> _reserve_alignment);
>> +  set_compressed_class_space_size(CompressedClassSpaceSize);
>>  }
>>
>>  void Metaspace::global_initialize() {
>>
>> Best,
>> Man
>>
>> On Wed, Nov 29, 2017 at 3:21 PM, Man Cao <[hidden email]> wrote:
>>>
>>> Hello,
>>>
>>> This patch is a follow-up fix for
>>> https://bugs.openjdk.java.net/browse/JDK-8087291
>>>
>>> This patch moves the call to set_compressed_class_space_size() after the
>>> flag value for CompressedClassSpaceSize is ergo-initialized, fixing the
>>> issue that the reserved size for compressed class space and metaspace is
>>> excessively large when MaxMetaspaceSize is set to a small value. More
>>> discussion about it is available here:
>>>
>>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2017-November/025200.html
>>>
>>> This code patch is attached. Could anyone review and/or sponsor this
>>> patch?
>>>
>>> Best,
>>> Man
>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: Make reserved_size for compressed class space and metaspace respect the ergo-initialized CompressedClassSpaceSize flag value

Man Cao
Hi Yasumasa,

Thanks for the review! Although I think moving or not moving
"CompressedClassSpaceSize = align_size_down_bounded()" are equivalent
because of logic of alignment and FLAG_SET_ERGO, I made the change you
suggested anyway.
The new patch is attached and inlined below.


--- old/src/hotspot/share/memory/metaspace.cpp 2017-12-08
18:42:48.960285998 -0800
+++ new/src/hotspot/share/memory/metaspace.cpp 2017-12-08
18:42:48.600288972 -0800
@@ -3322,7 +3322,6 @@
   MaxMetaspaceExpansion = align_down_bounded(MaxMetaspaceExpansion,
_commit_alignment);

   CompressedClassSpaceSize = align_down_bounded(CompressedClassSpaceSize,
_reserve_alignment);
-  set_compressed_class_space_size(CompressedClassSpaceSize);

   // Initial virtual space size will be calculated at global_initialize()
   size_t min_metaspace_sz =
@@ -3341,6 +3340,7 @@
                   min_metaspace_sz);
   }

+  set_compressed_class_space_size(CompressedClassSpaceSize);
 }

 void Metaspace::global_initialize() {


Thanks,
Man

On Thu, Dec 7, 2017 at 9:44 PM, Yasumasa Suenaga <[hidden email]> wrote:

> Hi Man,
>
> CompressedClassSpaceSize might be modified by FLAG_SET_ERGO.
> So I think you need to move set_compressed_class_space_size() only.
>
>
> Thanks,
>
> Yasumasa
>
>
>
> 2017-12-08 3:42 GMT+09:00 Man Cao <[hidden email]>:
> > Hello,
> >
> > This is a friendly ping. Could anyone review or sponsor this change? It's
> > just a two-liner change.
> >
> > -Man
> >
> > On Thu, Nov 30, 2017 at 2:03 PM, Man Cao <[hidden email]> wrote:
> >>
> >> I realized that the email attachment is probably dropped by the mailing
> >> list, so below is the inlined patch.
> >>
> >> --- old/src/hotspot/share/memory/metaspace.cpp 2017-11-29
> >> 14:56:59.017118444 -0800
> >> +++ new/src/hotspot/share/memory/metaspace.cpp 2017-11-29
> >> 14:56:58.657121375 -0800
> >> @@ -3321,9 +3321,6 @@
> >>    MinMetaspaceExpansion = align_down_bounded(MinMetaspaceExpansion,
> >> _commit_alignment);
> >>    MaxMetaspaceExpansion = align_down_bounded(MaxMetaspaceExpansion,
> >> _commit_alignment);
> >>
> >> -  CompressedClassSpaceSize = align_down_bounded(
> CompressedClassSpaceSize,
> >> _reserve_alignment);
> >> -  set_compressed_class_space_size(CompressedClassSpaceSize);
> >> -
> >>    // Initial virtual space size will be calculated at
> global_initialize()
> >>    size_t min_metaspace_sz =
> >>        VIRTUALSPACEMULTIPLIER * InitialBootClassLoaderMetaspaceSize;
> >> @@ -3341,6 +3338,8 @@
> >>                    min_metaspace_sz);
> >>    }
> >>
> >> +  CompressedClassSpaceSize = align_down_bounded(
> CompressedClassSpaceSize,
> >> _reserve_alignment);
> >> +  set_compressed_class_space_size(CompressedClassSpaceSize);
> >>  }
> >>
> >>  void Metaspace::global_initialize() {
> >>
> >> Best,
> >> Man
> >>
> >> On Wed, Nov 29, 2017 at 3:21 PM, Man Cao <[hidden email]> wrote:
> >>>
> >>> Hello,
> >>>
> >>> This patch is a follow-up fix for
> >>> https://bugs.openjdk.java.net/browse/JDK-8087291
> >>>
> >>> This patch moves the call to set_compressed_class_space_size() after
> the
> >>> flag value for CompressedClassSpaceSize is ergo-initialized, fixing the
> >>> issue that the reserved size for compressed class space and metaspace
> is
> >>> excessively large when MaxMetaspaceSize is set to a small value. More
> >>> discussion about it is available here:
> >>>
> >>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/
> 2017-November/025200.html
> >>>
> >>> This code patch is attached. Could anyone review and/or sponsor this
> >>> patch?
> >>>
> >>> Best,
> >>> Man
> >>
> >>
> >
>

jdk10hs.patch (980 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Make reserved_size for compressed class space and metaspace respect the ergo-initialized CompressedClassSpaceSize flag value

coleen.phillimore
Hi this looks good and I will sponsor this with you as contributor using
[hidden email] .

Have you signed the contributor agreement or are covered by one?

thanks,
Coleen

On 12/9/17 7:37 AM, Yasumasa Suenaga wrote:

> Hi Man,
>
> Looks good, but you need to get a sponsor.
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2017/12/09 11:57, Man Cao wrote:
>> Hi Yasumasa,
>>
>> Thanks for the review! Although I think moving or not moving
>> "CompressedClassSpaceSize = align_size_down_bounded()" are equivalent
>> because of logic of alignment and FLAG_SET_ERGO, I made the change
>> you suggested anyway.
>> The new patch is attached and inlined below.
>>
>>
>> --- old/src/hotspot/share/memory/metaspace.cpp2017-12-08
>> 18:42:48.960285998 -0800
>> +++ new/src/hotspot/share/memory/metaspace.cpp2017-12-08
>> 18:42:48.600288972 -0800
>> @@ -3322,7 +3322,6 @@
>>     MaxMetaspaceExpansion = align_down_bounded(MaxMetaspaceExpansion,
>> _commit_alignment);
>>     CompressedClassSpaceSize =
>> align_down_bounded(CompressedClassSpaceSize, _reserve_alignment);
>> -  set_compressed_class_space_size(CompressedClassSpaceSize);
>>     // Initial virtual space size will be calculated at
>> global_initialize()
>>     size_t min_metaspace_sz =
>> @@ -3341,6 +3340,7 @@
>>                     min_metaspace_sz);
>>     }
>> +  set_compressed_class_space_size(CompressedClassSpaceSize);
>>   }
>>   void Metaspace::global_initialize() {
>>
>>
>> Thanks,
>> Man
>>
>> On Thu, Dec 7, 2017 at 9:44 PM, Yasumasa Suenaga <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>>     Hi Man,
>>
>>     CompressedClassSpaceSize might be modified by FLAG_SET_ERGO.
>>     So I think you need to move set_compressed_class_space_size() only.
>>
>>
>>     Thanks,
>>
>>     Yasumasa
>>
>>
>>
>>     2017-12-08 3:42 GMT+09:00 Man Cao <[hidden email]
>> <mailto:[hidden email]>>:
>>      > Hello,
>>      >
>>      > This is a friendly ping. Could anyone review or sponsor this
>> change? It's
>>      > just a two-liner change.
>>      >
>>      > -Man
>>      >
>>      > On Thu, Nov 30, 2017 at 2:03 PM, Man Cao <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>      >>
>>      >> I realized that the email attachment is probably dropped by
>> the mailing
>>      >> list, so below is the inlined patch.
>>      >>
>>      >> --- old/src/hotspot/share/memory/metaspace.cpp 2017-11-29
>>      >> 14:56:59.017118444 -0800
>>      >> +++ new/src/hotspot/share/memory/metaspace.cpp 2017-11-29
>>      >> 14:56:58.657121375 -0800
>>      >> @@ -3321,9 +3321,6 @@
>>      >>    MinMetaspaceExpansion =
>> align_down_bounded(MinMetaspaceExpansion,
>>      >> _commit_alignment);
>>      >>    MaxMetaspaceExpansion =
>> align_down_bounded(MaxMetaspaceExpansion,
>>      >> _commit_alignment);
>>      >>
>>      >> -  CompressedClassSpaceSize =
>> align_down_bounded(CompressedClassSpaceSize,
>>      >> _reserve_alignment);
>>      >> - set_compressed_class_space_size(CompressedClassSpaceSize);
>>      >> -
>>      >>    // Initial virtual space size will be calculated at
>> global_initialize()
>>      >>    size_t min_metaspace_sz =
>>      >>        VIRTUALSPACEMULTIPLIER *
>> InitialBootClassLoaderMetaspaceSize;
>>      >> @@ -3341,6 +3338,8 @@
>>      >>                    min_metaspace_sz);
>>      >>    }
>>      >>
>>      >> +  CompressedClassSpaceSize =
>> align_down_bounded(CompressedClassSpaceSize,
>>      >> _reserve_alignment);
>>      >> + set_compressed_class_space_size(CompressedClassSpaceSize);
>>      >>  }
>>      >>
>>      >>  void Metaspace::global_initialize() {
>>      >>
>>      >> Best,
>>      >> Man
>>      >>
>>      >> On Wed, Nov 29, 2017 at 3:21 PM, Man Cao <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>      >>>
>>      >>> Hello,
>>      >>>
>>      >>> This patch is a follow-up fix for
>>      >>> https://bugs.openjdk.java.net/browse/JDK-8087291 
>> <https://bugs.openjdk.java.net/browse/JDK-8087291>
>>      >>>
>>      >>> This patch moves the call to
>> set_compressed_class_space_size() after the
>>      >>> flag value for CompressedClassSpaceSize is ergo-initialized,
>> fixing the
>>      >>> issue that the reserved size for compressed class space and
>> metaspace is
>>      >>> excessively large when MaxMetaspaceSize is set to a small
>> value. More
>>      >>> discussion about it is available here:
>>      >>>
>>      >>>
>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2017-November/025200.html 
>> <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2017-November/025200.html>
>>
>>      >>>
>>      >>> This code patch is attached. Could anyone review and/or
>> sponsor this
>>      >>> patch?
>>      >>>
>>      >>> Best,
>>      >>> Man
>>      >>
>>      >>
>>      >
>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: Make reserved_size for compressed class space and metaspace respect the ergo-initialized CompressedClassSpaceSize flag value

coleen.phillimore
In reply to this post by Man Cao
Oh, yes, and I don't see an open bug number for this issue.  Please open
a bug.
thanks,
Coleen

On 12/9/17 7:37 AM, Yasumasa Suenaga wrote:

> Hi Man,
>
> Looks good, but you need to get a sponsor.
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2017/12/09 11:57, Man Cao wrote:
>> Hi Yasumasa,
>>
>> Thanks for the review! Although I think moving or not moving
>> "CompressedClassSpaceSize = align_size_down_bounded()" are equivalent
>> because of logic of alignment and FLAG_SET_ERGO, I made the change
>> you suggested anyway.
>> The new patch is attached and inlined below.
>>
>>
>> --- old/src/hotspot/share/memory/metaspace.cpp2017-12-08
>> 18:42:48.960285998 -0800
>> +++ new/src/hotspot/share/memory/metaspace.cpp2017-12-08
>> 18:42:48.600288972 -0800
>> @@ -3322,7 +3322,6 @@
>>     MaxMetaspaceExpansion = align_down_bounded(MaxMetaspaceExpansion,
>> _commit_alignment);
>>     CompressedClassSpaceSize =
>> align_down_bounded(CompressedClassSpaceSize, _reserve_alignment);
>> -  set_compressed_class_space_size(CompressedClassSpaceSize);
>>     // Initial virtual space size will be calculated at
>> global_initialize()
>>     size_t min_metaspace_sz =
>> @@ -3341,6 +3340,7 @@
>>                     min_metaspace_sz);
>>     }
>> +  set_compressed_class_space_size(CompressedClassSpaceSize);
>>   }
>>   void Metaspace::global_initialize() {
>>
>>
>> Thanks,
>> Man
>>
>> On Thu, Dec 7, 2017 at 9:44 PM, Yasumasa Suenaga <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>>     Hi Man,
>>
>>     CompressedClassSpaceSize might be modified by FLAG_SET_ERGO.
>>     So I think you need to move set_compressed_class_space_size() only.
>>
>>
>>     Thanks,
>>
>>     Yasumasa
>>
>>
>>
>>     2017-12-08 3:42 GMT+09:00 Man Cao <[hidden email]
>> <mailto:[hidden email]>>:
>>      > Hello,
>>      >
>>      > This is a friendly ping. Could anyone review or sponsor this
>> change? It's
>>      > just a two-liner change.
>>      >
>>      > -Man
>>      >
>>      > On Thu, Nov 30, 2017 at 2:03 PM, Man Cao <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>      >>
>>      >> I realized that the email attachment is probably dropped by
>> the mailing
>>      >> list, so below is the inlined patch.
>>      >>
>>      >> --- old/src/hotspot/share/memory/metaspace.cpp 2017-11-29
>>      >> 14:56:59.017118444 -0800
>>      >> +++ new/src/hotspot/share/memory/metaspace.cpp 2017-11-29
>>      >> 14:56:58.657121375 -0800
>>      >> @@ -3321,9 +3321,6 @@
>>      >>    MinMetaspaceExpansion =
>> align_down_bounded(MinMetaspaceExpansion,
>>      >> _commit_alignment);
>>      >>    MaxMetaspaceExpansion =
>> align_down_bounded(MaxMetaspaceExpansion,
>>      >> _commit_alignment);
>>      >>
>>      >> -  CompressedClassSpaceSize =
>> align_down_bounded(CompressedClassSpaceSize,
>>      >> _reserve_alignment);
>>      >> - set_compressed_class_space_size(CompressedClassSpaceSize);
>>      >> -
>>      >>    // Initial virtual space size will be calculated at
>> global_initialize()
>>      >>    size_t min_metaspace_sz =
>>      >>        VIRTUALSPACEMULTIPLIER *
>> InitialBootClassLoaderMetaspaceSize;
>>      >> @@ -3341,6 +3338,8 @@
>>      >>                    min_metaspace_sz);
>>      >>    }
>>      >>
>>      >> +  CompressedClassSpaceSize =
>> align_down_bounded(CompressedClassSpaceSize,
>>      >> _reserve_alignment);
>>      >> + set_compressed_class_space_size(CompressedClassSpaceSize);
>>      >>  }
>>      >>
>>      >>  void Metaspace::global_initialize() {
>>      >>
>>      >> Best,
>>      >> Man
>>      >>
>>      >> On Wed, Nov 29, 2017 at 3:21 PM, Man Cao <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>      >>>
>>      >>> Hello,
>>      >>>
>>      >>> This patch is a follow-up fix for
>>      >>> https://bugs.openjdk.java.net/browse/JDK-8087291 
>> <https://bugs.openjdk.java.net/browse/JDK-8087291>
>>      >>>
>>      >>> This patch moves the call to
>> set_compressed_class_space_size() after the
>>      >>> flag value for CompressedClassSpaceSize is ergo-initialized,
>> fixing the
>>      >>> issue that the reserved size for compressed class space and
>> metaspace is
>>      >>> excessively large when MaxMetaspaceSize is set to a small
>> value. More
>>      >>> discussion about it is available here:
>>      >>>
>>      >>>
>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2017-November/025200.html 
>> <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2017-November/025200.html>
>>
>>      >>>
>>      >>> This code patch is attached. Could anyone review and/or
>> sponsor this
>>      >>> patch?
>>      >>>
>>      >>> Best,
>>      >>> Man
>>      >>
>>      >>
>>      >
>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: Make reserved_size for compressed class space and metaspace respect the ergo-initialized CompressedClassSpaceSize flag value

Man Cao
In reply to this post by coleen.phillimore
Hi Coleen,

Thanks for sponsoring this patch! Yes, I'm covered by the agreement signed
by Google Inc. [1]
Could this patch be a part of
https://bugs.openjdk.java.net/browse/JDK-8087291, as this is a simple
follow-up fix?
If not, could you or someone else create a bug, as I don't have an account
to create one?

[1] http://www.oracle.com/technetwork/community/oca-486395.html#g

-Man

On Mon, Dec 11, 2017 at 2:27 PM, <[hidden email]> wrote:

> Hi this looks good and I will sponsor this with you as contributor using
> [hidden email] .
>
> Have you signed the contributor agreement or are covered by one?
>
> thanks,
> Coleen
>
> On 12/9/17 7:37 AM, Yasumasa Suenaga wrote:
>
>> Hi Man,
>>
>> Looks good, but you need to get a sponsor.
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> On 2017/12/09 11:57, Man Cao wrote:
>>
>>> Hi Yasumasa,
>>>
>>> Thanks for the review! Although I think moving or not moving
>>> "CompressedClassSpaceSize = align_size_down_bounded()" are equivalent
>>> because of logic of alignment and FLAG_SET_ERGO, I made the change you
>>> suggested anyway.
>>> The new patch is attached and inlined below.
>>>
>>>
>>> --- old/src/hotspot/share/memory/metaspace.cpp2017-12-08
>>> 18:42:48.960285998 -0800
>>> +++ new/src/hotspot/share/memory/metaspace.cpp2017-12-08
>>> 18:42:48.600288972 -0800
>>> @@ -3322,7 +3322,6 @@
>>>     MaxMetaspaceExpansion = align_down_bounded(MaxMetaspaceExpansion,
>>> _commit_alignment);
>>>     CompressedClassSpaceSize = align_down_bounded(CompressedClassSpaceSize,
>>> _reserve_alignment);
>>> -  set_compressed_class_space_size(CompressedClassSpaceSize);
>>>     // Initial virtual space size will be calculated at
>>> global_initialize()
>>>     size_t min_metaspace_sz =
>>> @@ -3341,6 +3340,7 @@
>>>                     min_metaspace_sz);
>>>     }
>>> +  set_compressed_class_space_size(CompressedClassSpaceSize);
>>>   }
>>>   void Metaspace::global_initialize() {
>>>
>>>
>>> Thanks,
>>> Man
>>>
>>> On Thu, Dec 7, 2017 at 9:44 PM, Yasumasa Suenaga <[hidden email]
>>> <mailto:[hidden email]>> wrote:
>>>
>>>     Hi Man,
>>>
>>>     CompressedClassSpaceSize might be modified by FLAG_SET_ERGO.
>>>     So I think you need to move set_compressed_class_space_size() only.
>>>
>>>
>>>     Thanks,
>>>
>>>     Yasumasa
>>>
>>>
>>>
>>>     2017-12-08 3:42 GMT+09:00 Man Cao <[hidden email] <mailto:
>>> [hidden email]>>:
>>>      > Hello,
>>>      >
>>>      > This is a friendly ping. Could anyone review or sponsor this
>>> change? It's
>>>      > just a two-liner change.
>>>      >
>>>      > -Man
>>>      >
>>>      > On Thu, Nov 30, 2017 at 2:03 PM, Man Cao <[hidden email]
>>> <mailto:[hidden email]>> wrote:
>>>      >>
>>>      >> I realized that the email attachment is probably dropped by the
>>> mailing
>>>      >> list, so below is the inlined patch.
>>>      >>
>>>      >> --- old/src/hotspot/share/memory/metaspace.cpp 2017-11-29
>>>      >> 14:56:59.017118444 -0800
>>>      >> +++ new/src/hotspot/share/memory/metaspace.cpp 2017-11-29
>>>      >> 14:56:58.657121375 -0800
>>>      >> @@ -3321,9 +3321,6 @@
>>>      >>    MinMetaspaceExpansion = align_down_bounded(MinMetaspac
>>> eExpansion,
>>>      >> _commit_alignment);
>>>      >>    MaxMetaspaceExpansion = align_down_bounded(MaxMetaspac
>>> eExpansion,
>>>      >> _commit_alignment);
>>>      >>
>>>      >> -  CompressedClassSpaceSize = align_down_bounded(CompressedC
>>> lassSpaceSize,
>>>      >> _reserve_alignment);
>>>      >> - set_compressed_class_space_size(CompressedClassSpaceSize);
>>>      >> -
>>>      >>    // Initial virtual space size will be calculated at
>>> global_initialize()
>>>      >>    size_t min_metaspace_sz =
>>>      >>        VIRTUALSPACEMULTIPLIER * InitialBootClassLoaderMetaspac
>>> eSize;
>>>      >> @@ -3341,6 +3338,8 @@
>>>      >>                    min_metaspace_sz);
>>>      >>    }
>>>      >>
>>>      >> +  CompressedClassSpaceSize = align_down_bounded(CompressedC
>>> lassSpaceSize,
>>>      >> _reserve_alignment);
>>>      >> + set_compressed_class_space_size(CompressedClassSpaceSize);
>>>      >>  }
>>>      >>
>>>      >>  void Metaspace::global_initialize() {
>>>      >>
>>>      >> Best,
>>>      >> Man
>>>      >>
>>>      >> On Wed, Nov 29, 2017 at 3:21 PM, Man Cao <[hidden email]
>>> <mailto:[hidden email]>> wrote:
>>>      >>>
>>>      >>> Hello,
>>>      >>>
>>>      >>> This patch is a follow-up fix for
>>>      >>> https://bugs.openjdk.java.net/browse/JDK-8087291 <
>>> https://bugs.openjdk.java.net/browse/JDK-8087291>
>>>      >>>
>>>      >>> This patch moves the call to set_compressed_class_space_size()
>>> after the
>>>      >>> flag value for CompressedClassSpaceSize is ergo-initialized,
>>> fixing the
>>>      >>> issue that the reserved size for compressed class space and
>>> metaspace is
>>>      >>> excessively large when MaxMetaspaceSize is set to a small
>>> value. More
>>>      >>> discussion about it is available here:
>>>      >>>
>>>      >>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2
>>> 017-November/025200.html <http://mail.openjdk.java.net/
>>> pipermail/hotspot-runtime-dev/2017-November/025200.html>
>>>      >>>
>>>      >>> This code patch is attached. Could anyone review and/or sponsor
>>> this
>>>      >>> patch?
>>>      >>>
>>>      >>> Best,
>>>      >>> Man
>>>      >>
>>>      >>
>>>      >
>>>
>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: Make reserved_size for compressed class space and metaspace respect the ergo-initialized CompressedClassSpaceSize flag value

coleen.phillimore
Man, It has to be a new bug.  I filed bug:
https://bugs.openjdk.java.net/browse/JDK-8193386

and will sponsor this.
thank you,
Coleen

On 12/11/17 7:38 PM, Man Cao wrote:

> Hi Coleen,
>
> Thanks for sponsoring this patch! Yes, I'm covered by the agreement
> signed by Google Inc. [1]
> Could this patch be a part of
> https://bugs.openjdk.java.net/browse/JDK-8087291, as this is a simple
> follow-up fix?
> If not, could you or someone else create a bug, as I don't have an
> account to create one?
>
> [1] http://www.oracle.com/technetwork/community/oca-486395.html#g 
> <http://www.oracle.com/technetwork/community/oca-486395.html#g>
>
> -Man
>
> On Mon, Dec 11, 2017 at 2:27 PM, <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Hi this looks good and I will sponsor this with you as contributor
>     using [hidden email] <mailto:[hidden email]> .
>
>     Have you signed the contributor agreement or are covered by one?
>
>     thanks,
>     Coleen
>
>     On 12/9/17 7:37 AM, Yasumasa Suenaga wrote:
>
>         Hi Man,
>
>         Looks good, but you need to get a sponsor.
>
>
>         Thanks,
>
>         Yasumasa
>
>
>         On 2017/12/09 11:57, Man Cao wrote:
>
>             Hi Yasumasa,
>
>             Thanks for the review! Although I think moving or not
>             moving "CompressedClassSpaceSize =
>             align_size_down_bounded()" are equivalent because of logic
>             of alignment and FLAG_SET_ERGO, I made the change you
>             suggested anyway.
>             The new patch is attached and inlined below.
>
>
>             --- old/src/hotspot/share/memory/metaspace.cpp2017-12-08
>             18:42:48.960285998 -0800
>             +++ new/src/hotspot/share/memory/metaspace.cpp2017-12-08
>             18:42:48.600288972 -0800
>             @@ -3322,7 +3322,6 @@
>                 MaxMetaspaceExpansion =
>             align_down_bounded(MaxMetaspaceExpansion, _commit_alignment);
>                 CompressedClassSpaceSize =
>             align_down_bounded(CompressedClassSpaceSize,
>             _reserve_alignment);
>             -  set_compressed_class_space_size(CompressedClassSpaceSize);
>                 // Initial virtual space size will be calculated at
>             global_initialize()
>                 size_t min_metaspace_sz =
>             @@ -3341,6 +3340,7 @@
>                                 min_metaspace_sz);
>                 }
>             +  set_compressed_class_space_size(CompressedClassSpaceSize);
>               }
>               void Metaspace::global_initialize() {
>
>
>             Thanks,
>             Man
>
>             On Thu, Dec 7, 2017 at 9:44 PM, Yasumasa Suenaga
>             <[hidden email] <mailto:[hidden email]>
>             <mailto:[hidden email] <mailto:[hidden email]>>>
>             wrote:
>
>                 Hi Man,
>
>                 CompressedClassSpaceSize might be modified by
>             FLAG_SET_ERGO.
>                 So I think you need to move
>             set_compressed_class_space_size() only.
>
>
>                 Thanks,
>
>                 Yasumasa
>
>
>
>                 2017-12-08 3:42 GMT+09:00 Man Cao <[hidden email]
>             <mailto:[hidden email]> <mailto:[hidden email]
>             <mailto:[hidden email]>>>:
>                  > Hello,
>                  >
>                  > This is a friendly ping. Could anyone review or
>             sponsor this change? It's
>                  > just a two-liner change.
>                  >
>                  > -Man
>                  >
>                  > On Thu, Nov 30, 2017 at 2:03 PM, Man Cao
>             <[hidden email] <mailto:[hidden email]>
>             <mailto:[hidden email] <mailto:[hidden email]>>> wrote:
>                  >>
>                  >> I realized that the email attachment is probably
>             dropped by the mailing
>                  >> list, so below is the inlined patch.
>                  >>
>                  >> --- old/src/hotspot/share/memory/metaspace.cpp
>             2017-11-29
>                  >> 14:56:59.017118444 -0800
>                  >> +++ new/src/hotspot/share/memory/metaspace.cpp
>             2017-11-29
>                  >> 14:56:58.657121375 -0800
>                  >> @@ -3321,9 +3321,6 @@
>                  >>    MinMetaspaceExpansion =
>             align_down_bounded(MinMetaspaceExpansion,
>                  >> _commit_alignment);
>                  >>    MaxMetaspaceExpansion =
>             align_down_bounded(MaxMetaspaceExpansion,
>                  >> _commit_alignment);
>                  >>
>                  >> -  CompressedClassSpaceSize =
>             align_down_bounded(CompressedClassSpaceSize,
>                  >> _reserve_alignment);
>                  >> -
>             set_compressed_class_space_size(CompressedClassSpaceSize);
>                  >> -
>                  >>    // Initial virtual space size will be
>             calculated at global_initialize()
>                  >>    size_t min_metaspace_sz =
>                  >>        VIRTUALSPACEMULTIPLIER *
>             InitialBootClassLoaderMetaspaceSize;
>                  >> @@ -3341,6 +3338,8 @@
>                  >> min_metaspace_sz);
>                  >>    }
>                  >>
>                  >> +  CompressedClassSpaceSize =
>             align_down_bounded(CompressedClassSpaceSize,
>                  >> _reserve_alignment);
>                  >> +
>             set_compressed_class_space_size(CompressedClassSpaceSize);
>                  >>  }
>                  >>
>                  >>  void Metaspace::global_initialize() {
>                  >>
>                  >> Best,
>                  >> Man
>                  >>
>                  >> On Wed, Nov 29, 2017 at 3:21 PM, Man Cao
>             <[hidden email] <mailto:[hidden email]>
>             <mailto:[hidden email] <mailto:[hidden email]>>> wrote:
>                  >>>
>                  >>> Hello,
>                  >>>
>                  >>> This patch is a follow-up fix for
>                  >>> https://bugs.openjdk.java.net/browse/JDK-8087291
>             <https://bugs.openjdk.java.net/browse/JDK-8087291>
>             <https://bugs.openjdk.java.net/browse/JDK-8087291
>             <https://bugs.openjdk.java.net/browse/JDK-8087291>>
>                  >>>
>                  >>> This patch moves the call to
>             set_compressed_class_space_size() after the
>                  >>> flag value for CompressedClassSpaceSize is
>             ergo-initialized, fixing the
>                  >>> issue that the reserved size for compressed class
>             space and metaspace is
>                  >>> excessively large when MaxMetaspaceSize is set to
>             a small value. More
>                  >>> discussion about it is available here:
>                  >>>
>                  >>>
>             http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2017-November/025200.html
>             <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2017-November/025200.html>
>             <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2017-November/025200.html
>             <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2017-November/025200.html>>
>
>                  >>>
>                  >>> This code patch is attached. Could anyone review
>             and/or sponsor this
>                  >>> patch?
>                  >>>
>                  >>> Best,
>                  >>> Man
>                  >>
>                  >>
>                  >
>
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Make reserved_size for compressed class space and metaspace respect the ergo-initialized CompressedClassSpaceSize flag value

Man Cao
Great! Thanks again!

-Man

On Tue, Dec 12, 2017 at 8:29 AM, <[hidden email]> wrote:

> Man, It has to be a new bug.  I filed bug:
> https://bugs.openjdk.java.net/browse/JDK-8193386
>
> and will sponsor this.
> thank you,
> Coleen
>
>
> On 12/11/17 7:38 PM, Man Cao wrote:
>
> Hi Coleen,
>
> Thanks for sponsoring this patch! Yes, I'm covered by the agreement signed
> by Google Inc. [1]
> Could this patch be a part of https://bugs.openjdk.java.net/
> browse/JDK-8087291, as this is a simple follow-up fix?
> If not, could you or someone else create a bug, as I don't have an account
> to create one?
>
> [1] http://www.oracle.com/technetwork/community/oca-486395.html#g
>
> -Man
>
> On Mon, Dec 11, 2017 at 2:27 PM, <[hidden email]> wrote:
>
>> Hi this looks good and I will sponsor this with you as contributor using
>> [hidden email] .
>>
>> Have you signed the contributor agreement or are covered by one?
>>
>> thanks,
>> Coleen
>>
>> On 12/9/17 7:37 AM, Yasumasa Suenaga wrote:
>>
>>> Hi Man,
>>>
>>> Looks good, but you need to get a sponsor.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2017/12/09 11:57, Man Cao wrote:
>>>
>>>> Hi Yasumasa,
>>>>
>>>> Thanks for the review! Although I think moving or not moving
>>>> "CompressedClassSpaceSize = align_size_down_bounded()" are equivalent
>>>> because of logic of alignment and FLAG_SET_ERGO, I made the change you
>>>> suggested anyway.
>>>> The new patch is attached and inlined below.
>>>>
>>>>
>>>> --- old/src/hotspot/share/memory/metaspace.cpp2017-12-08
>>>> 18:42:48.960285998 -0800
>>>> +++ new/src/hotspot/share/memory/metaspace.cpp2017-12-08
>>>> 18:42:48.600288972 -0800
>>>> @@ -3322,7 +3322,6 @@
>>>>     MaxMetaspaceExpansion = align_down_bounded(MaxMetaspaceExpansion,
>>>> _commit_alignment);
>>>>     CompressedClassSpaceSize = align_down_bounded(CompressedClassSpaceSize,
>>>> _reserve_alignment);
>>>> -  set_compressed_class_space_size(CompressedClassSpaceSize);
>>>>     // Initial virtual space size will be calculated at
>>>> global_initialize()
>>>>     size_t min_metaspace_sz =
>>>> @@ -3341,6 +3340,7 @@
>>>>                     min_metaspace_sz);
>>>>     }
>>>> +  set_compressed_class_space_size(CompressedClassSpaceSize);
>>>>   }
>>>>   void Metaspace::global_initialize() {
>>>>
>>>>
>>>> Thanks,
>>>> Man
>>>>
>>>> On Thu, Dec 7, 2017 at 9:44 PM, Yasumasa Suenaga <[hidden email]
>>>> <mailto:[hidden email]>> wrote:
>>>>
>>>>     Hi Man,
>>>>
>>>>     CompressedClassSpaceSize might be modified by FLAG_SET_ERGO.
>>>>     So I think you need to move set_compressed_class_space_size() only.
>>>>
>>>>
>>>>     Thanks,
>>>>
>>>>     Yasumasa
>>>>
>>>>
>>>>
>>>>     2017-12-08 3:42 GMT+09:00 Man Cao <[hidden email] <mailto:
>>>> [hidden email]>>:
>>>>      > Hello,
>>>>      >
>>>>      > This is a friendly ping. Could anyone review or sponsor this
>>>> change? It's
>>>>      > just a two-liner change.
>>>>      >
>>>>      > -Man
>>>>      >
>>>>      > On Thu, Nov 30, 2017 at 2:03 PM, Man Cao <[hidden email]
>>>> <mailto:[hidden email]>> wrote:
>>>>      >>
>>>>      >> I realized that the email attachment is probably dropped by the
>>>> mailing
>>>>      >> list, so below is the inlined patch.
>>>>      >>
>>>>      >> --- old/src/hotspot/share/memory/metaspace.cpp 2017-11-29
>>>>      >> 14:56:59.017118444 -0800
>>>>      >> +++ new/src/hotspot/share/memory/metaspace.cpp 2017-11-29
>>>>      >> 14:56:58.657121375 -0800
>>>>      >> @@ -3321,9 +3321,6 @@
>>>>      >>    MinMetaspaceExpansion = align_down_bounded(MinMetaspac
>>>> eExpansion,
>>>>      >> _commit_alignment);
>>>>      >>    MaxMetaspaceExpansion = align_down_bounded(MaxMetaspac
>>>> eExpansion,
>>>>      >> _commit_alignment);
>>>>      >>
>>>>      >> -  CompressedClassSpaceSize = align_down_bounded(CompressedC
>>>> lassSpaceSize,
>>>>      >> _reserve_alignment);
>>>>      >> - set_compressed_class_space_size(CompressedClassSpaceSize);
>>>>      >> -
>>>>      >>    // Initial virtual space size will be calculated at
>>>> global_initialize()
>>>>      >>    size_t min_metaspace_sz =
>>>>      >>        VIRTUALSPACEMULTIPLIER * InitialBootClassLoaderMetaspac
>>>> eSize;
>>>>      >> @@ -3341,6 +3338,8 @@
>>>>      >>                    min_metaspace_sz);
>>>>      >>    }
>>>>      >>
>>>>      >> +  CompressedClassSpaceSize = align_down_bounded(CompressedC
>>>> lassSpaceSize,
>>>>      >> _reserve_alignment);
>>>>      >> + set_compressed_class_space_size(CompressedClassSpaceSize);
>>>>      >>  }
>>>>      >>
>>>>      >>  void Metaspace::global_initialize() {
>>>>      >>
>>>>      >> Best,
>>>>      >> Man
>>>>      >>
>>>>      >> On Wed, Nov 29, 2017 at 3:21 PM, Man Cao <[hidden email]
>>>> <mailto:[hidden email]>> wrote:
>>>>      >>>
>>>>      >>> Hello,
>>>>      >>>
>>>>      >>> This patch is a follow-up fix for
>>>>      >>> https://bugs.openjdk.java.net/browse/JDK-8087291 <
>>>> https://bugs.openjdk.java.net/browse/JDK-8087291>
>>>>      >>>
>>>>      >>> This patch moves the call to set_compressed_class_space_size()
>>>> after the
>>>>      >>> flag value for CompressedClassSpaceSize is ergo-initialized,
>>>> fixing the
>>>>      >>> issue that the reserved size for compressed class space and
>>>> metaspace is
>>>>      >>> excessively large when MaxMetaspaceSize is set to a small
>>>> value. More
>>>>      >>> discussion about it is available here:
>>>>      >>>
>>>>      >>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2
>>>> 017-November/025200.html <http://mail.openjdk.java.net/
>>>> pipermail/hotspot-runtime-dev/2017-November/025200.html>
>>>>      >>>
>>>>      >>> This code patch is attached. Could anyone review and/or
>>>> sponsor this
>>>>      >>> patch?
>>>>      >>>
>>>>      >>> Best,
>>>>      >>> Man
>>>>      >>
>>>>      >>
>>>>      >
>>>>
>>>>
>>>>
>>
>
>