<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<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">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<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>
</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>
</body>
</html>