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

[Jon Masamitsu]

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?

Thanks for the review.

Jon

>
>  - Derek

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