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