RFR: 8200106: Move NoSafepointVerifier out from gcLocker.hpp

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Thu Mar 22 18:33:04 UTC 2018


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.

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.

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.

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 ?

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.

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