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

[Bengt Rutisson]

Hi Jungwoo,

On 1/11/14 2:23 AM, Jungwoo Ha wrote:
> I attached the updated webrev.
> I reflected those comments from others except that making 
> os::active_core_count platform independent.
> I think it is impossible to do it without using ifdefs on os.cpp.
> Let me know what else needs to be done.

Thanks for the update.

In CMSCollector there is still this code to change the value for 
ConcGCThreads based on AdjustGCThreadsToCores.

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

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.

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?

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.

Bengt



>
>
>
> On Thu, Jan 9, 2014 at 5:04 PM, Jungwoo Ha <jwha at google.com 
> <mailto:jwha at google.com>> wrote:
>
>>
>>
>>             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.
>>
>>
>>         Sounds good to me.
>
>         Good.
>
>
>     I am now working on this update. If we pull
>     os::active_core_count() to os.cpp, where should the linux_x86
>     version be?
>
>
>     Jungwoo
>
>

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