RFR: 8200106: Move NoSafepointVerifier out from gcLocker.hpp

[David Holmes]

Hi Stefan,

Jumping in on one issue ...

On 23/03/2018 4:47 AM, 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.
>>
>> 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.

Why is that an issue?

And shouldn't gcLocker.cpp be including safepoint.hpp now? (I guess it 
gets it indirectly from "runtime/thread.inline.hpp")

Thanks,
David
-----

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