RFR (XS): 8025605: G1: Heap expansion logging misleading for fully expanded heap

[Bengt Rutisson]

On 9/28/13 11:40 AM, Jesper Wilhelmsson wrote:
> Bengt,
>
> I agree with Thomas comments and I have another question.
>
> VirtualSpace::expand_by() seems to fail if (uncommitted_size() < 
> bytes) while your check only prints the new message if 
> uncommitted_size == 0. Given a good motivation why this is correct I 
> think your change is fine :-)

The reason is stated in a comment in G1CollectedHeap::expand(): "because 
we ran out of swap".

I think it is fine to log a "failed" message for that case, but not when 
we don't expand because we have reached -Xmx.

Bengt

> /Jesper
>
>
> Thomas Schatzl skrev 28/9/13 9:45 AM:
>> Hi Bengt,
>>
>> On Fri, 2013-09-27 at 21:57 +0200, Bengt Rutisson wrote:
>>>
>>> Hi everyone,
>>>
>>> Could I have a couple of reviews for this small change?
>>>
>>> http://cr.openjdk.java.net/~brutisso/8025605/webrev.00/
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8025605
>>>
>>> Background:
>>>
>>> When running G1 with -XX:+PrintAdaptiveSizePolicy some information
>>> about the heap sizing is logged. When the heap is fully expanded but
>>> the ergonomics would have liked to expand even more the following
>>> message is logged:
>>>
>>> [G1Ergonomics (Heap Sizing) did not expand the heap, reason: heap
>>> expansion operation failed]
>>>
>>> It sounds like something failed, but in reality we just hit the -Xmx
>>> value. We should have a more informative message that does not look
>>> like a serious failure.
>>>
>>> With my suggested patch the message will instead be:
>>>
>>> [G1Ergonomics (Heap Sizing) did not expand the heap, reason: heap
>>> already fully expanded]
>>>
>>
>> I would somewhat actually prefer if the message were moved after the
>> message printed in line 1787. Otherwise this message would be different
>> to others if verbosity is high enough. I.e. missing the "expand the
>> heap" message with the two sizes message.
>>
>> Also, could you remove the assignment to the local variable
>> old_mem_size? I think the variable is unused. (As a bonus you could
>> remove the same assignment to a local "old_mem_size" variable in
>> shrink_helper too :)
>>
>> Thanks,
>>    Thomas
>>
>>
>>