Request for Review (s) - 8133023: ParallelGCThreads is not calculated correctly

[Jon Masamitsu]

Kim,


On 11/26/2015 02:34 PM, Kim Barrett wrote:
> On Nov 25, 2015, at 5:10 PM, Jon Masamitsu <jon.masamitsu at oracle.com> wrote:
>> I have a new fix for this bug.  My previous fix broke solaris-x86 (I
>> had not defined an early_initialize() for x86).  This fix is slightly
>> smaller and has the virtue of moving the required initialization
>> closer to where it is used.
>>
>> http://cr.openjdk.java.net/~jmasa/8133023/webrev.03/index.html
>>
>> Testing: JPRT build on all platforms, checked by hand that the correct number
>> of GC worker threads are created on later Niagara platforms.
>>
>> Thanks.
> ------------------------------------------------------------------------------
> The new location for this setup (from os::init_before_ergo) is better
> than the earlier proposed change, which had it in
> VM_version::early_initialize.  The latter is incorrect, because
> determine_features (sparc) examines some command-line arguments, and
> those aren't parsed until a later stage of Threads::create_vm.
>
> Specifically, determine_features (sparc) is presently looking at two
> CLAs: UseV8InstrsOnly (develop) and UseNiagaraInstrs (product).  I
> suspect UseV8InstrsOnly has served its purpose and could be purged.
> But moving the call to determine_features before argument parsing
> would have unintentionally ignored UseNiagaraInstrs.
>
> The new location doesn't have this problem.  Sorry I missed this in my
> earlier review.
>
> ------------------------------------------------------------------------------
> src/cpu/x86/vm/vm_version_x86.hpp
>   595   static void vm_init_before_ergo() {};
>
> Trailing semi-colon is not needed.

Fixed.

>
> ------------------------------------------------------------------------------
> src/cpu/sparc/vm/vm_version_sparc.cpp
>    39   guarantee(VM_Version::has_v9(), "only SPARC v9 is supported");
>    40   assert(_features != VM_Version::unknown_m, "System pre-initialization is not complete.");
>
> I think the assert should be first.  The test performed by the
> guarantee is really only valid if the pre-initialization has been
> performed.

Agreed.  Fixed.
>
> ------------------------------------------------------------------------------
> src/share/vm/runtime/os.cpp
>   319   VM_Version::vm_init_before_ergo();
>
> This call is in generic code, but only two definitions have been
> provided, for sparc and x86.  Missing are aarch64, ppc, and zero.

Add vm_init_before_ergo() for those even though JPRT does not
build them? I don't need to be convinced to do it.  Just
encouraged.  Say "do it" and I'll do it.

Jon

>
> ------------------------------------------------------------------------------
>

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