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

[Andrey Zakharov]

Fixed according comments.

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

hs-gc webrev:
http://cr.openjdk.java.net/~azakharov/8061715/webrev.06/hs-gc/

testing: in progress

Thanks.

02.03.2015 17:57, Thomas Schatzl пишет:
> Hi Andrey,
>
> On Thu, 2015-02-26 at 20:55 +0300, Andrey Zakharov wrote:
>> Updated test with
>>    - updated 2 spaces for c++ code and code style
>>    - updated copyrights
>>    - renamed variable hotCardTableSize
>>    - added output for current page size
>>
>> webrev:
>> http://cr.openjdk.java.net/~azakharov/8061715/webrev.05/
> - in HeapRegionManager::get_auxiliary_data_memory_usage(), please avoid
> declaring and initializing using multiple variables in a single
> statement, particularly if these statements span multiple lines.
>
> While coding guidelines do not explicitly prohibit this, it is easier to
> read if you repeat the type of the variable.
>
> Also, it would be easier to read, if there were an additional line
> between the return statement and the declaration of committed_sz. The
> existing code also spells out "sz".
>
> - the method bodies in the G1 code use an indentation of four spaces.
> Hotspot code uses two space.
>
> - not sure about this, but the changed tests do not add 8061715 to their
> @bug tag. It will be impossible to test particularly 8061715 then
> without finding the original bug number.
>
> - the webrev is missing the changes to the WhiteBox.java file at
> <top-level>/test/lib/sun/hotspot/WhiteBox.java. Please generate an extra
> webrev for that repo.
>
>> testing: yes (sthjprt 2015-02-26-172927.gtee.auxdata - I will restart
>> solaris on complete)
>>
>> Thanks.
>>
>>>>> - in TestShrinkAuxiliaryData.java, line 107 seems to be debugging code.
>>>>> Afaik jtreg already automatically puts all output into the log files.
>> This is why this code here: OutputAnalyzer with ProcessBuilder doesn't
>> implicitly output anything.
> Many other tests do not either (can be easily added for debugging), but
> if so, I recommend putting the System.out.println() statement before the
> output.shouldHaveExitValue(0); line. Otherwise the test will exit before
> printing the messages if there is some VM error.
>
>>>>> - 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.
>> Fixed
>>>>> - 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 dont want to run this test on box where page size is larger than
>> auxiliary data size for region, where decommitment wouldn't occurs for sure.
> Okay. But then the comment is wrong: it states "if auxdata size will be
> more than page size it would not decommit" which is the reverse of what
> you said above.
>
> I also think the comparison below should be a ">=" and not only ">"
> then.
>
> The skip message could be improved to mention "Skipping test because the
> auxiliary data is smaller than page size %d" to make the reason more
> clear.
>
> Thanks,
>    Thomas
>
>
>