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

[Jon Masamitsu]

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