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

[Jon Masamitsu]

Latest delta on the patch is at

http://cr.openjdk.java.net/~jmasa/8133023/delta.06/

It contains a name change from the original patch
vm_init_before_ergo() -> init_before_ergo() and some
additional comments.

Full patch is at

http://cr.openjdk.java.net/~jmasa/8133023/webrev.06/

Thanks.

Jon

On 12/7/2015 9:17 PM, David Holmes wrote:
> 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
>>>>
>>