Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8081833: Clean up JVMFlag getter/setter code #163

Closed
wants to merge 3 commits into from

Conversation

iklam
Copy link
Member

@iklam iklam commented Sep 15, 2020

This is a follow to JDK-8243208 Clean up JVMFlag implementation.

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.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/163/head:pull/163
$ git checkout pull/163

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 15, 2020

👋 Welcome back iklam! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Sep 15, 2020

@iklam The following labels will be automatically applied to this pull request: hotspot serviceability.

When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label (add|remove) "label" command.

@openjdk openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Sep 15, 2020
@iklam
Copy link
Member Author

iklam commented Sep 15, 2020

/label remove serviceability

@openjdk openjdk bot removed the serviceability serviceability-dev@openjdk.org label Sep 15, 2020
@openjdk
Copy link

openjdk bot commented Sep 15, 2020

@iklam
The serviceability label was successfully removed.

@iklam iklam marked this pull request as ready for review Sep 15, 2020
@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 15, 2020
@mlbridge
Copy link

mlbridge bot commented Sep 15, 2020

Webrevs

@mlbridge
Copy link

mlbridge bot commented Sep 15, 2020

Mailing list message from Kim Barrett on hotspot-dev:

The webrev for this is broken by https://bugs.openjdk.java.net/browse/SKARA-619

See, for example, jvmFlagAccess.hpp lines 88, 98, and lines 103-110.

bool status = true;
for (int i = 0; i < NUM_JVMFlagsEnum; i++) {
JVMFlagsEnum flag_enum = static_cast<JVMFlagsEnum>(i);
if (get_range_at(flag_enum) != NULL &&

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VMFlagAccess::check_range(JVMFlag::flag_from_enum(flag_enum) will check for "get_range_at(flag_enum) != NULL", so no need for this check here?

Copy link
Member Author

@iklam iklam Sep 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is called during VM start-up, so I want to short-circuit the check, and avoid calling VMFlagAccess::check_range if the flag has no ranges.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

#define FLAG_MEMBER_ENUM_DEVELOP(type, name, value, ...) FLAG_MEMBER_ENUM_(name)
#define FLAG_MEMBER_ENUM_PD_DEVELOP(type, name, ...) FLAG_MEMBER_ENUM_(name)
#define FLAG_MEMBER_ENUM_NOTPRODUCT(type, name, value, ...) FLAG_MEMBER_ENUM_(name)
#define FLAG_MEMBER_ENUM_DECLARE(type, name, ...) FLAG_MEMBER_ENUM_(name)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have other macros named:

DECLARE_PRODUCT_FLAG
MATERIALIZE_PRODUCT_FLAG

Can we rename this one as ENUMERATE_FLAG_MEMBER, (with the verb first in the name) to be more clear and consistent?

Copy link
Member Author

@iklam iklam Sep 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about DEFINE_FLAG_MEMBER_ENUM? In C++ terminology (https://en.cppreference.com/w/cpp/language/enum), we are building an "comma-separated list of enumerator definitions".

#define FLAG_MEMBER_SET_DEVELOP(type, name, value, ...) FLAG_MEMBER_SET_(type, name)
#define FLAG_MEMBER_SET_PD_DEVELOP(type, name, ...) FLAG_MEMBER_SET_(type, name)
#define FLAG_MEMBER_SET_NOTPRODUCT(type, name, value, ...) FLAG_MEMBER_SET_(type, name)
#define FLAG_MEMBER_SET_DECLARE(type, name, ...) FLAG_MEMBER_SET_(type, name)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have other macros named:

DECLARE_PRODUCT_FLAG
MATERIALIZE_PRODUCT_FLAG

Can we rename this one as SET_FLAG_MEMBER or DECLARE_FLAG_MEMBER, (with the verb first in the name) to be more clear and consistent?

Copy link
Member Author

@iklam iklam Sep 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, we are defining a "flag member setter function". I think we should rename to

#define FLAG_MEMBER_SETTER(name) Flag_##name##_set
#define FLAG_MEMBER_SETTER_(type, name) \
  inline JVMFlag::Error FLAG_MEMBER_SETER(name)(type value, JVMFlag::Flags origin) { \
    return JVMFlagAccess::set<JVM_FLAG_TYPE(type)>(FLAG_MEMBER_ENUM(name), value, origin); \
  }

#define DEFINE_FLAG_MEMBER_SETTER(type, name, ...) FLAG_MEMBER_SETTER_(type, name)

ALL_FLAGS(DEFINE_FLAG_MEMBER_SETTER, ...

#define FLAG_TYPE(type) (JVMFlag::TYPE_ ## type)
#define DEVELOP_FLAG_INIT( type, name, value, ...) JVMFlag(FLAG_MEMBER_ENUM(name), FLAG_TYPE(type), XSTR(name), (void*)&name, DEVELOP_KIND, __VA_ARGS__),
#define DEVELOP_FLAG_INIT_PD(type, name, ...) JVMFlag(FLAG_MEMBER_ENUM(name), FLAG_TYPE(type), XSTR(name), (void*)&name, DEVELOP_KIND_PD, __VA_ARGS__),
#define PRODUCT_FLAG_INIT( type, name, value, ...) JVMFlag(FLAG_MEMBER_ENUM(name), FLAG_TYPE(type), XSTR(name), (void*)&name, PRODUCT_KIND, __VA_ARGS__),
#define PRODUCT_FLAG_INIT_PD(type, name, ...) JVMFlag(FLAG_MEMBER_ENUM(name), FLAG_TYPE(type), XSTR(name), (void*)&name, PRODUCT_KIND_PD, __VA_ARGS__),
#define NOTPROD_FLAG_INIT( type, name, value, ...) JVMFlag(FLAG_MEMBER_ENUM(name), FLAG_TYPE(type), XSTR(name), (void*)&name, NOTPROD_KIND, __VA_ARGS__),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have other macros named:

DECLARE_PRODUCT_FLAG
MATERIALIZE_PRODUCT_FLAG

Can we rename these macros:

DEVELOP_FLAG_INIT --> INITIALIZE_DEVELOP_FLAG
DEVELOP_FLAG_INIT_PD --> INITIALIZE_PD_DEVELOP_FLAG
etc. ?

Copy link
Member Author

@iklam iklam Sep 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

#define JVM_FLAG_NON_STRING_TYPES_DO(f) \
f(bool) \
f(int) \
f(uint) \
f(intx) \
f(uintx) \
f(uint64_t) \
f(size_t) \
f(double)

#define JVM_FLAG_TYPE_DECLARE(t) \
TYPE_ ## t,

enum FlagType : int {
JVM_FLAG_NON_STRING_TYPES_DO(JVM_FLAG_TYPE_DECLARE)
// The two string types are a bit irregular: is_ccstr() returns true for both types.
TYPE_ccstr,
TYPE_ccstrlist,
NUM_FLAG_TYPES
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are those macros really needed here and better than simply saying:

enum FlagType : int {
TYPE_bool,
TYPE_int,
TYPE_uint,
TYPE_intx,
TYPE_uintx,
TYPE_uint64_t,
TYPE_size_t,
TYPE_double,
TYPE_ccstr,
TYPE_ccstrlist,
NUM_FLAG_TYPES
};

Copy link
Member Author

@iklam iklam Sep 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JVM_FLAG_NON_STRING_TYPES_DO is used in several places, such as declaring functions like is_int(), get_int(), set_int(), etc. I think it's less tedious/error prone than writing the same code 9 times.

// Only manageable flags can be accessed by writeableFlags.cpp
bool is_writeable() const { return is_manageable(); }
// All flags except "manageable" are assumed to be internal flags.
bool is_external() const { return is_manageable(); }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need is_writeable() and/or is_external() if all they do is return is_manageable() ?

Copy link
Contributor

@coleenp coleenp Sep 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_external() is odd and only has one caller. You could just fix the caller and remove is_external.

Copy link
Member Author

@iklam iklam Sep 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I am doing a "syntactic" clean up, I want to keep the code behavior the same, without doing any "semantic" changes. The caller of is_external() may have a (historically) different intention than the caller of is_manageable(). I want to leave such clean up in separate RFEs.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense

@@ -145,6 +144,41 @@ static constexpr const JVMFlagLimit* const flagLimitTable[1 + NUM_JVMFlagsEnum]
)
};

int JVMFlagLimit::_last_checked = -1;
JVMFlagsEnum JVMFlagLimit::_last_checked = static_cast<JVMFlagsEnum>(-1);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some "unchecked" enum value to JVMFlagsEnum enums (ex. UNCHECKED_FLAG) and use that instead of hardcoding to -1 ?

if (flag->type() != type_enum) {
return JVMFlag::WRONG_FORMAT;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this normally happen at runtime? Maybe replace it with assert_type() instead?

Copy link
Member Author

@iklam iklam Sep 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I am not sure. It's the same behavior as the olde code.

class FlagAccessImpl_bool : public TypedFlagAccessImpl<JVM_FLAG_TYPE(bool), EventBooleanFlagChanged> {
public:
JVMFlag::Error set_impl(JVMFlag* flag, void* value_addr, JVMFlag::Flags origin) const {
bool verbose = !JVMFlagLimit::validated_after_ergo();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only verbose when not AfterErgo? Maybe add comment explaining that.

Copy link
Member Author

@iklam iklam Sep 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand the old code correctly, we print out error messages only when parsing the command-line given by the user. When we later override values that are set programmatically by the FLAG_SET_XXX() macros, we don't print out error messages (or else the user will be confused). I will rewrite the code to make it clear, and add comments.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

Copy link
Contributor

@coleenp coleenp left a comment

I like all the consolidation (red!).

@@ -1419,7 +1420,7 @@ WB_ENTRY(void, WB_SetStringVMFlag(JNIEnv* env, jobject o, jstring name, jstring
bool needFree;
{
ThreadInVMfromNative ttvfn(thread); // back to VM
needFree = SetVMFlag <ccstr> (thread, env, name, &ccstrResult, &JVMFlag::ccstrAtPut);
needFree = SetVMFlag <JVM_FLAG_TYPE(ccstr)> (thread, env, name, &ccstrResult);
Copy link
Contributor

@coleenp coleenp Sep 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if these whitebox methods could be rewritten as macros, like the jni.cpp methods, like: DEFINE_GETFIELD, etc, in a future RFE.

Copy link
Member Author

@iklam iklam Sep 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not quite sure. On the one hand I like code reduction, but on the other hand macro-generated code will be difficult to debug.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was unsure here whether to suggest the same change, and decided against. It's a fine line...

// Only manageable flags can be accessed by writeableFlags.cpp
bool is_writeable() const { return is_manageable(); }
// All flags except "manageable" are assumed to be internal flags.
bool is_external() const { return is_manageable(); }
Copy link
Contributor

@coleenp coleenp Sep 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_external() is odd and only has one caller. You could just fix the caller and remove is_external.

return set_impl(flag, type_enum, value, origin);
}

// This is called by the FLAG_SET_XXX macros.
Copy link
Contributor

@coleenp coleenp Sep 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are both FLAG_SET_XXX and SET_FLAG_XXX macros ? Is this a typo in the comment?

@openjdk
Copy link

openjdk bot commented Sep 16, 2020

@iklam This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8081833: Clean up JVMFlag getter/setter code

Reviewed-by: gziemski, coleenp
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there has been 1 commit pushed to the master branch:

  • 0e98fc1: 8253237: [REDO] Improve large object handling during evacuation

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 0e98fc1ccd3a4053ccb178045aad97ee45fdc1ca.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Sep 16, 2020
Copy link
Contributor

@coleenp coleenp left a comment

The new names are nice.

@gerard-ziemski
Copy link

gerard-ziemski commented Sep 17, 2020

Looks great, thank you Ioi for cleaning this up!

@iklam
Copy link
Member Author

iklam commented Sep 22, 2020

/integrate

@openjdk openjdk bot closed this Sep 22, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 22, 2020
@openjdk
Copy link

openjdk bot commented Sep 22, 2020

@iklam Since your change was applied there has been 1 commit pushed to the master branch:

  • 0e98fc1: 8253237: [REDO] Improve large object handling during evacuation

Your commit was automatically rebased without conflicts.

Pushed as commit 282b9dc.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
3 participants