<html>
  <head>
    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    Hi everyone!<br>
    <br>
    Since the decision was made to change only tests that fail because
    of conflict for now (skip "selfish" tests), I post new webrev for
    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>
    <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~avstepan/eistepan/8062537/webrev.01/">http://cr.openjdk.java.net/~avstepan/eistepan/8062537/webrev.01/</a><br>
    <br>
    Thanks,<br>
    Evgeniya Stepanova
    <div class="moz-cite-prefix">On 04.11.2014 15:32, Dmitry Fazunenko
      wrote:<br>
    </div>
    <blockquote cite="mid:5458B960.8000305@oracle.com" type="cite">
      <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
      Nice plan! Please feel free to send me any feedback/questions
      regarding @requires<br>
      <br>
      Thanks,<br>
      Dima <br>
      <br>
      <br>
      <div class="moz-cite-prefix">On 04.11.2014 11:40, Bengt Rutisson
        wrote:<br>
      </div>
      <blockquote cite="mid:5458910B.2070100@oracle.com" type="cite">
        <meta content="text/html; charset=UTF-8"
          http-equiv="Content-Type">
        <div class="moz-cite-prefix"><br>
          Hi Dima,<br>
          <br>
          Thanks for the answers. I think the currently proposed patch
          is a good start. We will have to evolve the @requires tag in
          the future, but let's have that discussion separate from this
          review. And we can start that discussion later when we have
          more experience with the current version of @requires.<br>
          <br>
          Thanks for doing this!<br>
          Bengt<br>
          <br>
          <br>
          <br>
          On 11/3/14 10:12 PM, Dmitry Fazunenko wrote:<br>
        </div>
        <blockquote cite="mid:5457EFA6.7050404@oracle.com" type="cite">
          <meta content="text/html; charset=UTF-8"
            http-equiv="Content-Type">
          Hi Bengt,<br>
          <br>
          That's great that we have very closed visions! <br>
          <br>
          The general comment: currently, jtreg doesn't support any sort
          of plugins, so you can't provide a VM specific handler of the
          @requires or another tag. This is very annoying limitation and
          we have to live with it.<br>
          <br>
          A few more comments inline.<br>
          <br>
          <br>
          <div class="moz-cite-prefix">On 03.11.2014 16:31, Bengt
            Rutisson wrote:<br>
          </div>
          <blockquote cite="mid:545783A1.3050300@oracle.com" type="cite">
            <meta content="text/html; charset=UTF-8"
              http-equiv="Content-Type">
            <div class="moz-cite-prefix"><br>
              <br>
              Hi Dima,<br>
              <br>
              Answers inline.<br>
              <br>
              On 10/31/14 1:56 PM, Dmitry Fazunenko wrote:<br>
            </div>
            <blockquote cite="mid:545386E1.2050402@oracle.com"
              type="cite">
              <meta content="text/html; charset=UTF-8"
                http-equiv="Content-Type">
              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>
            </blockquote>
            <br>
            Yes, I'm sure you have done your best. It's just that we
            have been requesting this feature for 3 years and I was
            expecting us to be able to influence the feature much more
            than was the case now.<br>
          </blockquote>
          <br>
          My personal hope: @requires will address ~90% of existing
          issues.<br>
          <br>
          <blockquote cite="mid:545783A1.3050300@oracle.com" type="cite">
            <br>
            <blockquote cite="mid:545386E1.2050402@oracle.com"
              type="cite"> <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 moz-do-not-send="true" 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>
            </blockquote>
            <br>
            The problem with having this matching built in to JTreg is
            that it makes it very hard to change. When we discussed this
            a year ago I think we said that JTreg should only provide a
            means to test against the command line and a hook for
            running some java code in the @requires tag. That way we
            could put logic like this in a test library that is under
            our control. This would make it easy for us to change and
            also enables us to use different logic for different
            versions.<br>
          </blockquote>
          <br>
          I would be glad to have own harness...<br>
          <br>
          <blockquote cite="mid:545783A1.3050300@oracle.com" type="cite">
            <br>
            <blockquote cite="mid:545386E1.2050402@oracle.com"
              type="cite"> <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>
            </blockquote>
            <br>
            what do you mean with test case support? Hi Evgeniya,</blockquote>
          <br>
          Under test case support I mean ability to treat each @run as a
          separate test. Now<br>
          <br>
          @test<br>
          @run -XX:g1RegSize=1m MyTest <br>
          @run -XX:g1RegSize=2m MyTest<br>
          @run -XX:g1RegSize=4m MyTest<br>
          class MyTest {<br>
          }<br>
          <br>
          is always a single test. You can't exclude, or re-run a part
          of it.<br>
          <br>
          <br>
          <blockquote cite="mid:545783A1.3050300@oracle.com" type="cite">
            <br>
            <blockquote cite="mid:545386E1.2050402@oracle.com"
              type="cite"> <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>
            </blockquote>
            <br>
            Yes, I agree that taking one step at the time is good.
            Personally I would have preferred that the first step was a
            "just run the command line as specified in the @run tag"
            step.<br>
            <br>
            <blockquote cite="mid:545386E1.2050402@oracle.com"
              type="cite"> <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>
            </blockquote>
            <br>
            Agreed. And as I said, I think the patch looks ok. I have
            not looked at all tests. But if they now pass with the
            combinations that we test with I guess they should be ok.<br>
          </blockquote>
          <br>
          Excellent! Thanks a lot!<br>
          <br>
          <blockquote cite="mid:545783A1.3050300@oracle.com" type="cite">
            <br>
            <blockquote cite="mid:545386E1.2050402@oracle.com"
              type="cite"> <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>
            </blockquote>
            <br>
            Ok. Thanks.<br>
            <br>
            <blockquote cite="mid:545386E1.2050402@oracle.com"
              type="cite"> <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>
            </blockquote>
            <br>
            Do we really want to use the @requires functionality for
            this purpose? It seems like a way of misusing @requires. If
            we just want the tests to be run once I think Leonid's
            approach with tests lists seems more suitable.<br>
          </blockquote>
          <br>
          No, it's not a purpose of course, it's just side effect :)<br>
          <br>
          <br>
          <blockquote cite="mid:545783A1.3050300@oracle.com" type="cite">
            But are you sure that this is the reason for the @requires
            in this case? TestDefNewCMS does sound like a test that is
            DefNew specific. I don't see a reason to run it with ParNew.
            If it doesn't fail today it should probably be changed so
            that it does fail if it is run with the wrong GC.<br>
          </blockquote>
          <br>
          @requires - is not the silver bullet, but it's quite easy way
          to solve a lot of issues.<br>
          <br>
          I hope, @requires will allow to reduce the number of "selfish"
          tests, which produce a new java process to ignore vm flags
          coming from outside. No @requires, no other mechanism could
          100% protect a test from running with conflicting options, but
          this is not the goal.<br>
          <br>
          If one runs tests with an exotic option, like a new G2
          collector, there shouldn't mass failures caused by options
          conflicts. But a few failures could be handled manually.  <br>
          <br>
          <br>
          <blockquote cite="mid:545783A1.3050300@oracle.com" type="cite">
            <br>
            <blockquote cite="mid:545386E1.2050402@oracle.com"
              type="cite"> <br>
              <blockquote cite="mid:5452A8F7.8080709@oracle.com"
                type="cite"> 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 moz-do-not-send="true" 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>
            </blockquote>
            <br>
            Right. Glad to hear that you are actively working on this!<br>
          </blockquote>
          <br>
          I was going to say "not very actively", but never mind, we
          know about this problem. With introducing @requires mechanism
          it will become more important!<br>
          <br>
          <br>
          Thanks for your comments!<br>
          <br>
          -- Dima<br>
          <br>
          <br>
          <blockquote cite="mid:545783A1.3050300@oracle.com" type="cite">
            <br>
            Bengt<br>
            <br>
            <blockquote cite="mid:545386E1.2050402@oracle.com"
              type="cite"> <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>
            </blockquote>
            <br>
          </blockquote>
          <br>
        </blockquote>
        <br>
      </blockquote>
      <br>
    </blockquote>
    <br>
    <div class="moz-signature">-- <br>
      <i>Evgeniya Stepanova</i></div>
  </body>
</html>