RFR: 8200106: Move NoSafepointVerifier out from gcLocker.hpp

[coleen.phillimore at oracle.com]

On 3/22/18 7:45 PM, David Holmes wrote:
> Hi Stefan,
>
> I've looked through everything now and it all seems okay. One 
> additional comment, and a follow up below ...
>
> On 23/03/2018 8:18 AM, 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.
>
> Now I see what this part was about, I'll just comment that which ever 
> way this ends up, interfaceSupport.* needs some future tidy up. If we 
> are going to have the plain .hpp file then a lot of what is in the 
> .inline.hpp can possibly move across (ie basic class definitions).

The reason that I moved all of interfaceSupport.hpp classes to 
interfaceSupport.inline.hpp was because all of these classes are needed 
to be inlined into the JRT macros, and also are needed to be inlined at 
their call sites.  There were no classes left that didn't have this 
characteristic that could be included without their inline definitions 
and were needed as class declarations in header files.

Moving the inlined functions (which turned out to be all of them) to 
leave class declaration shells seems like a waste of time and a not 
meaningful division.   If you think differently and have a webrev, I 
would be interested to see if it turns out better than I expect it would.

Thanks,
Coleen
>
>>>>>>>
>>>>>>> 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 
>
> Wow! Is that the transitive closure? I see 7 direct includes and 2 of 
> them seem unnecessary to start with. I think os.hpp is likely the main 
> problem and that is easily fixed by moving somethings to the .cpp 
> file. This header file should not be something that code is reluctant 
> to include because of the issues you cite. It's primary role should be 
> to provide the is_at_safepoint function and the related assertions. 
> Another RFE I guess.
>
>> 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.
>
> That would be a step in the wrong direction. :)
>
> Thanks,
> David
>
>> 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
>>>>>>>
>>>>>>
>>>>
>>