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

[Jon Masamitsu]

Joe,

Looks good.  Thanks for going the extra mile (or two) on
this one.

Reviewed.

Jon

On 05/27/2015 08:08 AM, Joseph Provino wrote:
> Sorry, bad URL.  Try this one:
>
> http://cr.openjdk.java.net/~jprovino/8058967/webrev.03
>
> On 5/27/2015 10:12 AM, Joseph Provino wrote:
>>
>> Please review.  It's a relatively simple change that fixes the bug 
>> and only comes into play
>> when we would otherwise throw OOME.
>>
>> thanks.
>>
>> joe
>>
>> On 5/16/2015 2:38 PM, Joseph Provino wrote:
>>> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>