<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<tt>Hi Bengt,<br>
<br>
<br>
</tt>
<div class="moz-cite-prefix">On 24.11.2014 18:33, Bengt Rutisson
wrote:<br>
</div>
<blockquote cite="mid:547341A8.7060108@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<br>
Hi Evgeniya,<br>
<br>
<div class="moz-cite-prefix">On 2014-11-24 14:13, Evgeniya
Stepanova wrote:<br>
</div>
<blockquote cite="mid:54732F0A.3020806@oracle.com" type="cite">
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
Hi,<br>
<div class="moz-forward-container"> <br>
Could you please review changes for 8065749 ?<br>
Test TestG1HeapRegionSize.java is G1-specific and could not
work without -XX:+UseG1GC passed directly.<br>
Option added to the run.<br>
bug: <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8065749">https://bugs.openjdk.java.net/browse/JDK-8065749</a><br>
fix: <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eeistepan/8065749/webrev.00/">http://cr.openjdk.java.net/~eistepan/8065749/webrev.00/</a><br>
</div>
</blockquote>
<br>
I think your suggested fix will work, but I have a question
regarding the original change.<br>
<br>
Before the original change the test explicitly checked that it was
run with G1 and just skipped otherwise:<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://hg.openjdk.java.net/jdk9/hs-gc/hotspot/comparison/6464714dd742/test/gc/arguments/TestG1HeapRegionSize.java">http://hg.openjdk.java.net/jdk9/hs-gc/hotspot/comparison/6464714dd742/test/gc/arguments/TestG1HeapRegionSize.java</a><br>
<br>
Was this test really failing at all? I thought we were only going
to add @requires to tests that were failing due to conflicting GC
combinations. If it was working before I think a different
approach to fix the current problem would be to revert back to the
old code without @requires.<br>
</blockquote>
<br>
You are right, it was our mistake. This test wasn't reverted when we
decided to update only failing tests.<br>
Yes, a possible solution would be just to roll back the previous
change, but I don't think it makes sense just to roll back.<br>
<br>
What I suggest:<br>
1) keep this test G1 specific (@requires g1 and +UseG1 explicitly) -
what is proposed by this change<br>
2) make tests GC independent - remove @requires and check in tests
that RegionSize == <expected> in case of G1, and RegionSize ==
0 in case of another GC.<br>
<br>
I don't think that original variant (silently pass if not g1) is the
right solution.<br>
<br>
What do you think?<br>
<br>
-- Dima<br>
<br>
<br>
<br>
<br>
<br>
<blockquote cite="mid:547341A8.7060108@oracle.com" type="cite"> <br>
Thanks,<br>
Bengt<br>
<br>
<blockquote cite="mid:54732F0A.3020806@oracle.com" type="cite">
<div class="moz-forward-container"> <br>
Thanks,<br>
Evgeniya Stepanova<br>
<br>
-- <br>
<div class="moz-signature"> <i>Evgeniya Stepanova</i></div>
<br>
</div>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>