RFR: 8200106: Move NoSafepointVerifier out from gcLocker.hpp

Stefan Karlsson stefan.karlsson at oracle.com
Thu Mar 22 22:18:34 UTC 2018


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.

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



More information about the hotspot-dev mailing list