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

Joseph Provino joseph.provino at oracle.com
Wed May 27 14:12:29 UTC 2015


Please review.  It's a relatively simple change that fixes the bug and 
only comes into play
when we would otherwise throw OOME.

http://bussund0416.us.oracle.com/export/users/jprovino/hs-gc-8058967/hotspot/8058967/webrev.03

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




More information about the hotspot-gc-dev mailing list