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

[Jon Masamitsu]

On 12/08/2015 05:28 PM, David Holmes wrote:
> On 9/12/2015 7:12 AM, Jon Masamitsu wrote:
>>
>>
>> On 12/8/2015 11:37 AM, Kim Barrett wrote:
>>> On Dec 8, 2015, at 1:41 PM, Jon Masamitsu <jon.masamitsu at oracle.com>
>>> wrote:
>>>> 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/
>>> A couple minor things.  I don't need another webrev for these.  Looks
>>> good otherwise.
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>>> src/share/vm/runtime/vm_version.hpp
>>>    59   // Called as part of the runtime services initialization
>>>    60   // called from the management module initialization (via
>>> init_globals())
>>>
>>> Need period at end of line 59, and line 60 should start capitalized.
>> My intent was that it be a single sentence.  I've added a "which is" to
>> the end of the
>> line to make it more clear.
>>
>>   59   // Called as part of the runtime services initialization which is
>>   60   // called from the management module initialization (via
>> init_globals())
>
> That's why I suggested just saying "via init_globals" because the 
> details are complex as it is either initialized from management, if 
> management support is being built, else from runtime services.

I chose the "runtime services initialization which is called from the 
management modules ..."
because being less precise (not calling out init_globals()) but more 
generic, would be true
longer (than if the implementation changed and init_globals() mutated).

>
> But ok.

Thanks.

Jon

>
> Thanks,
> David
>
>
>
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>>> src/share/vm/runtime/vm_version.hpp
>>>    72   static void early_initialize() { }
>>>    73   // Called to initialize VM variables needing initialization
>>>    74   // after command line parsing
>>>    75   static void init_before_ergo(void) {}
>>>
>>> Missing period at end of sentence.
>>
>> Fixed.
>>
>>>
>>> I'd prefer there be a blank line between lines 72 and 73.
>>>
>>> Maybe the description here should have a similar sentence to the
>>> preceding description for early_initialize, e.g. add
>>>
>>>    Platforms that need to specialize this define
>>> VM_Version::init_before_ergo().
>>
>> I added it.
>>
>> Thanks.
>>
>> Jon
>>
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>>>
>>>
>>