<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<tt>Bengt</tt><tt>, Jesper</tt><tt><br>
<br>
Thank you for the review!<br>
<br>
Evgeniya<br>
</tt>
<div class="moz-cite-prefix">On 25.11.2014 17:01, Bengt Rutisson
wrote:<br>
</div>
<blockquote cite="mid:54747DC7.7090107@oracle.com" type="cite">
<meta http-equiv="Context-Type" content="text/html; charset=utf-8">
<br>
<div class="moz-cite-prefix">On 2014-11-25 12:11, Evgeniya
Stepanova wrote:<br>
</div>
<blockquote cite="mid:547463FF.8010702@oracle.com" type="cite"> Hi
Bengt,<br>
Ok, file is reverted.<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eeistepan/8065749/webrev.01">http://cr.openjdk.java.net/~eistepan/8065749/webrev.01</a><br>
</blockquote>
<br>
Thanks, looks good.<br>
<br>
<blockquote cite="mid:547463FF.8010702@oracle.com" type="cite"> <br>
But please note that test will run only if UseG1GC set. It will
be skipped otherwise (with no GC option set for example).<br>
</blockquote>
<br>
Agreed.<br>
<br>
Bengt<br>
<br>
<blockquote cite="mid:547463FF.8010702@oracle.com" type="cite"> <br>
Thanks,<br>
Evgeniya Stepanova<br>
<br>
<br>
<div class="moz-cite-prefix">On 25.11.2014 13:12, Bengt Rutisson
wrote:<br>
</div>
<blockquote cite="mid:547447F8.5040307@oracle.com" type="cite">
<br>
Hi Dima,<br>
<br>
<div class="moz-cite-prefix">On 2014-11-24 15:03, Dmitry
Fazunenko wrote:<br>
</div>
<blockquote cite="mid:54733AAB.5020005@oracle.com" type="cite">
<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"> <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"> 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>
</blockquote>
<br>
I don't think 2) makes sense. G1HeapRegionSize is not set to 0
for other collectors and there is really no reason to do that
either. So, testing the value of that with other GCs is just a
waste of time.<br>
<br>
Considering that, I don't really see why 1) is better than
reverting to the original code. After all, we are not sure how
we want @requires to work and in my mind it is better to use
it as little as possible until we have figured out how it
should work.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<blockquote cite="mid:54733AAB.5020005@oracle.com" type="cite">
<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>
</blockquote>
<br>
</blockquote>
<br>
<div class="moz-signature">-- <br>
<i>Evgeniya Stepanova</i></div>
</blockquote>
<br>
</blockquote>
<br>
<div class="moz-signature">-- <br>
<i>Evgeniya Stepanova</i></div>
</body>
</html>