RFR (XS): JDK-8058967 metaspace/shrink_grow/CompressedClassSpaceSize fails with OOM: Compressed class space

[Joseph Provino]

I just noticed Jon's comments.  Latest webrev is here:

http://cr.openjdk.java.net/~jprovino/8058967/webrev.02

joe

On 5/15/2015 6:14 PM, Jon Masamitsu wrote:
>
>
> On 5/15/2015 12:16 PM, Joseph Provino wrote:
>>
>> On 5/15/2015 2:29 PM, Jon Masamitsu wrote:
>>>
>>>
>>> On 5/15/2015 6:50 AM, Joseph Provino wrote:
>>>> Fixed the bug id in the subject line.
>>>>
>>>> On 5/14/2015 5:26 PM, Kim Barrett wrote:
>>>>> On May 14, 2015, at 3:45 PM, Joseph Provino 
>>>>> <joseph.provino at oracle.com> wrote:
>>>>>> Can I get reviews for the following fix?  The problem is that an 
>>>>>> attempt is made
>>>>>> to allocate a medium chunk for class metaspace but there are no 
>>>>>> medium chunks available.
>>>>>> However there are many small chunks available.
>>>>>>
>>>>>> If the allocation request size fits in a small chunk, the fix is 
>>>>>> to try allocating a small
>>>>>> chunk after failing to allocate a medium chunk.
>>>>>>
>>>>>> Changes are in one file.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~jprovino/8058967/webrev.00
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8058967
>>>>>>
>>>>>> Passed JPRT
>>>>>>
>>>>>> Aurora ad-hoc test of vm.parallel_class_loading:
>>>>>>
>>>>>> http://aurora.ru.oracle.com/functional/faces/RunDetails.xhtml?names=882047.VMSQE.adhoc.JPRT-1 
>>>>>>
>>>>>>
>>>>>> thanks.
>>>>>>
>>>>>> joe
>>>>> The proposed change violates the policy described in
>>>>> calc_chunk_size().  Either that policy is important and prevents this
>>>>> change, or the policy description needs to be updated (and an
>>>>> explanation of why that's ok needs to be provided).
>>>> I think the policy is important to reduce fragmentation but it also 
>>>> causes problems
>>>> when there is no room to expand and there are no medium chunks 
>>>> available.
>>>>
>>>> It seems better to use one of the small chunks than to throw OOME.
>>>>
>>>> I could change the comment to say something like that.
>>>
>>> I think the statement of the policy should be updated.
>>
>> How does this sound?  Should the comment be with the "if" statement or
>> where _small_chunk_limit is declared?
>
> I would put it as a description of
>
> 2063 MetaWord* SpaceManager::grow_and_allocate(size_t word_size) {
>
> That is, put it as a block statement before line 2063.
>
>
> Existing problem but would you mind changing
>
> 2084   // Get another chunk out of the virtual space
>
> to
>
> 2084   // Get another chunk
>
>
> because get_new_chunk() could get the chunk from the freelist.
>
>
>
>>
>>   /*
>>    * The policy is to allocate up to _small_chunk_limit small chunks
>>    * after which only medium chunks are allocated.  This is done to
>>    * reduce fragmentation.  In some cases, this can result in a lot
>>    * of small chunks being allocated to the point where it's not
>>    * possible to expand.  If this happens, there may be no medium chunks
>>    * available and  OOME would be thrown.  Instead of doing that,
>>    * if the allocation request size fits in a small chunk, an attempt
>>    * is made to allocate a small chunk.
>>    */
>>   if (next == NULL &&
>>       word_size + Metachunk::overhead() <= small_chunk_size() &&
>>       grow_chunks_by_words == medium_chunk_size()) {
>>     /*
>>      * There are no medium chunks available but a small chunk is big 
>> enough.
>>      * See if a small chunk is available.
>>      */
>>     next = get_new_chunk(word_size, small_chunk_size());
>>   }
>
> The if-condition looks a bit hard to read.  How about (beware, I 
> didn't count
> the parenthesis)
>
> bool SpaceManager::should_try_smaller_chunk(size_t word_size, size_t 
> chunk_size) {
>   return (word_size + Metachunk::overhead() <= small_chunk_size() &&
>       chunk_size == medium_chunk_size()));
> }
>
> and use
>
>   if (next == NULL && should_try_smaller_chunk(word_size, 
> grow_chunks_by_words)) {
>
> Jon
>
>>
>> joe
>>
>>>
>>>
>>> Jon
>>>
>>>>
>>>>> I wonder if the real problem might be that the test is making
>>>>> unreasonable assumptions, and needs to be changed.
>>>> I don't think so.  The test creates enough class loaders to use up 
>>>> all of class
>>>> metaspace.  Then it clears references to all the class loaders and 
>>>> does a gc.
>>>> The gc frees the class metaspace.  Then the test tries to create 
>>>> one more
>>>> class loader and OOME is thrown.
>>>>
>>>> It's certainly an unusual test case but seems reasonable to expect 
>>>> it to work.
>>>>
>>>> I think the real problem is in how metaspace is handled. The policy 
>>>> to only allocate
>>>> up to 4 small chunks per SpaceManager seems to be bad if most of 
>>>> the time <= 4 small chunks are needed.
>>>> When memory is freed there are going to be lots of small chunks and 
>>>> few if any medium chunks.
>>>>
>>>>>
>>>>> 2090   if (next == NULL && word_size + Metachunk::overhead() <= 
>>>>> small_chunk_size() &&
>>>>> 2091     grow_chunks_by_words == medium_chunk_size()) {
>>>>>
>>>>> I think line breaks between expressions would make that test a lot
>>>>> easier to read.  And indentation should be based on the open-paren
>>>>> rather than the if; as written, grow_chunks_by_words is indented
>>>>> appropriately for the body of the if.
>>>>>
>>>>
>>>> like this?
>>>>
>>>>   if (next == NULL &&
>>>>       word_size + Metachunk::overhead() <= small_chunk_size() &&
>>>>       grow_chunks_by_words == medium_chunk_size()) {
>>>>
>>>> joe
>>>
>>
>