RFR(XXS) 8190729: Adjustment to anonymous metadata space chunk allocation algorithm

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

RFR(XXS) 8190729: Adjustment to anonymous metadata space chunk allocation algorithm

Zhengyu Gu-2
Hi,

To follow up recent discussion of anonymous metadata space memory usage,
I would like submit this simple fix for review.

By continuing allocating anonymous metadata space from SpecializeChunk
pool, up to 4 chunks, it reduces the waste from 60+% to about 30%.


Bug: https://bugs.openjdk.java.net/browse/JDK-8190729
Webrev: http://cr.openjdk.java.net/~zgu/8190729/webrev.00/

Test:

   hotspot_tier1_runtime on Linux x64 (fastdebug and release)


Thanks,

-Zhengyu
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS) 8190729: Adjustment to anonymous metadata space chunk allocation algorithm

Thomas Stüfe-2
Hi Zhengyu,

looks good.

(I wonder whether if you just could have given the SpaceManager a back
pointer to its enclosing Metaspace instead of handing down _space_type and
mdType, but then, those are cleanups we can do later.)

In SpaceManager, could you please make _space_type - and _mdtype too -
const?

Why do you do this only for non-class metaspace?

Could please you embellish the comment in calc_chunk_size() a bit?

Would it be possible to have a regression test for this one, maybe as part
of the NMT tests? So that if we change the allocator logic in the future,
we can be sure not to reintroduce that bug. To me, it would be okay to do
this in a follow up item, if you are concerned about the jdk10 code freeze.

Thank you,

Kind Regards, Thomas


On Mon, Nov 27, 2017 at 6:09 PM, Zhengyu Gu <[hidden email]> wrote:

> Hi,
>
> To follow up recent discussion of anonymous metadata space memory usage, I
> would like submit this simple fix for review.
>
> By continuing allocating anonymous metadata space from SpecializeChunk
> pool, up to 4 chunks, it reduces the waste from 60+% to about 30%.
>
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8190729
> Webrev: http://cr.openjdk.java.net/~zgu/8190729/webrev.00/
>
> Test:
>
>   hotspot_tier1_runtime on Linux x64 (fastdebug and release)
>
>
> Thanks,
>
> -Zhengyu
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS) 8190729: Adjustment to anonymous metadata space chunk allocation algorithm

Zhengyu Gu-2
Hi Thomas,

Thanks for the quick review.

On 11/27/2017 01:36 PM, Thomas Stüfe wrote:

> Hi Zhengyu,
>
> looks good.
>
> (I wonder whether if you just could have given the SpaceManager a back
> pointer to its enclosing Metaspace instead of handing down _space_type
> and mdType, but then, those are cleanups we can do later.)
>
> In SpaceManager, could you please make _space_type - and _mdtype too -
> const?
Fixed.

>
> Why do you do this only for non-class metaspace?

Cause I have yet seen any anonymous class space above 1K, I would rather
leave them alone.

>
> Could please you embellish the comment in calc_chunk_size() a bit?
Done.

>
> Would it be possible to have a regression test for this one, maybe as
> part of the NMT tests? So that if we change the allocator logic in the
> future, we can be sure not to reintroduce that bug. To me, it would be
> okay to do this in a follow up item, if you are concerned about the
> jdk10 code freeze.

Probably gtest or JVM internal test (?), I will have to learn how to
write one.


Updated webrev: http://cr.openjdk.java.net/~zgu/8190729/webrev.01/

Thanks,

-Zhengyu



> Thank you,
>
> Kind Regards, Thomas
>
>
> On Mon, Nov 27, 2017 at 6:09 PM, Zhengyu Gu <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Hi,
>
>     To follow up recent discussion of anonymous metadata space memory
>     usage, I would like submit this simple fix for review.
>
>     By continuing allocating anonymous metadata space from
>     SpecializeChunk pool, up to 4 chunks, it reduces the waste from 60+%
>     to about 30%.
>
>
>     Bug: https://bugs.openjdk.java.net/browse/JDK-8190729
>     <https://bugs.openjdk.java.net/browse/JDK-8190729>
>     Webrev: http://cr.openjdk.java.net/~zgu/8190729/webrev.00/
>     <http://cr.openjdk.java.net/~zgu/8190729/webrev.00/>
>
>     Test:
>
>        hotspot_tier1_runtime on Linux x64 (fastdebug and release)
>
>
>     Thanks,
>
>     -Zhengyu
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS) 8190729: Adjustment to anonymous metadata space chunk allocation algorithm

Thomas Stüfe-2
Hi Zhengyu,

On Mon, Nov 27, 2017 at 8:38 PM, Zhengyu Gu <[hidden email]> wrote:

> Hi Thomas,
>
> Thanks for the quick review.
>
> On 11/27/2017 01:36 PM, Thomas Stüfe wrote:
>
>> Hi Zhengyu,
>>
>> looks good.
>>
>> (I wonder whether if you just could have given the SpaceManager a back
>> pointer to its enclosing Metaspace instead of handing down _space_type and
>> mdType, but then, those are cleanups we can do later.)
>>
>> In SpaceManager, could you please make _space_type - and _mdtype too -
>> const?
>>
> Fixed.
>
>
>> Why do you do this only for non-class metaspace?
>>
>
> Cause I have yet seen any anonymous class space above 1K, I would rather
> leave them alone.
>
>
Also it occurred to me that the use of "SpecializedChunk" enum implies
non-class metaspace, otherwise we would have to use "ClassSpecializedChunk"
(they currently have identical values, but that may change). So, in its
current form, this patch is clearly bound to Non-Class metaspace.

I think the change makes sense and is minimal invasive. But I still wish we
could do things simpler, maybe as part of a future cleanup? All this
information we hand down to the SpaceManager is just there to help him to
decide on a chunk-size-progression strategy. If we could come up with a
more general way to specify such a strategy (e.g. - from the top of my head
- a list of size-threshold-to-next-chunk-size tupels) - SpaceManager would
not need any special knowledge about mdType, space type, chunk sizes etc.
and would in turn become quite a bit simpler.


>
>> Could please you embellish the comment in calc_chunk_size() a bit?
>>
> Done.
>
> Thanks, comment is good.


>
>> Would it be possible to have a regression test for this one, maybe as
>> part of the NMT tests? So that if we change the allocator logic in the
>> future, we can be sure not to reintroduce that bug. To me, it would be okay
>> to do this in a follow up item, if you are concerned about the jdk10 code
>> freeze.
>>
>
> Probably gtest or JVM internal test (?), I will have to learn how to write
> one.
>
>
>
jtreg tests would be the most fitting. Quite easy to write actually. Some
examples are here:

/test/hotspot/jtreg/runtime/NMT/


> Updated webrev: http://cr.openjdk.java.net/~zgu/8190729/webrev.01/
>
> Thanks,
>
> -Zhengyu
>
>
Thanks. To me, change looks good. No need for a new review.

..Thomas


>
>
> Thank you,
>>
>> Kind Regards, Thomas
>>
>>
>> On Mon, Nov 27, 2017 at 6:09 PM, Zhengyu Gu <[hidden email] <mailto:
>> [hidden email]>> wrote:
>>
>>     Hi,
>>
>>     To follow up recent discussion of anonymous metadata space memory
>>     usage, I would like submit this simple fix for review.
>>
>>     By continuing allocating anonymous metadata space from
>>     SpecializeChunk pool, up to 4 chunks, it reduces the waste from 60+%
>>     to about 30%.
>>
>>
>>     Bug: https://bugs.openjdk.java.net/browse/JDK-8190729
>>     <https://bugs.openjdk.java.net/browse/JDK-8190729>
>>     Webrev: http://cr.openjdk.java.net/~zgu/8190729/webrev.00/
>>     <http://cr.openjdk.java.net/~zgu/8190729/webrev.00/>
>>
>>     Test:
>>
>>        hotspot_tier1_runtime on Linux x64 (fastdebug and release)
>>
>>
>>     Thanks,
>>
>>     -Zhengyu
>>
>>
>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS) 8190729: Adjustment to anonymous metadata space chunk allocation algorithm

Zhengyu Gu-2
>
>
>
> jtreg tests would be the most fitting. Quite easy to write actually.
> Some examples are here:
>
> /test/hotspot/jtreg/runtime/NMT/

Okay, I filed 8191931 (https://bugs.openjdk.java.net/browse/JDK-8191931)
for the test.

Thanks,

-Zhengyu

>
>     Updated webrev: http://cr.openjdk.java.net/~zgu/8190729/webrev.01/
>     <http://cr.openjdk.java.net/~zgu/8190729/webrev.01/>
>
>     Thanks,
>
>     -Zhengyu
>
>
> Thanks. To me, change looks good. No need for a new review.
>
> ..Thomas
>
>
>
>         Thank you,
>
>         Kind Regards, Thomas
>
>
>         On Mon, Nov 27, 2017 at 6:09 PM, Zhengyu Gu <[hidden email]
>         <mailto:[hidden email]> <mailto:[hidden email]
>         <mailto:[hidden email]>>> wrote:
>
>              Hi,
>
>              To follow up recent discussion of anonymous metadata space
>         memory
>              usage, I would like submit this simple fix for review.
>
>              By continuing allocating anonymous metadata space from
>              SpecializeChunk pool, up to 4 chunks, it reduces the waste
>         from 60+%
>              to about 30%.
>
>
>              Bug: https://bugs.openjdk.java.net/browse/JDK-8190729
>         <https://bugs.openjdk.java.net/browse/JDK-8190729>
>              <https://bugs.openjdk.java.net/browse/JDK-8190729
>         <https://bugs.openjdk.java.net/browse/JDK-8190729>>
>              Webrev: http://cr.openjdk.java.net/~zgu/8190729/webrev.00/
>         <http://cr.openjdk.java.net/~zgu/8190729/webrev.00/>
>              <http://cr.openjdk.java.net/~zgu/8190729/webrev.00/
>         <http://cr.openjdk.java.net/~zgu/8190729/webrev.00/>>
>
>              Test:
>
>                 hotspot_tier1_runtime on Linux x64 (fastdebug and release)
>
>
>              Thanks,
>
>              -Zhengyu
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS) 8190729: Adjustment to anonymous metadata space chunk allocation algorithm

coleen.phillimore
In reply to this post by Zhengyu Gu-2

Hi, this is a bit bigger than XXS but it looks good to me.   Only one nit:

+ if (_space_type == Metaspace::AnonymousMetaspaceType
+ && _mdtype == Metaspace::NonClassType
+ && sum_count_in_chunks_in_use(SpecializedIndex) <
_anon_metadata_specialize_chunk_limit
+ && word_size + Metachunk::overhead() <= SpecializedChunk) {
+ return SpecializedChunk;
+ }
+


I think the coding style is that the && are on the right hand side.   In
any case, I'll sponsor it for you.  Can you commit, and regenerate the
webrev with the commit message?   So I can use wget to get the patch.

thanks!
Coleen

On 11/27/17 12:09 PM, Zhengyu Gu wrote:

> Hi,
>
> To follow up recent discussion of anonymous metadata space memory
> usage, I would like submit this simple fix for review.
>
> By continuing allocating anonymous metadata space from SpecializeChunk
> pool, up to 4 chunks, it reduces the waste from 60+% to about 30%.
>
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8190729
> Webrev: http://cr.openjdk.java.net/~zgu/8190729/webrev.00/
>
> Test:
>
>   hotspot_tier1_runtime on Linux x64 (fastdebug and release)
>
>
> Thanks,
>
> -Zhengyu

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS) 8190729: Adjustment to anonymous metadata space chunk allocation algorithm

Zhengyu Gu-2
Hi Coleen,

I fixed the style and generated committed webrev:

http://cr.openjdk.java.net/~zgu/8190729/webrev.02/

Thanks a lot!

-Zhengyu

On 11/27/2017 05:04 PM, [hidden email] wrote:

>
> Hi, this is a bit bigger than XXS but it looks good to me.   Only one nit:
>
> + if (_space_type == Metaspace::AnonymousMetaspaceType
> + && _mdtype == Metaspace::NonClassType
> + && sum_count_in_chunks_in_use(SpecializedIndex) <
> _anon_metadata_specialize_chunk_limit
> + && word_size + Metachunk::overhead() <= SpecializedChunk) {
> + return SpecializedChunk;
> + }
> +
>
>
> I think the coding style is that the && are on the right hand side.   In
> any case, I'll sponsor it for you.  Can you commit, and regenerate the
> webrev with the commit message?   So I can use wget to get the patch.
>
> thanks!
> Coleen
>
> On 11/27/17 12:09 PM, Zhengyu Gu wrote:
>> Hi,
>>
>> To follow up recent discussion of anonymous metadata space memory
>> usage, I would like submit this simple fix for review.
>>
>> By continuing allocating anonymous metadata space from SpecializeChunk
>> pool, up to 4 chunks, it reduces the waste from 60+% to about 30%.
>>
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8190729
>> Webrev: http://cr.openjdk.java.net/~zgu/8190729/webrev.00/
>>
>> Test:
>>
>>   hotspot_tier1_runtime on Linux x64 (fastdebug and release)
>>
>>
>> Thanks,
>>
>> -Zhengyu
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS) 8190729: Adjustment to anonymous metadata space chunk allocation algorithm

Andrew Dinn
In reply to this post by Zhengyu Gu-2
On 27/11/17 19:38, Zhengyu Gu wrote:
> On 11/27/2017 01:36 PM, Thomas Stüfe wrote:
>>
>> Why do you do this only for non-class metaspace?
>
> Cause I have yet seen any anonymous class space above 1K, I would rather
> leave them alone.

That's no surprise. An InstanceKlass is ~450 bytes plus whatever
(little) space is taken up by the oopmap, vtable and itable. So, even
two classes will likely fit into a 1K chunk and one certainly will.

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS) 8190729: Adjustment to anonymous metadata space chunk allocation algorithm

Andrew Dinn
In reply to this post by Zhengyu Gu-2
On 27/11/17 22:25, Zhengyu Gu wrote:
> I fixed the style and generated committed webrev:
>
> http://cr.openjdk.java.net/~zgu/8190729/webrev.02/
Looks good.

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS) 8190729: Adjustment to anonymous metadata space chunk allocation algorithm

Zhengyu Gu-2
Thanks for the review, Andrew!

-Zhengyu

On 11/28/2017 05:01 AM, Andrew Dinn wrote:

> On 27/11/17 22:25, Zhengyu Gu wrote:
>> I fixed the style and generated committed webrev:
>>
>> http://cr.openjdk.java.net/~zgu/8190729/webrev.02/
> Looks good.
>
> regards,
>
>
> Andrew Dinn
> -----------
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
>