Request for Review (s) - 8157620: Guarantee in run_task(task, num_workers) fails
thomas.schatzl at oracle.com
Tue Jun 7 08:36:54 UTC 2016
On Fri, 2016-06-03 at 14:04 -0700, Jon Masamitsu wrote:
> A phase of a G1 collection chooses its own number of GC workers
> but doesn't add more workers if more are needed than have been
> created. Add additional workers as needed. Also changed the
> guarantee to check the number of workers against the total number
> of workers since the number of GC workers used to execute the
> task may be different than active_workers (as in this case).
> Added an execution of TestGCOld with parameters that evoked the
> Tested fix with TestGCOld.
> Stability tested with gc_test_suite
looks good, with two comments:
- I would _prefer_ if run_task(workgang, uint) always called
add_workers() itself. Otherwise all of these invocations need to call
add_workers() first themselves (if they do not limit themselves to
The run_task(workgang, uint) method has kind of been implemented as a
short/quick way to run a workgang with a given number of threads, so
this requirement adds additional boiler plate code again...
- in the test, maybe put the -Xmx/-Xms right after the @run
main/othervm like in the other tests just for consistency.
One comment related to the code changed, but unrelated to this CR: the
code in G1ConcurrentMark::calc_parallel_marking_threads() looks
(almost?) the same as AdaptiveSizePolicy::calc_active_conc_workers().
Any arguments about cleaning this up at some point?
When looking at the calculation of the
AdaptiveSizePolicy::calc_default_active_workers() method, wouldn't it
be better if instead of using Universe::heap()->capacity(), use
something like Universe::heap()->used() (and actually e.g. for g1 one
could subtract young gen and areas covered by primitive arrays too).
Actually, this calculation should be different for young/old gen gc
imho, as for both completely different areas are covered by the
collection... (just some random thoughts when looking at this).
More information about the hotspot-gc-dev