<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    <br>
    <br>
    <div class="moz-cite-prefix">On 1/29/19 4:51 AM, Thomas Schatzl
      wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:29d6bf77f5003d13cb0497288115c2a108ce9e5f.camel@oracle.com">
      <pre class="moz-quote-pre" wrap="">Hi,

On Mon, 2019-01-28 at 16:46 -0500, <a class="moz-txt-link-abbreviated" href="mailto:coleen.phillimore@oracle.com">coleen.phillimore@oracle.com</a> wrote:
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">Hi Thomas,  Can you forward the review thread for JDK-8213229?
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
  the thread starts at 
<a class="moz-txt-link-freetext" href="http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2019-January/024474.html">http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2019-January/024474.html</a>
 , but the interesting part is probably starting here 
<a class="moz-txt-link-freetext" href="http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2019-January/024649.html">http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2019-January/024649.html</a>
 .
</pre>
    </blockquote>
    <br>
    So this moves StringTable processing into the WeakProcessor.  good! 
    Not a review though.<br>
    <br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8217778/webrev/src/hotspot/share/gc/shared/weakProcessor.inline.hpp.frames.html">http://cr.openjdk.java.net/~tschatzl/8217778/webrev/src/hotspot/share/gc/shared/weakProcessor.inline.hpp.frames.html</a><br>
    <br>
    If these Counting closures are only used in the weakProcessor.cpp
    file, they should be defined there.  It looks like their use was
    moved.<br>
    <br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8217778/webrev/src/hotspot/share/gc/shared/weakProcessor.cpp.frames.html">http://cr.openjdk.java.net/~tschatzl/8217778/webrev/src/hotspot/share/gc/shared/weakProcessor.cpp.frames.html</a><br>
    <br>
    I don't know what "is_serial does but it doesn't look like you're
    using the num_dead() count when calling processor.<br>
    <br>
    <pre class="moz-quote-pre" wrap=""> 40     if (WeakProcessorPhases::is_serial(phase)) {
<a name="2" id="anc2"></a><span class="new" style="color: blue; font-weight: bold;">  41       CountingIsAliveClosure<BoolObjectClosure> cl(is_alive);</span>
  42       WeakProcessorPhases::processor(phase)(&cl, keep_alive);
  43     } else {
<a name="3" id="anc3"></a><span class="changed" style="color: blue;">  44       CountingSkippedIsAliveClosure<BoolObjectClosure, OopClosure> cl(is_alive, keep_alive);</span>
<span class="changed" style="color: blue;">  45       WeakProcessorPhases::oop_storage(phase)->oops_do(&cl);</span>
  46       if (WeakProcessorPhases::is_stringtable(phase)) {
<a name="4" id="anc4"></a><span class="changed" style="color: blue;">  47         StringTable::inc_dead_counter(cl.num_dead() + cl.num_skipped());</span>
<span class="changed" style="color: blue;">  48       }</span>
  49     }</pre>
    Thanks,<br>
    Coleen<br>
    <br>
    <blockquote type="cite"
      cite="mid:29d6bf77f5003d13cb0497288115c2a108ce9e5f.camel@oracle.com">
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">I wonder if we could just trigger_unloading after any GC has run and 
ignore dead string counts.  Are they ever zero?
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
That seems to be a waste of cpu cycles assuming that interned strings
typically have a "long" lifetime.</pre>
    </blockquote>
    <br>
    Since the VM has to intern any strings in the JVM_CONSTANT_String in
    the classfile, class unloading probably should trigger_unloading
    unconditionally if any classes are unloaded.  It seems very likely
    there would be unreferenced strings.<br>
    <br>
    But we may need the ServiceThread to unlink reclaimed entries even
    if classes aren't unloaded, which may be infrequent but there may
    have been stressful applications that have required it in the past.<br>
    <br>
    Can you log if the count is ever too low to process the StringTable?<br>
    <br>
    The change for this looks reasonable, but not a full review.  I kind
    of like keeping the concept of weak_oops_do() because it's a well
    known idiom but as Kim says, it's not necessary to call weak_oops_do
    on something that contains weak oops.<br>
    <br>
    <pre class="moz-quote-pre" wrap="">CR:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8217778">https://bugs.openjdk.java.net/browse/JDK-8217778</a>
Webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8217778/webrev/">http://cr.openjdk.java.net/~tschatzl/8217778/webrev/</a></pre>
    <br>
    Thanks,<br>
    Coleen<br>
    <blockquote type="cite"
      cite="mid:29d6bf77f5003d13cb0497288115c2a108ce9e5f.camel@oracle.com">
      <pre class="moz-quote-pre" wrap="">

Thanks,
  Thomas


</pre>
    </blockquote>
    <br>
  </body>
</html>