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

[David Holmes]

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.

But ok.

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
>
>>
>> ------------------------------------------------------------------------------
>>
>>
>>
>