<html>
  <head>
    <meta content="text/html; charset=windows-1252"
      http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <div class="moz-cite-prefix">Thanks Per!<br>
      <br>
      New issue:
      <meta http-equiv="content-type" content="text/html;
        charset=windows-1252">
      <meta http-equiv="content-type" content="text/html;
        charset=windows-1252">
      <a class="issue-link" data-issue-key="JDK-8140600"
        href="https://bugs.openjdk.java.net/browse/JDK-8140600"
        id="key-val" rel="4851033">JDK-8140600</a> Convert unnecessarily
      malloc'd Monitors to value members<br>
      <br>
      Kim already offered to sponsor, so I'll send it his way (he can
      more easily hunt me down if any issues arise).<br>
      <br>
       - Derek<br>
      <br>
      <br>
      On 10/27/15 11:13 AM, Per Liden wrote:<br>
    </div>
    <blockquote cite="mid:562F94A0.6090704@oracle.com" type="cite">Hi
      Derek,
      <br>
      <br>
      On 2015-10-27 14:31, Oracle wrote:
      <br>
      <blockquote type="cite">Hi Per,
        <br>
        <br>
        <blockquote type="cite">On Oct 27, 2015, at 7:22 AM, Per Liden
          <a class="moz-txt-link-rfc2396E" href="mailto:per.liden@oracle.com"><per.liden@oracle.com></a> wrote:
          <br>
          <br>
          Hi Derek,
          <br>
          <br>
          <blockquote type="cite">On 2015-10-26 22:01, Derek White
            wrote:
            <br>
            Hi Per,
            <br>
            <br>
            Even more on the monitor allocation issue below...
            <br>
            <br>
            <blockquote type="cite">  On 10/26/15 12:25 PM, Derek White
              wrote:
              <br>
              Hi Per,
              <br>
              <br>
              More comments below...
              <br>
              <br>
              <blockquote type="cite">On 10/26/15 12:21 PM, Derek White
                wrote:
                <br>
                Hi Per,
                <br>
                <br>
                <blockquote type="cite">On 10/26/15 10:56 AM, Per Liden
                  wrote:
                  <br>
                  <blockquote type="cite">On 2015-10-26 15:47, Derek
                    White wrote:
                    <br>
                    Hi Per,
                    <br>
                    <br>
                    <blockquote type="cite">On 10/26/15 3:09 AM, Per
                      Liden wrote:
                      <br>
                      Hi Derek,
                      <br>
                      <br>
                      <blockquote type="cite">On 2015-10-23 19:43, Derek
                        White wrote:
                        <br>
                        RFR 5.
                        <br>
                        <br>
                        This responds to Per's last comments and
                        Thomas's suggestions for
                        <br>
                        better
                        <br>
                        methods names and comments.
                        <br>
                        <br>
                        Webrev:
                        <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/8138920/webrev.05/">http://cr.openjdk.java.net/~drwhite/8138920/webrev.05/</a>
                        <br>
                        Webrev 05 vs 04:
                        <br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/8138920/webrev.05.vs.04/">http://cr.openjdk.java.net/~drwhite/8138920/webrev.05.vs.04/</a>
                        <br>
                        <br>
                        RFE: JDK-8138920
                        <a class="moz-txt-link-rfc2396E" href="https://bugs.openjdk.java.net/browse/JDK-8138920"><https://bugs.openjdk.java.net/browse/JDK-8138920></a>
                        <br>
                        Refactor the sampling thread from
                        ConcurrentG1RefineThread
                        <br>
                      </blockquote>
                      <br>
                      The patch looks good, but it looks like you forgot
                      to address the
                      <br>
                      following two comments.
                      <br>
                    </blockquote>
                    <br>
                    Ahh, I got the Monitor name, but forgot the thread
                    name. Thanks Per!
                    <br>
                  </blockquote>
                  <br>
                  Derek, note that my first comment below about _monitor
                  is not about
                  <br>
                  its name, but about how it's instantiated, i.e. please
                  remove the
                  <br>
                  dynamic allocation. Fixing the name is also good, but
                  that was not
                  <br>
                  what my comment was about.
                  <br>
                </blockquote>
                <br>
                OK. Sorry for rushing past this.
                <br>
              </blockquote>
              So Monitor is declared as a CHeapObj. Is it possible to
              avoid dynamic
              <br>
              allocation here? Are there examples of this anywhere?
              <br>
              <br>
              - Derek
              <br>
            </blockquote>
            <br>
            After talking with Kim about CHeapObj, it sounds like using
            a
            <br>
            CHeapObj-typed object as a value object is allowed, although
            there
            <br>
            wasn't unanamous agreement that is was a good idea.
            <br>
          </blockquote>
          <br>
          Using dynamic allocation when it's not needed is just sloppy,
          IMHO
          <br>
        </blockquote>
        <br>
        Agreed. This is one of many such pre-existing sloppy
        allocations. I'm happy to dedicate my time to cleaning these up
        once I'm certain it's safe to do so.
        <br>
        <br>
        In the meantime, Kim has been waiting for the cleanup of the
        sampling thread since Thursday.
        <br>
        <br>
        <blockquote type="cite">
          <blockquote type="cite">After more detective work, it looks
            like parallel GC as well as
            <br>
            MutexGangTaskDispatcher do destroy a dynamically allocated
            Monitor. And
            <br>
            it looks like SurrogateLockerThread is the one case that
            uses a Monitor
            <br>
            as a value member.
            <br>
            <br>
            But I'm more concerned about changing the lifetime of the
            Monitor
            <br>
            object. This may be preventing dead-lock on shutdown - Some
            thread A is
            <br>
            waiting on a monitor owned by thread B, and thread B's
            destructor is
            <br>
            called, destroying the Monitor being waited on.
            <br>
            <br>
            I'd rather not tack on this change at this point. If turning
            dynamically
            <br>
            allocated Monitors into value members is a good idea, I can
            add it to
            <br>
            the technical debt list. But it's not a risk-free change, so
            we'll need
            <br>
            to come up with some schemes for validation and testing.
            <br>
          </blockquote>
          <br>
          I think you're overstating the problem here. In this case the
          sample thread (or rather the ConcurrentG1Refine instance which
          owns the sample thread) is never destroyed so you wouldn't be
          changing the life time. On the other hand, if the sample
          thread had been destroyed, but not the _monitor, then we have
          a memory leak which should be fixed.
          <br>
        </blockquote>
        <br>
        The sampling thread ownership will soon be removed from
        ConcurrentG1Refine, so we can't rely on that. And the dangling
        Monitor idiom is so frequent in our code that I'm not sure that
        it's unintentional.
        <br>
        <br>
        I agree with you that this should be cleaned up, but I think any
        fix involving Monitor lifetime changes will deserve more
        consideration*** and review, and that the fix should be applied
        to more cases across our code.
        <br>
        <br>
        Meanwhile I'd like to put this code to bed. Are you ok with
        separating these issue to s new CR?
        <br>
      </blockquote>
      <br>
      I'm ok with that. Let's get this change pushed. I can sponsor it,
      just let me know where I can find the final patch.
      <br>
      <br>
      cheers,
      <br>
      /Per
      <br>
      <br>
      <blockquote type="cite">
        <br>
        Thanks for all your work reviewing this change!
        <br>
        <br>
        - Derek
        <br>
        <br>
        <br>
        *** I want to see if there's some way to assert that a Monitor
        has no waiters before deleting it, for example.
        <br>
        <br>
        <br>
        <blockquote type="cite">cheers,
          <br>
          /Per
          <br>
          <br>
          <blockquote type="cite">
            <br>
              - Derek
            <br>
            <blockquote type="cite">
              <blockquote type="cite">
                <blockquote type="cite">cheers,
                  <br>
                  /Per
                  <br>
                  <br>
                  <blockquote type="cite">
                    <br>
                      - Derek
                    <br>
                    <blockquote type="cite">
                      <br>
                      [...]
                      <br>
                      <blockquote type="cite">
                        <blockquote type="cite">
                          <blockquote type="cite">
                            <blockquote type="cite">g1YoungListSampleThread.cpp
                              <br>
                              ---------------------------
                              <br>
                              <br>
                                59   _monitor = new
                              Monitor(Mutex::nonleaf,
                              <br>
                                60                          "Sample
                              thread monitor",
                              <br>
                                61                          true,
                              <br>
                                62 Monitor::_safepoint_check_never);
                              <br>
                              <br>
                              It looks like _monitor could be a value
                              member instead of a
                              <br>
                              pointer,
                              <br>
                              to avoid the extra allocation here.
                              <br>
                              <br>
                                66   set_name("G1 Sample");
                              <br>
                              <br>
                              This should probably be updated to reflect
                              the new name.
                              <br>
                            </blockquote>
                          </blockquote>
                        </blockquote>
                      </blockquote>
                      <br>
                      I don't need to see a new webrev, but please
                      address these before
                      <br>
                      pushing.
                      <br>
                      <br>
                      cheers,
                      <br>
                      /Per
                      <br>
                    </blockquote>
                  </blockquote>
                </blockquote>
              </blockquote>
            </blockquote>
            <br>
          </blockquote>
        </blockquote>
      </blockquote>
    </blockquote>
    <br>
  </body>
</html>