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

[Jon Masamitsu]

On 1/15/2014 4:51 AM, Bengt Rutisson wrote:
>
> On 2014-01-13 22:39, Jungwoo Ha wrote:
>>
>>
>>     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.
>>
>>
>> I am happy to just use FLAG_SET_DEFAULT(ConcGCThreads, 
>> ParallelGCThreads / 2);
>> The original hotspot code used FLAG_SET_DEFAULT(ConcGCThreads, (3 + 
>> ParallelGCThreads) / 4); which I think is somewhat arbitrary.
>> Now that ParallelGCThreads will reduce on some configuration, 
>> dividing it into 4 seems to make the ConcGCThreads too small.
>
> 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.
>
>>
>>
>>     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?
>>
>>
>> The flag can be named better. However, ReduceGCThreads doesn't seem 
>> to reflect what this flag does.
>> I am pretty bad at naming, so let me summarize what this flag is 
>> actually doing.
>>
>> 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.
>> For example, ParallelGCThreads will remain the same regardless of 
>> whether hyperthreading is turned on/off.
>> Current hotspot code will have twice more GC threads if 
>> hyperthreading is on.
>> Usually, GC causes huge number of cache misses, thus having two GC 
>> threads competing for the same physical core hurts the GC throughput.
>> Current hotspot code doesn't consider CPU mask at all.
>> 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.
>> Thus, this flag is actually evaluating the number of GC threads to 
>> the number of physical cores available for the JVM process.
>
> 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.
>
> 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?

How about ScaleGCThreadsByCores?

Jon

>
>>
>>     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.
>>
>>
>> Can you clarify what you meant? /proc & cpu mask is dependent on 
>> Linux & x86, and I only tested on that platform.
>> The assumptions I used here is based on the x86 cache architecture.
>
> 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.
>
> Thanks,
> Bengt
>
>
>>
>> Jungwoo
>>
>

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