<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>