RFR: 8200106: Move NoSafepointVerifier out from gcLocker.hpp

[Stefan Karlsson]

On 2018-03-22 22:47, David Holmes wrote:
> 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?

I simply try to minimize includes in our our .hpp files.

>
> And shouldn't gcLocker.cpp be including safepoint.hpp now?

Thanks, I missed that because the line below used SafepointSynchronize.

StefanK

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