RFR: 8200106: Move NoSafepointVerifier out from gcLocker.hpp

Fri Mar 23 07:49:51 UTC 2018

On 2018-03-23 00:45, 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).

I moved the class to the .inline.hpp, and removed the 
interfaceSupport.hpp file I added.

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

Yes, I think it would make sense to start cleaning os.hpp at some point.

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

Yes. :)

StefanK

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