[RFR] 8061715: gc/g1/TestShrinkAuxiliaryData15.java fails with java.lang.RuntimeException: heap decommit failed - after > before

[Andrey Zakharov]

Hi
I've got rid of
sum_aux_data_size and now it looks bit more massively,
also remove _heap_spacer from sum to return only aux data.

webrev:
http://cr.openjdk.java.net/~azakharov/8061715/webrev.03/

testing started on Stockholm JPRT:
2015-02-25-164030.azakharov.hs-gc-clean

Observed failure of test on Solaris with ObjectAlignmentInBytes=256. 
Investigating.
Thanks.


18.02.2015 15:24, Mikael Gerdin пишет:
> Hi,
>
> On 2015-02-18 11:16, Thomas Schatzl wrote:
>> Hi Andrey,
>>
>>    sorry for the late review...
>>
>> On Thu, 2015-02-12 at 17:00 +0300, Andrey Zakharov wrote:
>>> Hello.
>>> Summary: test for auxiliary data in G1 fails as they cannot precisely
>>> measure this aux data size.
>>> I've added whitebox method to get this size.
>>> I've on little question about placement of helper method
>>> "sum_memory_usage", which currently placed in
>>> gc_implementation/g1/heapRegionManager.hpp but it doesn't logically
>>> related to HeapManager itself.
>>> If you can advice me where its best place it will be wonderful.
>>>
>>> webrev:
>>> http://cr.openjdk.java.net/~azakharov/8061715/webrev/
>>> bug:
>>> JDK-8061715 gc/g1/TestShrinkAuxiliaryData15.java fails with
>>> java.lang.RuntimeException: heap decommit failed - after > before
>>>
>>> Testing done in Stockholm's JPRT (2015-02-12-112315.azakharov.hs-gc) 
>>> all
>>> is fine.
>>
>> - the changes in the VM look good, there is one point though: the method
>> HeapRegionManager::get_auxiliary_data_memory_usage() not only returns
>> memory information about auxiliary data structures, but includes the
>> heap.
>> If this is intentional, the method name should be fixed to not read
>> "auxiliary".
>
> The sum_memory_usage doesn't seem to belong in HeapRegionManager, it 
> doesn't touch any class members and has no other callers.
>
> If you really want to factor out the additions in a separate method 
> I'd prefer it if you put it as a static non-class method just above 
> HeapRegionManager::get_auxiliary_data_memory_usage.
> If you want to keep it, please change it to use pointers instead of 
> references, we rarely use references as output parameters since it 
> makes it hard to understand the code at the call site.
>
> A lot of the JVM code you added has 4 space indentation instead of the 
> correct 2 spaces.
>
> /Mikael
>
>
>>
>> - in the whitebox.cpp files, in WB_G1AuxiliaryMemoryUsage there seems to
>> be a superfluous newline at line 327/328.
>>
>> - please fix copyright dates
>>
>> - in TestShrinkAuxiliaryData.java, line 107 seems to be debugging code.
>> Afaik jtreg already automatically puts all output into the log files.
>>
>> - line 174, I do not understand the comment. What does "if auxdata size
>> will be more than page size it would not decommit auxiliary data size is
>> about ~3.6% of heap size" seem to be at least two sentences.
>>
>> - what do the two reasons given in the comment of
>> checkEnvApplicability() actually mean? I.e. why do you not run the test
>> if large pages are enabled?
>>
>> I think the check is wrong too, it should check if UseLargePages is
>> enabled I think. You should use the HotSpotDiagnosticMXBean's
>> getVMOption() method.
>>
>> Did you check that Whitebox.
>>
>> The actual code does not help either.
>>
>> The second check just seems to check whether
>> REGION_SIZE*REGIONS_TO_ALLOCATE is smaller than the heap size. Why not
>> specify -Xmx of 100M directly when starting the VM instead?
>>
>> - it would be nice to not call the hot card cache "RSet cache". I do not
>> know why the switch has been called G1ConcRSLogCacheSize, but it does
>> not have to do much with the remembered set (it decreases the pressure
>> on the remembered set, but it has not a lot do to with it other than
>> that).
>>
>> Thanks,
>>    Thomas
>>
>>