Better default for ParallelGCThreads and ConcGCThreads by using number of physical cores and CPU mask.

[Bengt Rutisson]

Hi Jungwoo,

Thanks for contributing this change. Nice work.

First a couple general question.

Why did you only make this change for CMS? Shouldn't the other 
collectors benefit from this as well? You said you tried the other GCs 
as well. What were the results for them?

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.

os_linux_x86.cpp

This is where the important part of your change is. I don't know enough 
about Linux internals to validate the changes you have made there. I 
will leave that out of my review and let others (Runtime team?) with 
more Linux knowledge review it.


Some code comments:

Since core_count() returns 0 for all platforms except linux_x86, 
couldn't the following code be moved to a platform independent place?

  758 int os::active_core_count() {
  759   if (os::core_count() <= 0) {
  760     os::set_core_count(os::active_processor_count());
  761   }
  762   return os::core_count();
  763 }

That way this code would not have to be duplicated in all other platforms:

  726 int os::active_core_count() {
  727   return os::active_processor_count();
  728 }

We could just always use os::active_core_count() and rely on the value 
it returns.


VM_Version::calc_parallel_worker_threads()

  862     unsigned int ncpus = (unsigned int) os::active_core_count();
  863     return (ncpus <= 8) ? ncpus : ncpus / 2;

Isn't it strange that if we have 8 cores we get 8 parallel threads, but 
if we have 12 cores we only get 6? Wouldn't it make more sense to treat 
this limit as a max value, so:

return (ncpus <= 8) ? ncpus : MAX2(8, ncpus / 2);



concurrentMarkSweepGeneration.cpp

  639       if (AdjustGCThreadsToCores) {
  640         FLAG_SET_DEFAULT(ConcGCThreads, ParallelGCThreads / 2);
  641       } else {
  642         FLAG_SET_DEFAULT(ConcGCThreads, (3 + ParallelGCThreads) / 4);
  643       }

This looks a little odd to me. Why do we add 3? That does not seem to 
scale when we have a large number of parallel threads. Did you mean to 
multiply by 3?

Also, do we really need to use a different calculation for the 
ConcGCThreads? It would have been nice to be able to use the same logic 
for both cases. Did you measure any performance difference in the two cases?


Thanks,
Bengt


On 11/20/13 12:35 AM, Jungwoo Ha wrote:
> Hi,
>
> I am sending this webrev for the review.
> (On behalf of Jon Masamitsu, it is upload here)
> http://cr.openjdk.java.net/~jmasa/8028554/webrev.00/ 
> <http://cr.openjdk.java.net/%7Ejmasa/8028554/webrev.00/>
>
> The feature is a new heuristics to calculate the default 
> ParallelGCThreads and ConGCThreads.
> In x86, hyperthreading is generally bad for GC because of the cache 
> contention.
> Hence, using all the hyper-threaded cores will slow down the overall 
> GC performance.
> Current hotspot reads the number of processors that the Linux reports,
> which treats all hyper-threaded cores equally.
> Second problem is that when cpu mask is set, not all the cores are 
> available for the GC.
>
> The patch improves the heuristics by evaluating the actual available 
> physical cores
> from the proc filesystem and the CPU mask, and use that as the basis 
> for calculating the ParallelGCThreads and ConcGCThreads.
>
> The improvements of GC pause time is significant. We evaluated on 
> Nehalem, Westmere, Sandybridge as well as several AMD processors. We 
> also evaluated on various CPU mask configuration and single/dual 
> socket configurations.
> In almost all cases, there were speed up in GC pause time by 10~50%.
>
> We primarily use CMS collector for the evaluation, but we also tested 
> on other GCs as well.
> Please take a look and let me know if this patch can be accepted.
>
> Thanks,
> Jungwoo Ha
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20131121/fedcfb69/attachment.htm>