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

[David Holmes]

On 8/12/2015 2:56 AM, Jon Masamitsu wrote:
> David,
>
>
> On 12/04/2015 08:57 PM, David Holmes wrote:
>> Hi Jon,
>>
>> On 5/12/2015 2:39 AM, Jon Masamitsu wrote:
>>> Here's a new version of the fix (simpler thanks to a suggestion
>>> from Kim).
>>>
>>> http://cr.openjdk.java.net/~jmasa/8133023/webrev.05/
>>
>> Yes this seems quite neat. It is a pity about
>> yet-another-VM_Version-init-function but ...
>>
>> Only minor comment is that in VM_Version::vm_init_before_ergo the "vm"
>> part is somewhat unnecessary and doesn't fit with the existing:
>> initialize() and early_initialize(). How about just
>> initialize_before_ergo()?
>
> I dropped the "vm_" from the name.
>
>>
>> Also we now really need a comment explaining when the plain
>> initialize() function is used.
>
> Is this along the lines of what you had in mind?
>
>    // Called as part of the runtime services initialization
>    // called from the management module initialization.  Examines
>    // a variety of the hardware capabilities of the platform
>    // to determine which features can be used to execute the
>    // program.
>    static void initialize();

I had to trace this myself to see where it is placed :)

// Called via init_globals, after argument parsing and attaching
// of the main thread has occurred

Thanks,
David

> Jon
>
>>
>> Thanks,
>> David
>> -----
>>
>>> In the above webrev vm_version_x86.hpp appears but has no changes.
>>> There were changes to it in the previous version (04) but were undone in
>>> this latest version and may be why it appears in the webrev.
>>>
>>> The delta webrev is
>>>
>>> http://cr.openjdk.java.net/~jmasa/8133023/delta.05/
>>>
>>> It has a white-space change in vm_version_sparc.hpp  which
>>> does not appear in the diffs.
>>>
>>> Thanks.
>>>
>>> Jon
>>>
>>>
>>> On 12/2/2015 9:46 AM, Jon Masamitsu wrote:
>>>>
>>>>
>>>> On 11/30/2015 11:47 AM, Kim Barrett wrote:
>>>>> On Nov 30, 2015, at 12:20 PM, Jon Masamitsu
>>>>> <jon.masamitsu at oracle.com> wrote:
>>>>>>> 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.
>>>>> I think you should do it, rather than knowingly breaking the build
>>>>> for those targets when the needed code is so trivial.
>>>>>
>>>>
>>>> Here's the webrev for the delta for review comments to this point.
>>>>
>>>> http://cr.openjdk.java.net/~jmasa/8133023/delta.04/
>>>>
>>>> Webrev for complete patch is here.
>>>>
>>>> http://cr.openjdk.java.net/~jmasa/8133023/webrev.04/
>>>>
>>>> Jon
>>>
>