<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
<div class="moz-cite-prefix">On 11/24/13 11:48 PM, Bengt Rutisson
wrote:<br>
</div>
<blockquote cite="mid:529300E8.1030106@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 11/22/13 6:48 PM, Jon Masamitsu
wrote:<br>
</div>
<blockquote cite="mid:528F98EF.1090407@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<br>
<div class="moz-cite-prefix">On 11/22/2013 4:33 AM, Bengt
Rutisson wrote:<br>
</div>
<blockquote cite="mid:528F4F1E.7060309@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<br>
<br>
<blockquote
cite="mid:CA+n_jhiHyRoJrwvgXKV5X6vSsZWgGLJMmNYmMSLCwqmcyBtmzg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div> Do we need the flag AdjustGCThreadsToCores?
It seems like this is a good change and it is
always nice to reduce the number of flags we
have. If you feel unsure about the change it may
be good to be able to disable it, but in general
I think it would be better to decide that this
is the default behavior and go with it.<br>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>I have no strong opinion. If you guys can confirm
the improvements and feel like you don't need the
flag, that would be ideal.</div>
</div>
</div>
</div>
</blockquote>
<br>
Yes, this is an interesting dilemma. Personally I don't think
I have the cycles right now to do any performance testing of
this fix. But I assume that you don't have access to enough
machines and benchmarks to test this properly on all supported
platforms. I'll bring this up internally to see if there is
any process for handling this type of situation...<br>
<br>
Since your change is for Linux only and you have done the
measurements there, I'm fine with your results. I would prefer
skipping the AdjustGCThreadsToCores flag based on the data you
provided. Especially since we can still set ParallelGCThreads
explicitly on the command line if we need to work around a
regression somewhere.<br>
</blockquote>
<br>
I fully expect to need the AdjustGCThreadsToCores flag on Sparc
so I would prefer<br>
it be left in. <br>
</blockquote>
<br>
Can you explain why this will be particularly needed on Sparc? I
assume you mean Linux/Sparc since the Solaris implementation is
not changed by this patch. If we think this is not a good change
for Sparc, then maybe we should not do it there...<br>
</blockquote>
<br>
I say that for two reasons.<br>
<br>
In the past I've seen performance results that show that some
application<br>
running on solaris benefit from using all the hardware threads there
are<br>
for GC. Incrementally adding 1 more GC worker always decreases GC<br>
pauses. That is definitely not true for all applications. <br>
<br>
Niagara already is a special case (uses the 5/16's rule instead of
the 5/8's<br>
rule) so I expect that it will be different.<br>
<blockquote cite="mid:529300E8.1030106@oracle.com" type="cite"> <br>
<blockquote cite="mid:528F98EF.1090407@oracle.com" type="cite">
Make it an experimental flag to make it easier to remove later<br>
<pre><span class="new">1405 <font color="#ff0000">experimental</font>(bool, AdjustGCThreadsToCores, true, \</span>
<span class="new">1406 "Create GC worker threads with respect to the number of physical "\</span>
<span class="new">1407 "cores available from the CPU mask. Ignores hardware threads") \</span>
<span class="new">1408 </span></pre>
<br>
</blockquote>
<br>
Yes, this is a bit better, but I still don't fully understand why
we want to keep the old behavior under a flag. More comments on
that below.<br>
<br>
<blockquote cite="mid:528F98EF.1090407@oracle.com" type="cite"> We
do have the ParallelGCThreads flag to workaround regressions but
users don't<br>
know what value is currently set by the ergonomics so won't know
what value<br>
to use for ParallelGCThreads to workaround a regression. <br>
</blockquote>
<br>
It is not too hard to come up with the "old" default value for
ParallelGCThreads. We know what the logic was so if just get
processor information from the machine we can figure it out and in
worst case we can ask a customer to run with an older version of
the JDK and do "PrintFlagsFinal | grep ParallelGCThreads".<br>
</blockquote>
<br>
True but if there is a general category of workloads where the old
default is better,<br>
there will be many, many calls from users who see an increase in GC
pauses and<br>
have not idea why.<br>
<blockquote cite="mid:529300E8.1030106@oracle.com" type="cite"> <br>
I'm not going to insist that we remove the AdjustGCThreadsToCores
flag. We can keep it if you feel it is important. Personally I
just think the cost of having it is higher than the benefit. We
have extra code to support, more complex logic to take into
account when debugging and we most likely will not test with this
flag enabled so bugs may creep in without us noticing.<br>
</blockquote>
<br>
We don't have the data yet to know if we need AdjustGCThreadsToCores<br>
on other platforms. Actually, I don't know if we have the data to
say that<br>
we don't need the flag on linux. It may be workload dependent.<br>
It will make it easier to do the comparisons if we have a flag. <br>
If you think we should do the implementation on the other<br>
platforms and the performance testing before first integration,
that's a <br>
reasonable way to do it.<br>
<br>
Jon<br>
<blockquote cite="mid:529300E8.1030106@oracle.com" type="cite"> <br>
Bengt<br>
<br>
<blockquote cite="mid:528F98EF.1090407@oracle.com" type="cite"> <br>
Jon<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>