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

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