RFR: 8200106: Move NoSafepointVerifier out from gcLocker.hpp (original) (raw)

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Thu Mar 22 18:55:34 UTC 2018

On 3/22/18 2:47 PM, 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.

Thanks.

http://cr.openjdk.java.net/~stefank/8200106/webrev.01/src/hotspot/share/gc/shared/gcLocker.cpp.udiff.html

This seems strange to have another isatsafepoint function.   I don't see why you changed this as isatsafepoint 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.

Ok, it's a private method so that's ok.

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.

Agree.  Thought you needed both, but the declaration is better.

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.

Ok, I don't need to see another webrev.  Changes are reviewed as long as testing goes well.

thanks, Coleen

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