RFR: 8170737: Not enough old space utilisation

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR: 8170737: Not enough old space utilisation

Michail Chernov
Hi,

Could I have a reviews for this change, please?

http://cr.openjdk.java.net/~mchernov/8170737/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8170737

Test fails because Runtime.maxMemory() results depends on the heap size
shrinking. So need to calculate the expected heap occupation before a
garbage allocation. Tests were updated accordingly. Also
GcProvokerImpl.java was removed because it was not removed on previous
commit by mistake.
Tested via RBT.

Thanks,
Michail
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170737: Not enough old space utilisation

Dmitry Fazunenko
Hi Michail,

GarbageProducer tries to predict at the beginning how many objects to
allocate later to achieve target memory usage percent.
This way doesn't look like a good idea:
- objects created by libraries (during initialization) may not be
collected yet
- eatMetaspace() besides allocating metaspace consumes some heap space,
but this amount is not taken into account.

Will it make sense to use MXBean for heap as well as for metaspace?

Thanks,
Dima

ps: one more nit: "GarbageProducer{" - please insert a space.

On 13.01.2017 14:56, Michail Chernov wrote:

> Hi,
>
> Could I have a reviews for this change, please?
>
> http://cr.openjdk.java.net/~mchernov/8170737/webrev.00/
> https://bugs.openjdk.java.net/browse/JDK-8170737
>
> Test fails because Runtime.maxMemory() results depends on the heap
> size shrinking. So need to calculate the expected heap occupation
> before a garbage allocation. Tests were updated accordingly. Also
> GcProvokerImpl.java was removed because it was not removed on previous
> commit by mistake.
> Tested via RBT.
>
> Thanks,
> Michail

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170737: Not enough old space utilisation

Michail Chernov
Hi Dima,

Thanks for your response.

Updated webrev:
http://cr.openjdk.java.net/~mchernov/8170737/webrev.01/

Tests were refactored.
There were several problems:
1. Checking of jstat output consistency should not be performed during
GC cycles because it can cause to additional failures. There is checking
in GarbageProducerTestBase.java that during jstat execution were not any
GC cycles. Jstat gets performance counters values sequentially, so GC
cycles should be avoided.
2. New gen size should be limited, because there can be OOME while
eating 70% of heap on some configurations.
3. Tests failed to get expected old gen occupancy because background VM
activity can change real occupancy. Now tests check real heap occupancy
(using MXBean) and compares it with jstat output.

The testing is in progress.

Thanks,
Michail

On 13/01/2017 19:55, Dmitry Fazunenko wrote:

> Hi Michail,
>
> GarbageProducer tries to predict at the beginning how many objects to
> allocate later to achieve target memory usage percent.
> This way doesn't look like a good idea:
> - objects created by libraries (during initialization) may not be
> collected yet
> - eatMetaspace() besides allocating metaspace consumes some heap
> space, but this amount is not taken into account.
>
> Will it make sense to use MXBean for heap as well as for metaspace?
>
> Thanks,
> Dima
>
> ps: one more nit: "GarbageProducer{" - please insert a space.
>
> On 13.01.2017 14:56, Michail Chernov wrote:
>> Hi,
>>
>> Could I have a reviews for this change, please?
>>
>> http://cr.openjdk.java.net/~mchernov/8170737/webrev.00/
>> https://bugs.openjdk.java.net/browse/JDK-8170737
>>
>> Test fails because Runtime.maxMemory() results depends on the heap
>> size shrinking. So need to calculate the expected heap occupation
>> before a garbage allocation. Tests were updated accordingly. Also
>> GcProvokerImpl.java was removed because it was not removed on
>> previous commit by mistake.
>> Tested via RBT.
>>
>> Thanks,
>> Michail
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170737: Not enough old space utilisation

Michail Chernov
Hi,

I updated the webrev according to offline coments from Dmitry Fazunenko.
GarbageProducerTestBase was renamed to GarbageProducerTest.
GcCauseTest02 and GcTest02 now uses GarbageProducerTest directly without
inheritance.

http://cr.openjdk.java.net/~mchernov/8170737/webrev.02_to_01/
http://cr.openjdk.java.net/~mchernov/8170737/webrev.02/

Thanks,
Michail

On 24/01/2017 17:52, Michail Chernov wrote:

> Hi Dima,
>
> Thanks for your response.
>
> Updated webrev:
> http://cr.openjdk.java.net/~mchernov/8170737/webrev.01/
>
> Tests were refactored.
> There were several problems:
> 1. Checking of jstat output consistency should not be performed during
> GC cycles because it can cause to additional failures. There is
> checking in GarbageProducerTestBase.java that during jstat execution
> were not any GC cycles. Jstat gets performance counters values
> sequentially, so GC cycles should be avoided.
> 2. New gen size should be limited, because there can be OOME while
> eating 70% of heap on some configurations.
> 3. Tests failed to get expected old gen occupancy because background
> VM activity can change real occupancy. Now tests check real heap
> occupancy (using MXBean) and compares it with jstat output.
>
> The testing is in progress.
>
> Thanks,
> Michail
>
> On 13/01/2017 19:55, Dmitry Fazunenko wrote:
>> Hi Michail,
>>
>> GarbageProducer tries to predict at the beginning how many objects to
>> allocate later to achieve target memory usage percent.
>> This way doesn't look like a good idea:
>> - objects created by libraries (during initialization) may not be
>> collected yet
>> - eatMetaspace() besides allocating metaspace consumes some heap
>> space, but this amount is not taken into account.
>>
>> Will it make sense to use MXBean for heap as well as for metaspace?
>>
>> Thanks,
>> Dima
>>
>> ps: one more nit: "GarbageProducer{" - please insert a space.
>>
>> On 13.01.2017 14:56, Michail Chernov wrote:
>>> Hi,
>>>
>>> Could I have a reviews for this change, please?
>>>
>>> http://cr.openjdk.java.net/~mchernov/8170737/webrev.00/
>>> https://bugs.openjdk.java.net/browse/JDK-8170737
>>>
>>> Test fails because Runtime.maxMemory() results depends on the heap
>>> size shrinking. So need to calculate the expected heap occupation
>>> before a garbage allocation. Tests were updated accordingly. Also
>>> GcProvokerImpl.java was removed because it was not removed on
>>> previous commit by mistake.
>>> Tested via RBT.
>>>
>>> Thanks,
>>> Michail
>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170737: Not enough old space utilisation

Dmitry Fazunenko
Hi Michail,

new version looks good to me.
Thanks for addressing my comments.

-- Dima

On 26.01.2017 14:00, Michail Chernov wrote:

> Hi,
>
> I updated the webrev according to offline coments from Dmitry Fazunenko.
> GarbageProducerTestBase was renamed to GarbageProducerTest.
> GcCauseTest02 and GcTest02 now uses GarbageProducerTest directly
> without inheritance.
>
> http://cr.openjdk.java.net/~mchernov/8170737/webrev.02_to_01/
> http://cr.openjdk.java.net/~mchernov/8170737/webrev.02/
>
> Thanks,
> Michail
>
> On 24/01/2017 17:52, Michail Chernov wrote:
>> Hi Dima,
>>
>> Thanks for your response.
>>
>> Updated webrev:
>> http://cr.openjdk.java.net/~mchernov/8170737/webrev.01/
>>
>> Tests were refactored.
>> There were several problems:
>> 1. Checking of jstat output consistency should not be performed
>> during GC cycles because it can cause to additional failures. There
>> is checking in GarbageProducerTestBase.java that during jstat
>> execution were not any GC cycles. Jstat gets performance counters
>> values sequentially, so GC cycles should be avoided.
>> 2. New gen size should be limited, because there can be OOME while
>> eating 70% of heap on some configurations.
>> 3. Tests failed to get expected old gen occupancy because background
>> VM activity can change real occupancy. Now tests check real heap
>> occupancy (using MXBean) and compares it with jstat output.
>>
>> The testing is in progress.
>>
>> Thanks,
>> Michail
>>
>> On 13/01/2017 19:55, Dmitry Fazunenko wrote:
>>> Hi Michail,
>>>
>>> GarbageProducer tries to predict at the beginning how many objects
>>> to allocate later to achieve target memory usage percent.
>>> This way doesn't look like a good idea:
>>> - objects created by libraries (during initialization) may not be
>>> collected yet
>>> - eatMetaspace() besides allocating metaspace consumes some heap
>>> space, but this amount is not taken into account.
>>>
>>> Will it make sense to use MXBean for heap as well as for metaspace?
>>>
>>> Thanks,
>>> Dima
>>>
>>> ps: one more nit: "GarbageProducer{" - please insert a space.
>>>
>>> On 13.01.2017 14:56, Michail Chernov wrote:
>>>> Hi,
>>>>
>>>> Could I have a reviews for this change, please?
>>>>
>>>> http://cr.openjdk.java.net/~mchernov/8170737/webrev.00/
>>>> https://bugs.openjdk.java.net/browse/JDK-8170737
>>>>
>>>> Test fails because Runtime.maxMemory() results depends on the heap
>>>> size shrinking. So need to calculate the expected heap occupation
>>>> before a garbage allocation. Tests were updated accordingly. Also
>>>> GcProvokerImpl.java was removed because it was not removed on
>>>> previous commit by mistake.
>>>> Tested via RBT.
>>>>
>>>> Thanks,
>>>> Michail
>>>
>>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170737: Not enough old space utilisation

Michail Chernov
Hi Dima,

Thanks a lot for reviewing this.
Could someone else take a look at this change, please?

Thanks,
Michail

On 26/01/2017 14:54, Dmitry Fazunenko wrote:

> Hi Michail,
>
> new version looks good to me.
> Thanks for addressing my comments.
>
> -- Dima
>
> On 26.01.2017 14:00, Michail Chernov wrote:
>> Hi,
>>
>> I updated the webrev according to offline coments from Dmitry Fazunenko.
>> GarbageProducerTestBase was renamed to GarbageProducerTest.
>> GcCauseTest02 and GcTest02 now uses GarbageProducerTest directly
>> without inheritance.
>>
>> http://cr.openjdk.java.net/~mchernov/8170737/webrev.02_to_01/
>> http://cr.openjdk.java.net/~mchernov/8170737/webrev.02/
>>
>> Thanks,
>> Michail
>>
>> On 24/01/2017 17:52, Michail Chernov wrote:
>>> Hi Dima,
>>>
>>> Thanks for your response.
>>>
>>> Updated webrev:
>>> http://cr.openjdk.java.net/~mchernov/8170737/webrev.01/
>>>
>>> Tests were refactored.
>>> There were several problems:
>>> 1. Checking of jstat output consistency should not be performed
>>> during GC cycles because it can cause to additional failures. There
>>> is checking in GarbageProducerTestBase.java that during jstat
>>> execution were not any GC cycles. Jstat gets performance counters
>>> values sequentially, so GC cycles should be avoided.
>>> 2. New gen size should be limited, because there can be OOME while
>>> eating 70% of heap on some configurations.
>>> 3. Tests failed to get expected old gen occupancy because background
>>> VM activity can change real occupancy. Now tests check real heap
>>> occupancy (using MXBean) and compares it with jstat output.
>>>
>>> The testing is in progress.
>>>
>>> Thanks,
>>> Michail
>>>
>>> On 13/01/2017 19:55, Dmitry Fazunenko wrote:
>>>> Hi Michail,
>>>>
>>>> GarbageProducer tries to predict at the beginning how many objects
>>>> to allocate later to achieve target memory usage percent.
>>>> This way doesn't look like a good idea:
>>>> - objects created by libraries (during initialization) may not be
>>>> collected yet
>>>> - eatMetaspace() besides allocating metaspace consumes some heap
>>>> space, but this amount is not taken into account.
>>>>
>>>> Will it make sense to use MXBean for heap as well as for metaspace?
>>>>
>>>> Thanks,
>>>> Dima
>>>>
>>>> ps: one more nit: "GarbageProducer{" - please insert a space.
>>>>
>>>> On 13.01.2017 14:56, Michail Chernov wrote:
>>>>> Hi,
>>>>>
>>>>> Could I have a reviews for this change, please?
>>>>>
>>>>> http://cr.openjdk.java.net/~mchernov/8170737/webrev.00/
>>>>> https://bugs.openjdk.java.net/browse/JDK-8170737
>>>>>
>>>>> Test fails because Runtime.maxMemory() results depends on the heap
>>>>> size shrinking. So need to calculate the expected heap occupation
>>>>> before a garbage allocation. Tests were updated accordingly. Also
>>>>> GcProvokerImpl.java was removed because it was not removed on
>>>>> previous commit by mistake.
>>>>> Tested via RBT.
>>>>>
>>>>> Thanks,
>>>>> Michail
>>>>
>>>>
>>>
>>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170737: Not enough old space utilisation

Michail Chernov
In reply to this post by Dmitry Fazunenko
Hello,

Could I get a second review for this change, please?

Thanks,
Michail

On 26/01/2017 14:54, Dmitry Fazunenko wrote:

> Hi Michail,
>
> new version looks good to me.
> Thanks for addressing my comments.
>
> -- Dima
>
> On 26.01.2017 14:00, Michail Chernov wrote:
>> Hi,
>>
>> I updated the webrev according to offline coments from Dmitry Fazunenko.
>> GarbageProducerTestBase was renamed to GarbageProducerTest.
>> GcCauseTest02 and GcTest02 now uses GarbageProducerTest directly
>> without inheritance.
>>
>> http://cr.openjdk.java.net/~mchernov/8170737/webrev.02_to_01/
>> http://cr.openjdk.java.net/~mchernov/8170737/webrev.02/
>>
>> Thanks,
>> Michail
>>
>> On 24/01/2017 17:52, Michail Chernov wrote:
>>> Hi Dima,
>>>
>>> Thanks for your response.
>>>
>>> Updated webrev:
>>> http://cr.openjdk.java.net/~mchernov/8170737/webrev.01/
>>>
>>> Tests were refactored.
>>> There were several problems:
>>> 1. Checking of jstat output consistency should not be performed
>>> during GC cycles because it can cause to additional failures. There
>>> is checking in GarbageProducerTestBase.java that during jstat
>>> execution were not any GC cycles. Jstat gets performance counters
>>> values sequentially, so GC cycles should be avoided.
>>> 2. New gen size should be limited, because there can be OOME while
>>> eating 70% of heap on some configurations.
>>> 3. Tests failed to get expected old gen occupancy because background
>>> VM activity can change real occupancy. Now tests check real heap
>>> occupancy (using MXBean) and compares it with jstat output.
>>>
>>> The testing is in progress.
>>>
>>> Thanks,
>>> Michail
>>>
>>> On 13/01/2017 19:55, Dmitry Fazunenko wrote:
>>>> Hi Michail,
>>>>
>>>> GarbageProducer tries to predict at the beginning how many objects
>>>> to allocate later to achieve target memory usage percent.
>>>> This way doesn't look like a good idea:
>>>> - objects created by libraries (during initialization) may not be
>>>> collected yet
>>>> - eatMetaspace() besides allocating metaspace consumes some heap
>>>> space, but this amount is not taken into account.
>>>>
>>>> Will it make sense to use MXBean for heap as well as for metaspace?
>>>>
>>>> Thanks,
>>>> Dima
>>>>
>>>> ps: one more nit: "GarbageProducer{" - please insert a space.
>>>>
>>>> On 13.01.2017 14:56, Michail Chernov wrote:
>>>>> Hi,
>>>>>
>>>>> Could I have a reviews for this change, please?
>>>>>
>>>>> http://cr.openjdk.java.net/~mchernov/8170737/webrev.00/
>>>>> https://bugs.openjdk.java.net/browse/JDK-8170737
>>>>>
>>>>> Test fails because Runtime.maxMemory() results depends on the heap
>>>>> size shrinking. So need to calculate the expected heap occupation
>>>>> before a garbage allocation. Tests were updated accordingly. Also
>>>>> GcProvokerImpl.java was removed because it was not removed on
>>>>> previous commit by mistake.
>>>>> Tested via RBT.
>>>>>
>>>>> Thanks,
>>>>> Michail
>>>>
>>>>
>>>
>>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170737: Not enough old space utilisation

Thomas Schatzl
Hi Michail,

On Fri, 2017-02-03 at 13:10 +0300, Michail Chernov wrote:
> Hello,
>
> Could I get a second review for this change, please?
>

  looks good as far as I can see.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8170737: Not enough old space utilisation

Michail Chernov
Hi Thomas,

Thank you for reviewing!

Michail

On 03/02/2017 13:23, Thomas Schatzl wrote:

> Hi Michail,
>
> On Fri, 2017-02-03 at 13:10 +0300, Michail Chernov wrote:
>> Hello,
>>
>> Could I get a second review for this change, please?
>>
>    looks good as far as I can see.
>
> Thanks,
>    Thomas
>
>

Loading...