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

[Derek White]

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?

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