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

[Thomas Stüfe]

Hi Zhengyu,

On Mon, Nov 27, 2017 at 8:38 PM, Zhengyu Gu <zgu at redhat.com> 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 <zgu at redhat.com <mailto:
>> zgu at redhat.com>> 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
>>
>>
>>