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

Thomas Schatzl thomas.schatzl at oracle.com
Mon Mar 2 14:57:33 UTC 2015


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






More information about the hotspot-gc-dev mailing list