<div dir="ltr">I renamed the flag and the updated webrev is posted.<div><a href="http://cr.openjdk.java.net/~jmasa/8028554/webrev.02/" target="_blank" style="font-family:arial,sans-serif;font-size:13px">http://cr.openjdk.java.net/~jmasa/8028554/webrev.02/</a><br>
</div><div><br></div><div><blockquote type="cite" style="color:rgb(80,0,80);font-family:arial,sans-serif;font-size:13px"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><br>In CMSCollector there is still this code to change the value for ConcGCThreads based on AdjustGCThreadsToCores.<div><br><br> 639 if (AdjustGCThreadsToCores) {<br> 640 FLAG_SET_DEFAULT(ConcGCThreads, ParallelGCThreads / 2);<br>
641 } else {<br> 642 FLAG_SET_DEFAULT(ConcGCThreads, (3 + ParallelGCThreads) / 4);<br> 643 }<br><br></div>Do you think that is needed or can we use the same logic in both cases given that ParallelGCThreads has a different value if AdjustGCThreadsToCores is enabled.</div>
</blockquote><div><br></div></div></div></div></blockquote> For this part, I still kept the flag. Otherwise, ConcGCThreads number will be too big when the flag is off & hyperthreading is on.<br></div><div><br></div><div>
Jungwoo</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jan 16, 2014 at 5:35 AM, Bengt Rutisson <span dir="ltr"><<a href="mailto:bengt.rutisson@oracle.com" target="_blank">bengt.rutisson@oracle.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<br>
<div>On 2014-01-15 22:29, Jungwoo Ha wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">Sounds good to me.</div>
</blockquote>
<br>
Yes, I like ScaleGCThreadsByCores better too.<span class="HOEnZb"><font color="#888888"><br>
<br>
Bengt</font></span><div><div class="h5"><br>
<br>
<blockquote type="cite">
<div class="gmail_extra"><br>
<br>
<div class="gmail_quote">On Wed, Jan 15, 2014 at 10:52 AM, Jon
Masamitsu <span dir="ltr"><<a href="mailto:jon.masamitsu@oracle.com" target="_blank">jon.masamitsu@oracle.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<div>
<div> <br>
<div>On 1/15/2014 4:51 AM, Bengt Rutisson wrote:<br>
</div>
<blockquote type="cite"> <br>
<div>On 2014-01-13 22:39, Jungwoo Ha wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"> <br>
In CMSCollector there is still this code
to change the value for ConcGCThreads
based on AdjustGCThreadsToCores.
<div><br>
<br>
639 if (AdjustGCThreadsToCores)
{<br>
640
FLAG_SET_DEFAULT(ConcGCThreads,
ParallelGCThreads / 2);<br>
641 } else {<br>
642
FLAG_SET_DEFAULT(ConcGCThreads, (3 +
ParallelGCThreads) / 4);<br>
643 }<br>
<br>
</div>
Do you think that is needed or can we
use the same logic in both cases given
that ParallelGCThreads has a different
value if AdjustGCThreadsToCores is
enabled.<br>
</div>
</blockquote>
<div><br>
</div>
<div>I am happy to just use
FLAG_SET_DEFAULT(ConcGCThreads,
ParallelGCThreads / 2);<br>
The original hotspot code used
FLAG_SET_DEFAULT(ConcGCThreads, (3 +
ParallelGCThreads) / 4); which I think is
somewhat arbitrary.</div>
<div>Now that ParallelGCThreads will reduce
on some configuration, dividing it into 4
seems to make the ConcGCThreads too small.</div>
</div>
</div>
</div>
</blockquote>
<br>
Hm. Changing to FLAG_SET_DEFAULT(ConcGCThreads,
ParallelGCThreads / 2) might be the way to go, but I
think that should probably done as a separate
change. That way we can performance test it more
thoroughly.<br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> <br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"> <br>
Also, I don't fully understand the name
AdjustGCThreadsToCores. In
VM_Version::calc_parallel_worker_threads()
for x86 we simply active_core_count with
2 if this flag is enabled. So, the flag
does not really adjust to the cores. It
seems like it is reduces the number of
GC threads. How about calling the flag
ReduceGCThreads or something like that?<br>
</div>
</blockquote>
<div><br>
</div>
<div>The flag can be named better. However,
ReduceGCThreads doesn't seem to reflect
what this flag does.</div>
<div>I am pretty bad at naming, so let me
summarize what this flag is actually
doing.</div>
<div><br>
</div>
<div>The flag adjusts the GC threads to the
number of "available" physical cores
reported by /proc filesystem and the CPU
mask set by sched_setaffinity.</div>
<div>For example, ParallelGCThreads will
remain the same regardless of whether
hyperthreading is turned on/off.</div>
<div>Current hotspot code will have twice
more GC threads if hyperthreading is on.</div>
<div>Usually, GC causes huge number of cache
misses, thus having two GC threads
competing for the same physical core hurts
the GC throughput.</div>
<div>Current hotspot code doesn't consider
CPU mask at all.</div>
<div>For example, even though the machine
has 64 cores, if CPU mask is set for 2
cores, current hotspot calculates the
number of GC threads based on 64.</div>
<div>Thus, this flag is actually evaluating
the number of GC threads to the number of
physical cores available for the JVM
process.</div>
</div>
</div>
</div>
</blockquote>
<br>
Right. In VM_Version::calc_parallel_worker_threads()
we take the value of os::active_core_count() and
divide it by 2. I guess this is to reduce the cache
issues. But if the flag is called
AdjustGCThreadsToCores I would have expected that we
set the number of GC threads to be equal to the core
count. That's why I suggested "Reduce" in the name.<br>
</blockquote>
<blockquote type="cite"> <br>
Naming is hard and I am not particularly fond of the
name ReduceGCThreads either. But maybe we can try to
come up with something else?<br>
</blockquote>
<br>
</div>
</div>
How about ScaleGCThreadsByCores?<span><font color="#888888"><br>
<br>
Jon</font></span>
<div><br>
<br>
<blockquote type="cite"> <br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"> I
think I pointed this out earlier, but I
don't feel comfortable reviewing the
changes in os_linux_x86.cpp. I hope
someone from the Runtime team can review
that.</div>
</blockquote>
<div><br>
</div>
<div>Can you clarify what you meant? /proc
& cpu mask is dependent on Linux &
x86, and I only tested on that platform.</div>
<div>The assumptions I used here is based on
the x86 cache architecture.</div>
</div>
</div>
</div>
</blockquote>
<br>
What I was trying to say was that I don't know enough
about Linux to be confident that your implementation
of os::active_core_count() is the simplest and most
stable way to retrieve that information. I'm sure it
is good, I am just not the right person to review this
piece of the code. That's why I think it would be good
if someone from the Runtime team looked at this.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><br>
</div>
<div>Jungwoo</div>
<div><br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
</blockquote>
<br>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</blockquote>
<br>
</div></div></div>
</blockquote></div><br></div>