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

[Joseph Provino]

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
>>>>>
>>>>
>>>
>>
>