RFR: 8200106: Move NoSafepointVerifier out from gcLocker.hpp

[coleen.phillimore at oracle.com]

On 3/22/18 6:18 PM, Stefan Karlsson wrote:
> On 2018-03-22 23:05, David Holmes wrote:
>> On 23/03/2018 7:59 AM, Stefan Karlsson wrote:
>>> 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.
>>
>> If it was to avoid some kind of problem I could understand, but just 
>> to "minimize" this seems like false economy. You shouldn't need to 
>> add forwarding wrapper functions in the local API just because you 
>> don't want to include the real API's header file.
>
> Of course I don't "need" to, but safepoint.hpp includes 158 other 
> HotSpot headers, and whenever I touch any of those, I'll get mostly 
> unnecessary recompiles of files that included gcLocker.hpp. By being 
> more restrictive with our includes, we could minimize our compile 
> times, and that's worth the tiny overhead of doing this change.
>
> Minimizing our includes in our header files also help reduce the risk 
> of getting cyclic dependencies, which do cause problems.
>
> If you don't like this, I could move is_active to gcLocker.inline.hpp, 
> and then I'd add #include "gc/shared/gcLocker.inline.hpp" to the ~50 
> files that use is_active.

No!  I thought your solution was best given that 
gcLocker::is_at_safepoint() was private.  Maybe is_active() could be in 
the cpp file instead?
Coleen
>
> StefanK
>
>> David
>>
>>>>
>>>> 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
>>>>>>
>>>>>
>>>
>