RFR: 8081833: Clean up JVMFlag getter/setter code
Gerard Ziemski
gziemski at openjdk.java.net
Wed Sep 16 15:22:40 UTC 2020
On Tue, 15 Sep 2020 05:01:13 GMT, Ioi Lam <iklam at openjdk.org> wrote:
> This is a follow to [JDK-8243208 Clean up JVMFlag implementation](https://bugs.openjdk.java.net/browse/JDK-8243208).
>
> I try to use templates and inheritance (and a bit of macros) to get rid of the large amount of duplicated code in the
> JVMFlag getters/setters.
> Stefan Karlsson and I have tried this several times already, but I think the current attempt probably has the least
> amount of hacks and most functionality:
> - Type safety is enforced up to the point of loading/storing `JVMFlag::_addr`, without any unchecked typecasts.
> - Runtime type checking is implemented by the `JVM_FLAG_TYPE(t)` macro (see jvmFlagAccess.hpp), by passing along an
> integer (`JVMFlag::TYPE_int`, etc). This is more efficient than previous attempts that passed along a function pointer
> (see http://cr.openjdk.java.net/~stefank/8081833/webrev.04/)
> - The old `JVMFlag::xxxAtPut` functions were undocumented. There are actually two groups that have different usage. I
> added documentation in jvmFlagAccess.hpp to explain this.
> - I got rid of the `JVMFlagEx` class and moved the code into `JVMFlag`. This is possible with C++11 which allows forward
> declaration of `enum JVMFlagsEnum : int;`
> - I changed `JVMFlag::_type` from a string into an enum. I am surprised this had lasted for over 20 years.
> - I also changed `JVMFlag` from a struct into a class and made some fields private. I probably will do more cleanup in a
> separate RFE.
>
> Please start with jvmFlagAccess.hpp, jvmFlagAccess.hpp and then globals_extension.hpp.
>
> Probably not everyone is in love with the `JVM_FLAG_TYPE(t)` macro. I'll be happy to try out alternatives.
Changes requested by gziemski (Committer).
src/hotspot/share/runtime/flags/jvmFlagLimit.cpp line 147:
> 145: };
> 146:
> 147: JVMFlagsEnum JVMFlagLimit::_last_checked = static_cast<JVMFlagsEnum>(-1);
Can we add some "unchecked" enum value to JVMFlagsEnum enums (ex. UNCHECKED_FLAG) and use that instead of hardcoding
to -1 ?
src/hotspot/share/runtime/flags/jvmFlagAccess.hpp line 78:
> 76: if (flag->type() != type_enum) {
> 77: return JVMFlag::WRONG_FORMAT;
> 78: }
Can this normally happen at runtime? Maybe replace it with assert_type() instead?
src/hotspot/share/runtime/flags/jvmFlagAccess.cpp line 91:
> 89: public:
> 90: JVMFlag::Error set_impl(JVMFlag* flag, void* value_addr, JVMFlag::Flags origin) const {
> 91: bool verbose = !JVMFlagLimit::validated_after_ergo();
Why only verbose when not AfterErgo? Maybe add comment explaining that.
-------------
PR: https://git.openjdk.java.net/jdk/pull/163
More information about the hotspot-dev
mailing list