Request for review (s) : 8017462: G1: guarantee fails with UseDynamicNumberOfGCThreads

[Derek White]

On 3/10/15 2:32 PM, Jon Masamitsu wrote:
>
> On 3/10/2015 9:43 AM, Derek White wrote:
>> On 3/10/15 12:26 PM, Jon Masamitsu wrote:
>>> 8017462: G1: guarantee fails with UseDynamicNumberOfGCThreads
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8017462
>>>
>>> When fewer than the maximum number of threads was being used for
>>> a parallel section, phase times for the threads that did not execute 
>>> and
>>> averages for the phase were misleading.  The fix passes the active 
>>> number of
>>> GC threads  to the G1 phase timers.
>>>
>>> http://cr.openjdk.java.net/~jmasa/8017462/webrev.00/
>>>
>>> Tested with gc_test_suite.
>> Hi Jon,
>>
>> Looks good to me.
>>
>> Just a question: In g1StringDedup.cpp:
>>   void G1StringDedup::unlink_or_oops_do(BoolObjectClosure* is_alive, OopClosure* keep_alive, bool allow_resize_and_rehash) {
>>     assert(is_enabled(), "String deduplication not enabled");
>> -  G1CollectorPolicy* g1p = G1CollectedHeap::heap()->g1_policy();
>> -  g1p->phase_times()->note_string_dedup_fixup_start();
>> +  G1CollectedHeap* g1h = G1CollectedHeap::heap();
>> +  G1CollectorPolicy* g1p = g1h->g1_policy();
>> +  uint active_workers = g1h->workers()->active_workers();
>> +  g1p->phase_times()->note_string_dedup_fixup_start(active_workers);
>>     double fixup_start = os::elapsedTime();
>>   
>>     G1StringDedupUnlinkOrOopsDoTask task(is_alive, keep_alive, allow_resize_and_rehash);
>> -  G1CollectedHeap* g1h = G1CollectedHeap::heap();
>> -  g1h->set_par_threads();
>> +  g1h->set_par_threads(active_workers);
>>
>> You switched from not changing the number of active workers 
>> "set_par_threads()" to explicitly setting the
>> number "g1h->set_par_threads(active_workers)".
>>
>> Do you expect to get a different number of active threads this way, 
>> or are you just making the code clearer by
>> making the count explicit?
>
> I expect to get a different number of active workers.  The policy for 
> number of GC workers depends
> on the number of Java threads and the size of the heap that has been 
> committed.  Either/both can
> change so the number of GC workers can change.  Does that answer the 
> question?

OK, Thanks for the info Jon. So I guess there could be a slight 
performance difference then. To extent that the new active count is more 
up to date, it could be a tiny win.

  - Derek

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