<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi.<br>
Here is updated webrevs<br>
<span style="font-weight: bold;color: #cc0000;"> </span><a
href="http://cr.openjdk.java.net/%7Eiignatyev/anzakharov/8037924/webrev.14/">http://cr.openjdk.java.net/~iignatyev/anzakharov/8037924/webrev.14/</a><br>
<a
href="http://cr.openjdk.java.net/%7Eiignatyev/anzakharov/8037925/webrev.07/">http://cr.openjdk.java.net/~iignatyev/anzakharov/8037925/webrev.07/</a><br>
<br>
Thanks.<br>
<br>
<br>
<div class="moz-cite-prefix">On 10.04.2014 16:13, Erik Helin wrote:<br>
</div>
<blockquote cite="mid:53468AD3.8020908@oracle.com" type="cite">Given
that the issue for cleaning up these tests exist, is assigned and
is already being worked on, I think this patch is ok.
<br>
<br>
Erik
<br>
<br>
On 2014-04-10 14:09, Erik Helin wrote:
<br>
<blockquote type="cite">Ok,
<br>
<br>
here is a new patch made by Andrey:
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~ehelin/8037924/webrev.13/">http://cr.openjdk.java.net/~ehelin/8037924/webrev.13/</a>
<br>
<br>
The new patch is now only for JDK-8037924, this patch does not
fix
<br>
JDK-8037925 (a separate patch will be sent out for that).
<br>
<br>
This patch makes no changes to the testlibrary, it only
introduces the
<br>
new test. Please note that there already an issue filed for
cleaning up
<br>
some of the duplication in these tests:
<br>
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8039489">https://bugs.openjdk.java.net/browse/JDK-8039489</a>
<br>
<br>
Thanks,
<br>
Erik
<br>
<br>
On 2014-04-03 16:01, Erik Helin wrote:
<br>
<blockquote type="cite">Andrey, Igor,
<br>
<br>
maybe we should have a chat session on IM where can discuss
this?
<br>
<br>
Thanks,
<br>
Erik
<br>
<br>
On 2014-04-03 15:57, Erik Helin wrote:
<br>
<blockquote type="cite">Andrey,
<br>
<br>
is all the changes in this webrev part of the patch? The
files in this
<br>
change looks very similar to the files in
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jwilhelm/8037925/webrev.04/">http://cr.openjdk.java.net/~jwilhelm/8037925/webrev.04/</a> ?
<br>
<br>
Thanks,
<br>
Erik
<br>
<br>
On 2014-04-02 19:46, Jesper Wilhelmsson wrote:
<br>
<blockquote type="cite">New webrev is uploaded here:
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.12/">http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.12/</a>
<br>
<br>
/Jesper
<br>
<br>
Andrey Zakharov skrev 2/4/14 19:17:
<br>
<blockquote type="cite">Hi, here is not uploaded yet
webrev with totally reverted inheritance.
<br>
Jesper, can you please upload to your space.
<br>
Thanks.
<br>
<br>
<br>
On 02.04.2014 20:23, Igor Ignatyev wrote:
<br>
<blockquote type="cite">Andrey,
<br>
<br>
Why do test classes extend TestShrinkHeap?
<br>
<br>
Igor
<br>
<br>
On 04/02/2014 08:09 PM, Andrey Zakharov wrote:
<br>
<blockquote type="cite">Here is version without hidden
testflow:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.11/">http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.11/</a>
<br>
Thanks.
<br>
<br>
On 01.04.2014 16:25, Jesper Wilhelmsson wrote:
<br>
<blockquote type="cite">Uploaded:
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.10/">http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.10/</a>
<br>
<br>
In general would agree regarding avoiding
duplicated code, but
<br>
when
<br>
it comes to the control flow I agree with Erik.
More hard to read
<br>
code
<br>
leads to harder maintenance than duplicating a few
lines of code.
<br>
/Jesper
<br>
<br>
Andrey Zakharov skrev 1/4/14 11:05:
<br>
<blockquote type="cite">Hi, Erik. Thanks for
comments.
<br>
<br>
On 01.04.2014 10:49, Erik Helin wrote:
<br>
<blockquote type="cite">Andrey,
<br>
<br>
a couple of comments:
<br>
- We do not use the @author tag in the tests
<br>
(if you've seen other tests have the @author
tag, then that is
<br>
a bug)
<br>
</blockquote>
removed
<br>
<blockquote type="cite">- I'm not a big fan of
using inheritance for sharing code between
<br>
tests because it makes it very hard to open
a test, e.g.
<br>
TestHumongousShrinkHeap.java, and see what
it does.
<br>
</blockquote>
Agreed, I'm too. But doubled code leads to
harder maintenance.
<br>
<blockquote type="cite">
<br>
The code you share in TestShrinkHeap is
basically the check and
<br>
the
<br>
control flow of the tests. I would prefer if
you could move the
<br>
control flow (System.gc(), sample, eat,
sample, free, check
<br>
samples)
<br>
into the tests and then write helper
functions for retrieving
<br>
the
<br>
flag values.
<br>
<br>
As for the check, please use the assertions
in
<br>
testlibrary/com/oracle/java/testlibrary/Asserts.java
instead of
<br>
throwing a RuntimeException.
<br>
</blockquote>
Good point, changed.
<br>
Jesper, can we upload webrev.10 from attachment?
Thanks.
<br>
I'm still need GC reviewers to approve push.
<br>
<br>
Thanks.
<br>
<blockquote type="cite">
<br>
Thanks,
<br>
Erik
<br>
<br>
On 2014-03-31 13:46, Jesper Wilhelmsson wrote:
<br>
<blockquote type="cite">Hi,
<br>
<br>
New webrev uploaded.
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.09/">http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.09/</a>
<br>
<br>
I'm not a Reviewer either so if you got
Igor's blessing
<br>
already, my
<br>
review won't be enough to push
unfortunately.
<br>
/Jesper
<br>
<br>
Andrey Zakharov skrev 31/3/14 12:15:
<br>
<blockquote type="cite">Hi,
<br>
Jepser, here is updated webrev.09
<br>
Thomas, Jesper can you review it as well?
<br>
Thanks.
<br>
<br>
On 31.03.2014 13:16, Igor Ignatyev wrote:
<br>
<blockquote type="cite">Andrey,
<br>
<br>
1. TEST.groups:
<br>
please update copyright year:
<br>
<blockquote type="cite"> 2 # Copyright
(c) 2013, Oracle and/or its
affiliates. All
<br>
rights
<br>
reserved.
<br>
</blockquote>
should be
<br>
<blockquote type="cite"> 2 # Copyright
(c) 2013, 2014, Oracle and/or its
affiliates.
<br>
All
<br>
rights
<br>
reserved.
<br>
</blockquote>
<br>
2. TestShrinkHeap.java
<br>
<blockquote type="cite"> 22 */
<br>
23 package
com.oracle.java.testlibrary.gc;
<br>
</blockquote>
add empty line between 22 and 23 as you
do in all other files,
<br>
<br>
otherwise it looks good to me.
<br>
<br>
Keep in mind I'm not a Reviewer, but I
can be mentioned in
<br>
'Reviewed-by'
<br>
section )
<br>
<br>
Thanks,
<br>
Igor
<br>
<br>
On 03/31/2014 10:33 AM, Andrey Zakharov
wrote:
<br>
<blockquote type="cite">Hi, team
<br>
webrev:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.08/">http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.08/</a>
<br>
bug:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8037924">https://bugs.openjdk.java.net/browse/JDK-8037924</a>
<br>
bug:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8037925">https://bugs.openjdk.java.net/browse/JDK-8037925</a>
<br>
<br>
Thanks
<br>
<br>
On 30.03.2014 22:06, Jesper
Wilhelmsson wrote:
<br>
<blockquote type="cite">Webrev
uploaded:
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.08/">http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.08/</a>
<br>
<br>
/Jesper
<br>
</blockquote>
Thank you, Jesper.
<br>
<br>
<blockquote type="cite">
<br>
Andrey Zakharov skrev 30/3/14 10:03:
<br>
<blockquote type="cite">Hi! Here is
webrev.08,
<br>
Jesper, can you upload it? Thanks!
<br>
<br>
Igor, thanks for well detailed
review!
<br>
<br>
On 30.03.2014 02:33, Igor Ignatyev
wrote:
<br>
<blockquote type="cite">Andrey,
<br>
<br>
1. both tests:
<br>
<blockquote type="cite"> 48
public void eat() {
<br>
61 public void free() {
<br>
</blockquote>
change visibility of concrete
methods to protected
<br>
2. TestHumongousShrinkHeap
<br>
<blockquote type="cite"> 32 */
<br>
33 import
<br>
com.oracle.java.testlibrary.gc.MemoryUsagePrinter;
<br>
</blockquote>
add empty line between these
lines or remove empty line
<br>
#32 in
<br>
TestDynShrinkHeap
<br>
<blockquote type="cite"> 62
//do not free last one list
<br>
</blockquote>
space after '//'
<br>
3. both tests:
<br>
<blockquote type="cite"> 26 *
@bug 8037924
<br>
26 * @bug 8037925
<br>
</blockquote>
after @bug you should specify
product bug ids which can be
<br>
tested by
<br>
this
<br>
test. I guess 8016479 instead of
8037924, and
<br>
8036025/8037925.
<br>
<br>
otherwise it looks good, thank
you for your work and
<br>
patience )
<br>
<br>
Igor
<br>
<br>
On 03/30/2014 02:21 AM, Jesper
Wilhelmsson wrote:
<br>
<blockquote type="cite">Hi, New
webrev in place:
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.07/">http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.07/</a>
<br>
<br>
/Jesper
<br>
<br>
Andrey Zakharov skrev 29/3/14
20:57:
<br>
<blockquote type="cite">Hi,
here updated webrev.
<br>
Jesper, can you, please,
upload it to your webspace.
<br>
Igor, please, see comment
below, everything
<br>
uncommented is
<br>
done.
<br>
Thanks.
<br>
<br>
On 29.03.2014 20:08, Igor
Ignatyev wrote:
<br>
<blockquote type="cite">why
isn't
'TestDynShrinkHeap.gc()'
<br>
'TestShrinkHeap.gc()'?
<br>
</blockquote>
I cut this out.
<br>
<blockquote type="cite">5.
if you need assured
fullGC, we have WhiteBox
API for
<br>
it --
<br>
s.h.WhiteBox.fullGC()
<br>
<br>
</blockquote>
Not sure is it applicable
for me, because every time
in
<br>
feature
<br>
ticket
<br>
descriptions I see
"System.gc()", so... let it
be.
<br>
<blockquote type="cite">Thanks
<br>
Igor
<br>
<br>
On 03/29/2014 07:37 PM,
Andrey Zakharov wrote:
<br>
<blockquote type="cite">Hi,
team!
<br>
webrev:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.06/">http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.06/</a>
<br>
<br>
bug:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8037924">https://bugs.openjdk.java.net/browse/JDK-8037924</a>
<br>
bug:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8037925">https://bugs.openjdk.java.net/browse/JDK-8037925</a>
<br>
<br>
Thanks.
<br>
<br>
On 29.03.2014 16:19,
Andrey Zakharov wrote:
<br>
<blockquote type="cite">Hi,
Igor, here is just
non-uploaded
attachment to
<br>
review. It
<br>
will be
<br>
upload later.
<br>
Thanks.
<br>
<br>
On 29.03.2014 12:10,
Igor Ignatyev wrote:
<br>
<blockquote
type="cite">TestDynShrinkHeap:
<br>
<blockquote
type="cite"> 45
public static
byte[] temp;
<br>
</blockquote>
unused field
<br>
<br>
it looks like
'g1/TestHumongousShrinkHeap.java'
and
<br>
'parallelScavenge/TestDynShrinkHeap.java'
should
<br>
have one
<br>
super
<br>
class,
<br>
<br>
abstract class
TestDyn {
<br>
<br>
protected static
final String
<br>
MIN_FREE_RATIO_FLAG_NAME
=
<br>
"MinHeapFreeRatio";
<br>
protected static
final String
<br>
MAX_FREE_RATIO_FLAG_NAME
=
<br>
"MaxHeapFreeRatio";
<br>
<br>
protected abstract
void eat();
<br>
<br>
protected abstract
void free();
<br>
<br>
public final void
test() {
<br>
gc();
<br>
MemoryUsagePrinter.printMemoryUsage("init");
<br>
eat();
<br>
MemoryUsagePrinter.printMemoryUsage("eaten");
<br>
MemoryUsage muFull
=
<br>
ManagementFactory.getMemoryMXBean().getHeapMemoryUsage();
<br>
<br>
<br>
<br>
<br>
free();
<br>
MemoryUsage muFree
=
<br>
ManagementFactory.getMemoryMXBean().getHeapMemoryUsage();
<br>
<br>
<br>
<br>
<br>
if
(!(muFree.getCommitted()
<
<br>
muFull.getCommitted()))
{
<br>
throw new
RuntimeException(
<br>
String.format("committed
free heap size is
not
<br>
less than
<br>
committed full heap
size, heap hasn't
been
<br>
shrunk?%n%s =
<br>
%s%n%s =
<br>
%s",
<br>
MIN_FREE_RATIO_FLAG_NAME,
<br>
<br>
ManagementFactoryHelper.getDiagnosticMXBean().getVMOption(MIN_FREE_RATIO_FLAG_NAME).getValue(),
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
MAX_FREE_RATIO_FLAG_NAME,
<br>
<br>
ManagementFactoryHelper.getDiagnosticMXBean().getVMOption(MAX_FREE_RATIO_FLAG_NAME).getValue()
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
)
<br>
);
<br>
}
<br>
gc();
<br>
MemoryUsagePrinter.printMemoryUsage("done");
<br>
}
<br>
<br>
public static void
gc() {
<br>
MemoryUsagePrinter.printMemoryUsage("before
full
<br>
GC");
<br>
System.gc();
<br>
MemoryUsagePrinter.printMemoryUsage("after
full
<br>
GC");
<br>
}
<br>
<br>
<br>
}
<br>
<br>
<br>
<br>
Igor
<br>
<br>
On 03/29/2014 10:59
AM, Andrey Zakharov
wrote:
<br>
<blockquote
type="cite">Hi,
here is updated
webrev:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.05/">http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.05/</a>
<br>
<br>
<br>
<br>
<br>
Any comments are
welcome.
<br>
Thanks.
<br>
<br>
On 28.03.2014
19:28, Igor
Ignatyev wrote:
<br>
<blockquote
type="cite">On
03/28/2014 06:06
PM, Andrey
Zakharov wrote:
<br>
<blockquote
type="cite">
<br>
On 28.03.2014
17:48, Igor
Ignatyev
wrote:
<br>
<blockquote
type="cite">Andrey,
<br>
<br>
<blockquote
type="cite">
<blockquote
type="cite">And
as I said in
8037925:
<br>
<blockquote
type="cite">You
don't need to
use enum here
and string
<br>
constant
<br>
(Labels.*) in
<br>
TestDynShrinkHeap.
I've
mistakenly
think that
<br>
MemoryUsagePrinter
<br>
uses
<br>
String
argument to
change its own
behavior.
<br>
Sorry for
<br>
useless
<br>
extra
<br>
work.
<br>
</blockquote>
</blockquote>
</blockquote>
Please remove
class
'Labels', it's
unnecessary
<br>
and
<br>
just
<br>
decrease
<br>
readability.
<br>
</blockquote>
Ok
<br>
<blockquote
type="cite">
<br>
<blockquote
type="cite">
<blockquote
type="cite">
<blockquote
type="cite">
<blockquote
type="cite">26
* @key gc
<br>
</blockquote>
why do you
need this?
<br>
</blockquote>
Hm, used as
common
practice.
<br>
</blockquote>
</blockquote>
if you don't
understand
meaning of
'@key gc',
<br>
please don't
<br>
use
<br>
it.
<br>
</blockquote>
I think that
this is used
for test
execution - to
<br>
execute
<br>
all test
<br>
that
<br>
relates to
"gc" for
example.
<br>
</blockquote>
All test related
to "gc" are
located in
<br>
test/gc, so
<br>
we don't
<br>
need it
<br>
<blockquote
type="cite">
<blockquote
type="cite">
<br>
otherwise it
looks good.
<br>
<br>
Igor
<br>
<br>
On 03/28/2014
05:37 PM,
Andrey
Zakharov
wrote:
<br>
<blockquote
type="cite">Hi,
here is
updated
webrev:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.04/">http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.04/</a>
<br>
<br>
<br>
<br>
<br>
<br>
Thanks.
<br>
<br>
On 28.03.2014
02:29, Igor
Ignatyev
wrote:
<br>
<blockquote
type="cite">Andrey,
<br>
you've placed
the wrong
link, the
correct
<br>
one is
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.02/">http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.02/</a>.
<br>
<br>
<br>
<br>
<br>
<br>
And as I said
in 8037925:
<br>
<blockquote
type="cite">You
don't need to
use enum here
and string
<br>
constant
<br>
(Labels.*) in
<br>
TestDynShrinkHeap.
I've
mistakenly
think that
<br>
MemoryUsagePrinter
<br>
uses
<br>
String
argument to
change its own
behavior.
<br>
Sorry for
<br>
useless
<br>
extra
<br>
work.
<br>
</blockquote>
<br>
MemoryUsagePrinter:
<br>
<blockquote
type="cite">44
float
freeratio = 1f
- (float)
<br>
memusage.getUsed()
/
<br>
memusage.getCommitted();
<br>
</blockquote>
I'd prefer
'1.0f', but
it's only my
fad, it's
<br>
not
<br>
necessary to
<br>
change.
<br>
<br>
TestDynShrinkHeap:
<br>
<blockquote
type="cite">96
StringBuilder
strb = new
<br>
StringBuilder("committed
<br>
heap size
under pressure
is not less
than
<br>
committed
<br>
full heap
<br>
size,
<br>
heap hasn't
been
shrunk?");
<br>
97
<br>
strb.append(System.getProperty("line.separator"));
<br>
<br>
<br>
<br>
<br>
98
strb.append(MinFreeRatioFlagName
+ " =
<br>
" +
<br>
minHeapFreeValue);
<br>
99
<br>
strb.append(System.getProperty("line.separator"));
<br>
<br>
<br>
<br>
<br>
100
strb.append(MaxFreeRatioFlagName
+ " =
<br>
" +
<br>
maxHeapFreeValue);
<br>
101 throw new
<br>
RuntimeException(strb.toString());
<br>
</blockquote>
it can be
replaced by:
<br>
throw new
<br>
RuntimeException(String.format("committed
<br>
heap size
<br>
under
<br>
pressure is
not less than
committed full
heap
<br>
size, heap
<br>
hasn't
<br>
been
<br>
shrunk?%n%s=%d%n%s=%n",MinFreeRatioFlagName,minHeapFreeValue,MaxFreeRatioFlagName,maxHeapFreeValue));
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
but it's also
not necessary.
<br>
<br>
Thanks
<br>
Igor
<br>
<br>
On 03/27/2014
04:42 PM,
Andrey
Zakharov
wrote:
<br>
<blockquote
type="cite">Here
is updated
webrev with
string
constants:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jwilhelm/8037925/webrev.02/">http://cr.openjdk.java.net/~jwilhelm/8037925/webrev.02/</a>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
Thanks.
<br>
<br>
<br>
On 26.03.2014
20:02, Andrey
Zakharov
wrote:
<br>
<blockquote
type="cite">Test
to check that
ParallelGC
respect
<br>
dynamic
<br>
change of
<br>
MaxFreeRatio
<br>
and shrinks
heap.
<br>
webrev:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jwilhelm/8037924/webrev/">http://cr.openjdk.java.net/~jwilhelm/8037924/webrev/</a>
<br>
<br>
<br>
<br>
<br>
<br>
bug:
<br>
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8037924">https://bugs.openjdk.java.net/browse/JDK-8037924</a>
<br>
<br>
<br>
<br>
<br>
<br>
Thanks.
<br>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<br>
</body>
</html>