<html>
  <head>
    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    Hi Bengt, <br>
    <br>
    Thanks a lot for your detailed feedback, we appreciate it very much!<br>
    <br>
    See comments inline.<br>
    <br>
    <div class="moz-cite-prefix">On 31.10.2014 1:09, Bengt Rutisson
      wrote:<br>
    </div>
    <blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite">
      <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
      <div class="moz-cite-prefix"><br>
        Hi Evgeniya,<br>
        <br>
        On 10/30/14 3:05 PM, Evgeniya Stepanova wrote:<br>
      </div>
      <blockquote cite="mid:545245C5.4050504@oracle.com" type="cite">
        <meta http-equiv="content-type" content="text/html;
          charset=UTF-8">
        Hi,<br>
        <br>
        Please review changes for 8062537, the OpenJDK/hotspot part of
        the <a moz-do-not-send="true" id="key-val" rel="4684019"
          href="https://bugs.openjdk.java.net/browse/JDK-8019361">JDK-8019361</a><br>
        <br>
        bug: <a moz-do-not-send="true" class="moz-txt-link-freetext"
          href="https://bugs.openjdk.java.net/browse/JDK-8062537">https://bugs.openjdk.java.net/browse/JDK-8062537</a><br>
        fix: <a moz-do-not-send="true" class="moz-txt-link-freetext"
          href="http://cr.openjdk.java.net/%7Eeistepan/8062537/webrev.00/">http://cr.openjdk.java.net/~eistepan/8062537/webrev.00/</a><br>
        <br>
        Problem: Some tests explicitly set GC and fail when jtreg set
        another GC.<br>
        Solution: Such tests marked with the jtreg tag "requires" to
        skip test if there is a conflict<br>
      </blockquote>
      <br>
      Thanks for fixing this! It is really great that we finally start
      sorting this out.<br>
      <br>
      First a general comment. The @requires tag has been developed
      without much cooperation with the GC team. We did have a lot of
      feedback when it was first presented a year ago, but it does not
      seem like this feedback was incorporated into the @requires that
      was eventually built.<br>
    </blockquote>
    <br>
    We tried to implement as much developer's wishes as possible. But
    not everything is possible, sorry for that.<br>
    <br>
    <blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite"> <br>
      I think this change that gets proposed now is a big step forward
      and I won't object to it. But I am pretty convinced that we will
      soon run in to the limitations of the current @requires
      implementation and we will have to redo this work.<br>
      <br>
      Some of the points I don't really like about the @requires tag
      are:<br>
      <br>
      - the "vm.gc" abstraction is more limiting than helping. It would
      have been better to just "require" any command line flag.<br>
    </blockquote>
    "vm.gc" is an alias to a very popular flag. It's also possible to
    use: <br>
    vm.opt.UseG1GC == true instead.<br>
    <br>
    The table with all vars available in jtreg:<br>
<a class="moz-txt-link-freetext" href="http://jre.us.oracle.com/java/re/jtreg/4.1/promoted/latest/binaries/jtreg/doc/jtreg/tag-spec.html#requires_names">http://jre.us.oracle.com/java/re/jtreg/4.1/promoted/latest/binaries/jtreg/doc/jtreg/tag-spec.html#requires_names</a><br>
    <br>
    <blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite"> -
      the requirement should be per @run tag. Right now we have to do
      what you did in this change and use vm.gc=null even when some
      tests could actually have been run when a GC was specified.<br>
    </blockquote>
    it would be great, but it will unlikely happen in jtreg, as well as
    test case support.<br>
    <br>
    <blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite"> -
      there are many tests that require more than just a specific GC.
      Often there are other flags that can't be changed either for the
      test to work properly.<br>
    </blockquote>
    <br>
    yes. conflicting GC is just the most popular problem caused by
    conflicting options.<br>
    If we address this issue and we are satisfied with solution, we
    could move further.<br>
    <br>
    <blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite"> <br>
      Maybe this is not the right place to discuss the current
      implementation of the @requires tag. I just want to say that I'm
      not too happy about how the @requires tag turned out. But assuming
      we have to use it the way it is now I guess the proposed changeset
      looks good.<br>
    </blockquote>
    <br>
    yes, this thread is about change made by Evgeniya, not about jtreg
    :)<br>
    And thanks for reviewing it!<br>
    <br>
    <blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite"> <br>
      <blockquote cite="mid:545245C5.4050504@oracle.com" type="cite">
        Tested locally with different GC flags (-XX:+UseG1GC,
        -XX:+UseParallelGC, -XX:+UseSerialGC, -XX:+UseConcMarkSweep and
        without any GC flag). Tests are being excluded as expected. No
        tests failed because of the conflict.<br>
      </blockquote>
      Have you tested with -Xconcgc too? It's an alias for
      -XX:+UseConcMarkSweepGC.<br>
    </blockquote>
    <br>
    '-Xconcgc' is not supported yet. (bug in jtreg, I will submit)<br>
    <br>
    <blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite"> <br>
      I think some of the test, like
      test/gc/startup_warnings/TestDefNewCMS.java, will fail if you run
      with -XX:+UseParNewGC. Others, like 
      test/gc/startup_warnings/TestParNewCMS.java, will fail if you run
      with -XX:-UseParNewGC. Could you test these two cases too?<br>
    </blockquote>
    <br>
    These two tests ignore vm flags. <br>
    Add @requires here is not necessary, but it will allow not execute
    the tests when not needed.<br>
    So, if we run HS tests with 4 GC, we don't need to run these tests 4
    times, 1 should be enough.<br>
    <br>
    <br>
    <blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite"> <br>
      Similarly it looks to me like there are tests that will fail if
      you run them with
      <meta http-equiv="content-type" content="text/html; charset=UTF-8">
      -XX:-UseParallelOldGC or -XX:+UseParallelOldGC.<br>
    </blockquote>
    <blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite"> <br>
      Just a heads up. These two tests will soon be removed. I'm about
      to push a changeset that removes them:<br>
      <br>
      <meta http-equiv="content-type" content="text/html; charset=UTF-8">
      test/gc/startup_warnings/TestCMSIncrementalMode.java<br>
      test/gc/startup_warnings/TestCMSNoIncrementalMode.java<br>
    </blockquote>
    okay, thank for letting us know.<br>
    <br>
    <blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite"> <br>
      Is there some way of making sure that all tests are run at one
      time or another. With this change there is a risk that some tests
      are never run and always skipped. Will we somehow be tracking what
      gets skipped and make sure that all tests are at least run once
      with the correct GC so that it is not skipped all the time?<br>
    </blockquote>
    <br>
    This is a very good question! <br>
    jtreg now doesn't report skipped tests, hopefully it will do soon,
    after getting fix of:<br>
    <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/CODETOOLS-7900934">https://bugs.openjdk.java.net/browse/CODETOOLS-7900934</a><br>
    <br>
    And yes, tracking tests which are not run is important thing. <br>
    @requires - is not the only to exclude test from execution.<br>
    <br>
    Other examples:<br>
    <br>
    /*<br>
      *@ignore<br>
      *@test<br>
      */<br>
    ...<br>
    <br>
    /*@bug 4445555<br>
      *@test<br>
      */<br>
    ...<br>
    Such tests will never be run, because jtreg treats as test only
    files with @test on the first place...<br>
    <br>
    So,  making sure that tests do not disappear is important SQE task,
    we know about that, we're thinking on solution (may be very
    actively).  But this subject for another discussion, not within RFR
    :)<br>
    <br>
    Thanks,<br>
    Dima<br>
    <br>
    <br>
    <br>
    <blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite"> <br>
      Thanks,<br>
      Bengt<br>
      <br>
      <blockquote cite="mid:545245C5.4050504@oracle.com" type="cite"> <br>
        Thanks,<br>
        Evgeniya Stepanova
        <div class="moz-signature"><br>
        </div>
      </blockquote>
      <br>
    </blockquote>
    <br>
  </body>
</html>