<p dir="ltr">Thanks for the fix. Looks good to me.</p>
<div class="gmail_quote">On Nov 11, 2015 11:56 PM, "Jon Masamitsu" <<a href="mailto:jon.masamitsu@oracle.com">jon.masamitsu@oracle.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  
    
  
  <div bgcolor="#FFFFFF" text="#000000">
    Jungwoo,<br>
    <br>
    I reworked the patch just a bit. <br>
    <br>
    <a href="http://cr.openjdk.java.net/~jmasa/8141356/webrev.00/" target="_blank">http://cr.openjdk.java.net/~jmasa/8141356/webrev.00/</a><br>
    <br>
    I moved the definition of stop() to the .cpp file.  The .cpp<br>
    file already had an include of concurrentMarkSweepThread.hpp<br>
    and I think we're trying to minimize the include of .hpp files<br>
    into other .hpp files.<br>
    <br>
    I also added the guard<br>
    <pre><span>#if INCLUDE_ALL_GCS

There is a build that does not include all the GC
and the guard is needed in that build.

Is this version all right with you?

Jon
</span></pre>
    <br>
    <div>On 11/8/2015 9:55 AM, Jungwoo Ha wrote:<br>
    </div>
    <blockquote type="cite">
      <div dir="ltr">Thanks for the review, Kim. --Jungwoo</div>
      <div class="gmail_extra"><br>
        <div class="gmail_quote">On Thu, Nov 5, 2015 at 10:40 PM, Kim
          Barrett <span dir="ltr"><<a href="mailto:kim.barrett@oracle.com" target="_blank">kim.barrett@oracle.com</a>></span>
          wrote:<br>
          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>On Nov 3, 2015, at 4:14 PM, Jungwoo Ha <<a href="mailto:jwha@google.com" target="_blank"><a href="mailto:jwha@google.com" target="_blank">jwha@google.com</a></a>>
              wrote:<br>
              ><br>
              > BUG: <a href="https://bugs.openjdk.java.net/browse/JDK-8141356" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8141356</a><br>
              > Webrev: <a href="http://cr.openjdk.java.net/%7Ejwha/8141356/webrev.00/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jwha/8141356/webrev.00/</a><br>
              ><br>
              > Inside before_exit(), it says<br>
              ><br>
              >   // Stop concurrent GC threads<br>
              >   Universe::heap()->stop();<br>
              ><br>
              > but GenCollectedHeap never implemented stop() method,
              and thus calling empty SharedHeap::stop() method.<br>
              > This causes CMS threads to run during the VM
              termination and ends up crashing.<br>
              > ConcurrentMarkSweepThread::stop() is currently a dead
              code and never gets called.<br>
              > The patch just implemented GenCollectedHeap::stop()
              to call ConcurrentMarkSweepThread::stop().<br>
              <br>
            </span>I was going to ask why CMS doesn’t have its own heap
            class where the implementation of stop should be placed.<br>
            But I see that CMS is already significantly entangled with
            GenCollectedHeap.  Given that, change looks good.<br>
            <br>
          </blockquote>
        </div>
        <br>
        <br clear="all">
        <div><br>
        </div>
        -- <br>
        <div>
          <div dir="ltr">
            <div><span style="font-size:12.8000001907349px">Jungwoo Ha |
                Java Platform Team | <a href="mailto:jwha@google.com" target="_blank">jwha@google.com</a></span><br>
            </div>
            <div><br>
            </div>
          </div>
        </div>
      </div>
    </blockquote>
    <br>
  </div>

</blockquote></div>