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

[Jon Masamitsu]

On 12/17/2015 09:13 AM, Tom Benson wrote:
> Hi Jon,
> OK, I don't object strongly...  It just seemed like extra overhead for 
> what is ultimately an assert.

It is just an assert but the question is where do you put it.
Having the test means I don't have to think about where
to put it (genesis() is fine but I had to go look through the
code to see that it was late enough during initialization).

But I don't need to push this change and can add the assert
when I'm doing some work in GC thread initialization.

Let's drop it.

Jon

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