Request for review (s) - 8145031: Add regression test for 8133023

[Tom Benson]

Hi Jon,
OK, I don't object strongly...  It just seemed like extra overhead for 
what is ultimately an assert.

I did notice that there is some GC-specific code in an 'ifdef ASSERT' at 
the end of Universe::genesis, at the end of init.  It does some setup 
for FullGCAlot, apparently.  So that would be a possible place to put 
it, but I bet you'd rather get rid of the code that's already  there. 8^)
Tom


On 12/17/2015 10:52 AM, Jon Masamitsu wrote:
> Tom,
>
> The other reason I wanted to add the test was that I was surprised
> by the failure.  The order of initialization was moved around at
> some point in jdk8 and created this failure.   I could have
> done some assertion checking when the GC threads were
> created but that would have been during JVM initialization
> and I wanted to get completely past initialization and then
> do the checking.  I don't expect initialization  to be moved
> around so much that GC thread initialization would be done
> before cpu identification but I didn't expect the first bug
> could happen either.
>
> Jon
>
>
> On 12/16/2015 02:45 PM, Jon Masamitsu wrote:
>> Tom,
>>
>> Thanks for looking at this.
>>
>> On 12/16/2015 07:45 AM, Tom Benson wrote:
>>> Hi Jon,
>>> This looks OK to me (modulo Thomas' comments), but it's not clear to 
>>> me why a unit test is needed for this case, rather than a simple 
>>> assert somewhere, such as when the threads are created. Thanks,
>>
>> Do you mean add an assertion when each GC thread is created?
>>
>> I think that would add checks to 2 places (one for ParallelGC and
>> one for the other GC's - ignoring the concurrent threads). Adding
>> the check at one place seemed better.
>>
>> Or maybe you meant someplace else?
>>
>> Jon
>>
>>> Tom
>>>
>>> On 12/15/2015 2:01 PM, Jon Masamitsu wrote:
>>>>
>>>>
>>>> On 12/15/2015 2:24 AM, Thomas Schatzl wrote:
>>>>> Hi Jon,
>>>>>
>>>>> On Thu, 2015-12-10 at 11:50 -0800, Jon Masamitsu wrote:
>>>>>> The cause of the failure in 8133023 was the calculation of the
>>>>>> ParallelGCThreads using
>>>>>> information that had not yet been initialized. Add a test that 
>>>>>> checks
>>>>>> that, after full
>>>>>> initialization, the fresh calculation of ParallelGCThreads is
>>>>>> consistent with the value
>>>>>> calculated earlier.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~jmasa/8145031/webrev.00/
>>>>>>
>>>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8145031
>>>>>>
>>>>>> Tested with and without the fix for 8133023.
>>>>>>
>>>>>    seems okay, with one nit: the indendation of the parameters for 
>>>>> the
>>>>> assert in Abstract_VM_Version::FinalParallelGCThreads_test() seems 
>>>>> off.
>>>>>
>>>>> Also there is an extra space between the "void" and the method name.
>>>>
>>>> Both fixed.  Thanks.
>>>>
>>>> Jon
>>>>
>>>>>
>>>>> I do not need to re-review.
>>>>>
>>>>> Thanks,
>>>>>    Thomas
>>>>>
>>>>>
>>>>
>>>
>>
>