<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Thomas,<br>
<br>
Thanks a lot for your comment!<br>
<br>
Updated webrev<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~mchernov/8141278/webrev.01/">http://cr.openjdk.java.net/~mchernov/8141278/webrev.01/</a><br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~mchernov/8141278/webrev.01_to_00/">http://cr.openjdk.java.net/~mchernov/8141278/webrev.01_to_00/</a><br>
<br>
See comments below:<br>
<br>
<div class="moz-cite-prefix">On 14.01.2016 17:04, Thomas Schatzl
wrote:<br>
</div>
<blockquote cite="mid:1452780293.2224.71.camel@oracle.com"
type="cite"><br>
<pre wrap="">
General comments:
- would it help if the PLAB logging would print out what generation the
particular output is for? That would probably make the code easier to
understand than the current way of filtering out every second printout
(and potentially decrease the size of LogParser a lot).</pre>
</blockquote>
Yes, it would be good. Appropriate CR was created couple months ago:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8141141">https://bugs.openjdk.java.net/browse/JDK-8141141</a><br>
<blockquote cite="mid:1452780293.2224.71.camel@oracle.com"
type="cite">
<pre wrap="">
Also, unified logging should help you getting only the messages you are
interested in.</pre>
</blockquote>
New logging options are in PLABUtils.java - <tt>-Xlog:gc=debug,gc+plab=debug</tt><br>
<blockquote cite="mid:1452780293.2224.71.camel@oracle.com"
type="cite">
<pre wrap="">
</pre>
<pre wrap="">
TestPLABNoResize.java
- the @summary description should be more concise, e.g. change
@summary: "Test for PLAB allocation in Survivor and Old" to "Test that
PLABs are never resized if PLAB resizing is disabled."</pre>
</blockquote>
Summary was updated. Also TestPLABNoResize was renamed to
TestPLABPromotion. I've made it because this test is also applicable
for resizing PLAB. This test doesn't check fixed size of PLAB, but
common PLAB's behavior. So, added new test cases with
-XX:+ResizePLAB (TestPLABPromotion.java line 100+)<br>
<blockquote cite="mid:1452780293.2224.71.camel@oracle.com"
type="cite">
<pre wrap="">
- line 65: the comment does not tell what the threshold is used for.
E.g. instead of "some threshold" use "Threshold to determine whether
the correct amount of objects were promoted. This is only an
approximate threshold for these checks."</pre>
</blockquote>
Comment was updated. Threshold is so big because on some cases on
some platforms I got next log:<br>
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<pre style="color: rgb(0, 0, 0); font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; widows: 1; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; white-space: pre-wrap;"><meta http-equiv="content-type" content="text/html; charset=utf-8">Test case details:
Young gen size : 64M
Predefined PLAB size : 1024
Parallel GC buffer waste pct : 30
Chunk size : 1000
Parallel GC threads : 3
Objects are created : reachable
PLAB size is fixed: yes
Test expectations:
PLAB allocation : unexpected
Direct allocation : expected
Command line: [/export/home/aurora/CommonData/TEST_JAVA_HOME/bin/java -d64 -cp /export/home/aurora/CommonData/jtreg_dir/lib/javatest.jar:/export/home/aurora/CommonData/jtreg_dir/lib/jtreg.jar:/export/home/aurora/sandbox/results/workDir/classes/1/gc/g1/plab:/export/home/aurora/CommonData/j2se_jdk/hotspot/test/gc/g1/plab:/export/home/aurora/sandbox/results/workDir/classes/1/testlibrary:/export/home/aurora/sandbox/results/workDir/test/lib:/export/home/aurora/sandbox/results/workDir/classes/1 -Xmixed -server -Xmixed -server -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -Xlog:gc=debug,gc+plab=debug -XX:+UseG1GC -XX:G1HeapRegionSize=1m -XX:OldSize=64m -XX:-UseAdaptiveSizePolicy -XX:-UseTLAB -XX:SurvivorRatio=1 -XX:ParallelGCThreads=3 -XX:ParallelGCBufferWastePct=30 -XX:OldPLABSize=1024 -XX:YoungPLABSize=1024 -XX:-ResizePLAB -Dchunk.size=1000 -Dreachable=true -XX:NewSize=64m -XX:MaxNewSize=64m -Dmem.to.fill=33554432 gc.g1.plab.lib.AppPLABPromotion ]
Survivor PLAB allocated: 4224 Direct allocated: 33551368 Mem consumed: 33554432
Old PLAB allocated: 4224 Direct allocated: 33822640 Mem consumed: 33554432
[0.077s][info ][gc] Using G1
[5.564s][info ][gc] GC(0) Pause Full (Last ditch collection) 1M->0M(128M) (5.497s, 5.564s) 66.905ms
[6.130s][debug ][gc,plab] GC(1) (allocated = 682 wasted = 0 unused = 154 used = 528 undo_waste = 0 region_end_waste = 336 regions filled = 0 direct_allocated = 4193921 failure_used = 0 failure_waste = 0)
[6.130s][debug ][gc,plab] GC(1) (allocated = 0 wasted = 0 unused = 0 used = 0 undo_waste = 0 region_end_waste = 0 regions filled = 0 direct_allocated = 0 failure_used = 0 failure_waste = 0)
[6.143s][info ][gc ] GC(1) Pause Young (WhiteBox Initiated Young GC) 32M->32M(128M) (5.967s, 6.143s) 175.963ms
[6.558s][debug ][gc,plab] GC(2) (allocated = 0 wasted = 0 unused = 0 used = 0 undo_waste = 0 region_end_waste = 0 regions filled = 0 direct_allocated = 0 failure_used = 0 failure_waste = 0)
[6.558s][debug ][gc,plab] GC(2) (allocated = 682 wasted = 0 unused = 154 used = 528 undo_waste = 33909 region_end_waste = 336 regions filled = 0 direct_allocated = 4227830 failure_used = 0 failure_waste = 0)
[6.570s][info ][gc ] GC(2) Pause Young (WhiteBox Initiated Young GC) 32M->33M(128M) (6.144s, 6.570s) 426.536ms
<pre style="color: rgb(0, 0, 0); font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; widows: 1; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; white-space: pre-wrap;"></pre></pre>Difference for Survivor is 3064, for Old is 268208. Does it mean that PLAB works incorrectly? No, it means that test should handle this 'special case'. This is the reason, why threshold is so big.
<blockquote cite="mid:1452780293.2224.71.camel@oracle.com" type="cite"><pre wrap="">
Actually I am not sure whether it is useful to all these checks with
the threshold, because it seems to be very large.</pre></blockquote>This test doesn't check that VM promotes exactly the same amount of memory from on gen to other, so at my point of view using of threshold here is good way to solve some potential problems which this test can cause in nightly testing.
<blockquote cite="mid:1452780293.2224.71.camel@oracle.com" type="cite"><pre wrap="">
- line 144: I think "No dead object should be promoted." reads better
</pre></blockquote>Fixed.
<blockquote cite="mid:1452780293.2224.71.camel@oracle.com" type="cite"><pre wrap="">
- line 145+: if we really need to check the correct amount of promoted
bytes (not sure if this is useful here), also check that the sums of
plabAllocated and directAllocated are below that threshold.
</pre></blockquote>Added, see TestPLABPromotion 188+.
<blockquote cite="mid:1452780293.2224.71.camel@oracle.com" type="cite"><pre wrap="">
- maybe the TestCase class could (but possibly should not?) be merged
in some way that the TestCase for PLABResize seems to be a part of the
TestCase for PLABNoResize</pre></blockquote>Don't think that there are two similar cases. It can be one class, but it will have a lot of extra unnecessary code for both cases.
<blockquote cite="mid:1452780293.2224.71.camel@oracle.com" type="cite">
<pre wrap="">
The Storage class ...
This class will not store than a given number of objects...
If its capacity is exceeded, old objects will be overwritten.
E.g. if capacity is one, only the last object added will be saved.
</pre></blockquote>Storage was renamed to MemoryConsumer.
Also comments were fixed.
Thanks
Michail
</body></html>