RFR: 8200106: Move NoSafepointVerifier out from gcLocker.hpp

[coleen.phillimore at oracle.com]

On 3/22/18 2:47 PM, Stefan Karlsson wrote:
> On 2018-03-22 19:33, coleen.phillimore at oracle.com wrote:
>>
>> http://cr.openjdk.java.net/~stefank/8200106/webrev.01/src/hotspot/share/runtime/interfaceSupport.hpp.html 
>>
>>
>> This is really interesting. I never noticed this buried in gcLocker.  
>> I think this should probably go in interfaceSupport.inline.hpp like 
>> all the classes that are only used there instead, unless you think it 
>> should be used on its own.  I'm not sure about that.
>
> I can move that.

Thanks.

>>
>> http://cr.openjdk.java.net/~stefank/8200106/webrev.01/src/hotspot/share/gc/shared/gcLocker.cpp.udiff.html 
>>
>>
>> This seems strange to have another is_at_safepoint function.   I 
>> don't see why you changed this as is_at_safepoint is inlined in 
>> safepoint.hpp.
>
> I did that so that we wouldn't have to include safepoint.hpp in 
> gcLocker.hpp, just to be able to do an assert.

Ok, it's a private method so that's ok.
>
>>
>> http://cr.openjdk.java.net/~stefank/8200106/webrev.01/src/hotspot/share/gc/shared/gcLocker.inline.hpp.udiff.html 
>>
>>
>> I thought the inline keyword should be on both declaration and 
>> definition (?)  Do we need these functions to be inlined anyway? Can 
>> we put them in gcLocker.cpp and remove the .inline file.  It looks 
>> like the inline file is not included in most places anyway with this 
>> change.
>
> You can have it either at the declaration or the definition. The 
> benefit of having it on the declaration is that the compiler will 
> complain instead of the linker. This might be performance sensitive, 
> so I didn't want to risk moving it to the .cpp file.

Agree.  Thought you needed both, but the declaration is better.
>
>>
>> http://cr.openjdk.java.net/~stefank/8200106/webrev.01/src/hotspot/share/runtime/interfaceSupport.cpp.udiff.html 
>>
>>
>> nit, can you add // ASSERT to the #endif ?
>
> Sure.
>
>>
>> http://cr.openjdk.java.net/~stefank/8200106/webrev.01/src/hotspot/share/runtime/safepointVerifiers.hpp.html 
>>
>>
>> I like this and the name seems Ok.  This will be a lot easier to find 
>> than in GCLocker.  Thank you for this change.
>
> Thanks! I'll make the changes and will send out a new webrev.

Ok, I don't need to see another webrev.  Changes are reviewed as long as 
testing goes well.

thanks,
Coleen
>
> StefanK
>>
>> Coleen
>>
>>
>> On 3/22/18 12:26 PM, Stefan Karlsson wrote:
>>> This patch builds upon the changes in:
>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-March/030928.html 
>>>
>>>
>>> StefanK
>>>
>>> On 2018-03-22 17:24, Stefan Karlsson wrote:
>>>> Hi all,
>>>>
>>>> Please review this patch to separate out the NoSafepointVerifier 
>>>> class (and friends) from gcLocker.hpp into its own file.
>>>>
>>>> http://cr.openjdk.java.net/~stefank/8200106/webrev.01
>>>> https://bugs.openjdk.java.net/browse/JDK-8200106
>>>>
>>>> After this patch gcLocker.hpp only contains code for the GCLocker. 
>>>> I've gone through all usages of the GCCLocker and 
>>>> NoSafepointVerifier classes and changed the code to include the 
>>>> correct headers.
>>>>
>>>> The new files are names safepointVerifiers.hpp/cpp and the main 
>>>> class is NoSafepointVerifier. However, I also moved the 
>>>> NoGCVerifier, which is the parent class of NoSafepointVerifier, and 
>>>> NoAllocVerfier. I think all of these are used to verify that we 
>>>> don't do things that will interact badly with safepoints, hence the 
>>>> name of the new file. Are others OK with the name?
>>>>
>>>> Thanks,
>>>> StefanK
>>
>