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

8264285: Clean the modification of ccstr JVM flags #3254

Closed

Conversation

@iklam
Copy link
Member

@iklam iklam commented Mar 29, 2021

There are two versions of JVMFlagAccess::ccstrAtPut() for modifying JVM flags of the ccstr type (i.e., strings).

  • One version requires the caller to free the old value, but some callers don't do that (writeableFlags.cpp).
  • The other version frees the old value on behalf of the caller. However, this version is accessible only via FLAG_SET_XXX macros and is currently unused. So it's unclear whether it actually works.

We should combine these two versions into a single function, fix problems in the callers, and add test cases. The old value should be freed automatically, because typically the caller isn't interested in the old value.

Note that the FLAG_SET_XXX macros do not return the old value. Requiring the caller of FLAG_SET_XXX to free the old value would be tedious and error prone.


Progress

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

Issue

  • JDK-8264285: Clean the modification of ccstr JVM flags

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3254/head:pull/3254
$ git checkout pull/3254

Update a local copy of the PR:
$ git checkout pull/3254
$ git pull https://git.openjdk.java.net/jdk pull/3254/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3254

View PR using the GUI difftool:
$ git pr show -t 3254

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3254.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 29, 2021

👋 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 openjdk bot commented Mar 29, 2021

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

  • hotspot
  • jmx
  • 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 pull request command.

@iklam iklam marked this pull request as ready for review Mar 29, 2021
@openjdk openjdk bot added the rfr label Mar 29, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 29, 2021

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Ioi,

This looks good to me. Thanks for fixing it up.

One minor nit below.

Thanks,
David

if (err == JVMFlag::SUCCESS) {
assert(value == NULL, "old value is freed automatically and not returned");
}
Comment on lines +248 to +250

This comment has been minimized.

@dholmes-ora

dholmes-ora Mar 30, 2021
Member

The whole block should be ifdef DEBUG.

This comment has been minimized.

@iklam

iklam Mar 31, 2021
Author Member

Since this whole block can be optimized out by the C compiler in product builds, I'd rather leave out the #ifdef to avoid clutter.

@openjdk
Copy link

@openjdk openjdk bot commented Mar 30, 2021

@iklam This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8264285: Clean the modification of ccstr JVM flags

Reviewed-by: dholmes, coleenp

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ 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 label Mar 30, 2021
Copy link

@coleenp coleenp left a comment

I had a question but overall nice cleanup!

// Unlike the other APIs, the old vale is NOT returned, so the caller won't need to free it.
// The callers typically don't care what the old value is.
// If the caller really wants to know the old value, read it (and make a copy if necessary)
// before calling this API.

This comment has been minimized.

@coleenp

coleenp Mar 31, 2021

good comment!

// For example, DummyManageableStringFlag is needed because we don't
// have any MANAGEABLE flags of the ccstr type, but we really need to
// make sure the implementation is correct (in terms of memory allocation)
// just in case someone may add such a flag in the future.

This comment has been minimized.

@coleenp

coleenp Mar 31, 2021

Could you have just added a develop flag to the manageable flags instead?

This comment has been minimized.

@iklam

iklam Mar 31, 2021
Author Member

I had to use a product flag due to the following code, which should have been removed as part of JDK-8243208, but I was afraid to do so because I didn't have a test case. I.e., all of our diagnostic/manageable/experimental flags were product flags.

With this PR, now I have a test case -- I changed DummyManageableStringFlag to a notproduct flag, and removed the following code. I am re-running tiers1-4 now.

void JVMFlag::check_all_flag_declarations() {
  for (JVMFlag* current = &flagTable[0]; current->_name != NULL; current++) {
    int flags = static_cast<int>(current->_flags);
    // Backwards compatibility. This will be relaxed/removed in JDK-7123237.
    int mask = JVMFlag::KIND_DIAGNOSTIC | JVMFlag::KIND_MANAGEABLE | JVMFlag::KIND_EXPERIMENTAL;
    if ((flags & mask) != 0) {
      assert((flags & mask) == JVMFlag::KIND_DIAGNOSTIC ||
             (flags & mask) == JVMFlag::KIND_MANAGEABLE ||
             (flags & mask) == JVMFlag::KIND_EXPERIMENTAL,
             "%s can be declared with at most one of "
             "DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL", current->_name);
      assert((flags & KIND_NOT_PRODUCT) == 0 &&
             (flags & KIND_DEVELOP) == 0,
             "%s has an optional DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL "
             "attribute; it must be declared as a product flag", current->_name);
    }
  }
}

This comment has been minimized.

@coleenp

coleenp Apr 1, 2021

What's the difference between notproduct and develop again? Do we run tests with the optimized build and why would this flag be available in that build? ie. why not develop?

This comment has been minimized.

@iklam

iklam Apr 1, 2021
Author Member

From the top of globals.hpp:

  • develop flags are settable / visible only during development and are constant in the PRODUCT version
  • notproduct flags are settable / visible only during development and are not declared in the PRODUCT version

Since this flag is only used in test cases, and specifically for modifying its value, it doesn't make sense to expose this flag to the PRODUCT version at all. So I chose notproduct, so we can save a few bytes for the PRODUCT as well.

@coleenp
coleenp approved these changes Apr 1, 2021
// For example, DummyManageableStringFlag is needed because we don't
// have any MANAGEABLE flags of the ccstr type, but we really need to
// make sure the implementation is correct (in terms of memory allocation)
// just in case someone may add such a flag in the future.

This comment has been minimized.

@coleenp

coleenp Apr 1, 2021

What's the difference between notproduct and develop again? Do we run tests with the optimized build and why would this flag be available in that build? ie. why not develop?

This reverts commit 673aaaf.
constraint) \
\
product(ccstr, DummyManageableStringFlag, NULL, MANAGEABLE, \
"Dummy flag for testing string handling in WriteableFlags") \

This comment has been minimized.

@coleenp

coleenp Apr 1, 2021

So this is in essence a product/manageable flag in debug only mode, which doesn't make sense at all, necessitating another macro that looks honestly bizarre. So I think that means a non-product manageable flag or even a develop manageable flag is something that we want, we want to be able to write a develop flag by the management interface. I agree diagnostic and experimental flags only seem to make sense as product flags.

The doc could simply be updated. Also the doc at the top of this file refers to CCC which is no longer -> CSR.

// MANAGEABLE flags are writeable external product flags.
// They are dynamically writeable through the JDK management interface
// (com.sun.management.HotSpotDiagnosticMXBean API) and also through JConsole.
// These flags are external exported interface (see CCC). The list of
// manageable flags can be queried programmatically through the management
// interface.

I'm not going to spend time typing about this minor point. The improvement is good and should be checked in.

@iklam
Copy link
Member Author

@iklam iklam commented Apr 1, 2021

Thanks @coleenp and @dholmes-ora for reviewing.
/integrate

@openjdk openjdk bot closed this Apr 1, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Apr 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 1, 2021

@iklam Since your change was applied there have been 2 commits pushed to the master branch:

  • 6e0da99: 8263448: CTW: fatal error: meet not symmetric
  • 328e951: 8169629: Annotations with lambda expressions cause AnnotationFormatError

Your commit was automatically rebased without conflicts.

Pushed as commit 5858399.

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 2, 2021

Mailing list message from David Holmes on hotspot-dev:

On 1/04/2021 3:29 pm, Ioi Lam wrote:

On 3/31/21 10:22 PM, David Holmes wrote:

On 1/04/2021 5:05 am, Ioi Lam wrote:

On Wed, 31 Mar 2021 12:58:50 GMT, Coleen Phillimore
<coleenp at openjdk.org> wrote:

36: // have any MANAGEABLE flags of the ccstr type, but we really
need to
37: // make sure the implementation is correct (in terms of memory
allocation)
38: // just in case someone may add such a flag in the future.

Could you have just added a develop flag to the manageable flags
instead?

I had to use a `product` flag due to the following code, which should
have been removed as part of
[JDK-8243208](https://bugs.openjdk.java.net/browse/JDK-8243208), but
I was afraid to do so because I didn't have a test case. I.e., all of
our diagnostic/manageable/experimental flags were `product` flags.

With this PR, now I have a test case -- I changed
`DummyManageableStringFlag` to a `notproduct` flag, and removed the
following code. I am re-running tiers1-4 now.

Sorry but I don't understand how a test involving one flag replaces a
chunk of code that validated every flag declaration ??

When I did JDK-8243208, I wasn't sure if the VM would actually support
diagnostic/manageable/experimental flags that were not `product`. So I
added check_all_flag_declarations() to prevent anyone from adding such a
flag "casually" without thinking about.

Now that I have added such a flag, and I believe I have tested pretty
thoroughly (tiers 1-4), I think the VM indeed supports these flags. So
now I feel it's safe to remove check_all_flag_declarations().

But the check was checking two things:

1. That diagnostic/manageable/experimental are mutually exclusive
2. That they only apply to product flags

I believe both of these rules still stand based on what we defined such
attributes to mean. And even if you think #2 should not apply, #1 still
does and so could still be checked. And if #2 is no longer our rule then
the documentation in globals.hpp should be updated - though IMHO #2
should remain as-is.

David
-----

Thanks
- Ioi

David
-----

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 2, 2021

Mailing list message from David Holmes on jmx-dev:

On 1/04/2021 5:05 am, Ioi Lam wrote:

On Wed, 31 Mar 2021 12:58:50 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:

relax flag attributions (ala JDK-7123237)

src/hotspot/share/runtime/flags/debug_globals.hpp line 38:

36: // have any MANAGEABLE flags of the ccstr type, but we really need to
37: // make sure the implementation is correct (in terms of memory allocation)
38: // just in case someone may add such a flag in the future.

Could you have just added a develop flag to the manageable flags instead?

I had to use a `product` flag due to the following code, which should have been removed as part of [JDK-8243208](https://bugs.openjdk.java.net/browse/JDK-8243208), but I was afraid to do so because I didn't have a test case. I.e., all of our diagnostic/manageable/experimental flags were `product` flags.

With this PR, now I have a test case -- I changed `DummyManageableStringFlag` to a `notproduct` flag, and removed the following code. I am re-running tiers1-4 now.

Sorry but I don't understand how a test involving one flag replaces a
chunk of code that validated every flag declaration ??

David
-----

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 2, 2021

Mailing list message from Ioi Lam on jmx-dev:

On 3/31/21 10:22 PM, David Holmes wrote:

On 1/04/2021 5:05 am, Ioi Lam wrote:

On Wed, 31 Mar 2021 12:58:50 GMT, Coleen Phillimore
<coleenp at openjdk.org> wrote:

36: // have any MANAGEABLE flags of the ccstr type, but we really
need to
37: // make sure the implementation is correct (in terms of memory
allocation)
38: // just in case someone may add such a flag in the future.

Could you have just added a develop flag to the manageable flags
instead?

I had to use a `product` flag due to the following code, which should
have been removed as part of
[JDK-8243208](https://bugs.openjdk.java.net/browse/JDK-8243208), but
I was afraid to do so because I didn't have a test case. I.e., all of
our diagnostic/manageable/experimental flags were `product` flags.

With this PR, now I have a test case -- I changed
`DummyManageableStringFlag` to a `notproduct` flag, and removed the
following code. I am re-running tiers1-4 now.

Sorry but I don't understand how a test involving one flag replaces a
chunk of code that validated every flag declaration ??

When I did JDK-8243208, I wasn't sure if the VM would actually support
diagnostic/manageable/experimental flags that were not `product`. So I
added check_all_flag_declarations() to prevent anyone from adding such a
flag "casually" without thinking about.

Now that I have added such a flag, and I believe I have tested pretty
thoroughly (tiers 1-4), I think the VM indeed supports these flags. So
now I feel it's safe to remove check_all_flag_declarations().

Thanks
- Ioi

David
-----

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 2, 2021

Mailing list message from Ioi Lam on hotspot-dev:

On 3/31/21 10:47 PM, David Holmes wrote:

On 1/04/2021 3:29 pm, Ioi Lam wrote:

On 3/31/21 10:22 PM, David Holmes wrote:

On 1/04/2021 5:05 am, Ioi Lam wrote:

On Wed, 31 Mar 2021 12:58:50 GMT, Coleen Phillimore
<coleenp at openjdk.org> wrote:

36: // have any MANAGEABLE flags of the ccstr type, but we really
need to
37: // make sure the implementation is correct (in terms of
memory allocation)
38: // just in case someone may add such a flag in the future.

Could you have just added a develop flag to the manageable flags
instead?

I had to use a `product` flag due to the following code, which
should have been removed as part of
[JDK-8243208](https://bugs.openjdk.java.net/browse/JDK-8243208),
but I was afraid to do so because I didn't have a test case. I.e.,
all of our diagnostic/manageable/experimental flags were `product`
flags.

With this PR, now I have a test case -- I changed
`DummyManageableStringFlag` to a `notproduct` flag, and removed the
following code. I am re-running tiers1-4 now.

Sorry but I don't understand how a test involving one flag replaces
a chunk of code that validated every flag declaration ??

When I did JDK-8243208, I wasn't sure if the VM would actually
support diagnostic/manageable/experimental flags that were not
`product`. So I added check_all_flag_declarations() to prevent anyone
from adding such a flag "casually" without thinking about.

Now that I have added such a flag, and I believe I have tested pretty
thoroughly (tiers 1-4), I think the VM indeed supports these flags.
So now I feel it's safe to remove check_all_flag_declarations().

But the check was checking two things:

1. That diagnostic/manageable/experimental are mutually exclusive
2. That they only apply to product flags

I believe both of these rules still stand based on what we defined
such attributes to mean. And even if you think #2 should not apply, #1
still does and so could still be checked. And if #2 is no longer our
rule then the documentation in globals.hpp should be updated - though
IMHO #2 should remain as-is.

I think neither #1 and #2 make sense. These were limitation introduced
by the old flags implementation, where you had to declare a flag using
one of these macros

??? diagnostic(type, name, ....
??? manageable(type, name, ....
??? experimental(type, name, ....

That's why you have #1 (mutual exclusion).

#2 was also due to the implementation. It makes no sense that you can't
have an develop flag for an experimental feature.

However, in the old implementation, adding more variations would cause
macro explosion. See
https://github.com/openjdk/jdk/blame/7d8519fffe46b6b5139b3aa51b18fcf0249a9e14/src/hotspot/share/runtime/flags/jvmFlag.cpp#L776

Anyway, changing this should be done in a separate RFE. I have reverted
[v2] from this PR, so we are back to [v1].

Thanks
- Ioi

David
-----

Thanks
- Ioi

David
-----

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 2, 2021

Mailing list message from David Holmes on hotspot-dev:

On 1/04/2021 4:19 pm, Ioi Lam wrote:

On 3/31/21 10:47 PM, David Holmes wrote:

On 1/04/2021 3:29 pm, Ioi Lam wrote:

On 3/31/21 10:22 PM, David Holmes wrote:

On 1/04/2021 5:05 am, Ioi Lam wrote:

On Wed, 31 Mar 2021 12:58:50 GMT, Coleen Phillimore
<coleenp at openjdk.org> wrote:

36: // have any MANAGEABLE flags of the ccstr type, but we really
need to
37: // make sure the implementation is correct (in terms of
memory allocation)
38: // just in case someone may add such a flag in the future.

Could you have just added a develop flag to the manageable flags
instead?

I had to use a `product` flag due to the following code, which
should have been removed as part of
[JDK-8243208](https://bugs.openjdk.java.net/browse/JDK-8243208),
but I was afraid to do so because I didn't have a test case. I.e.,
all of our diagnostic/manageable/experimental flags were `product`
flags.

With this PR, now I have a test case -- I changed
`DummyManageableStringFlag` to a `notproduct` flag, and removed the
following code. I am re-running tiers1-4 now.

Sorry but I don't understand how a test involving one flag replaces
a chunk of code that validated every flag declaration ??

When I did JDK-8243208, I wasn't sure if the VM would actually
support diagnostic/manageable/experimental flags that were not
`product`. So I added check_all_flag_declarations() to prevent anyone
from adding such a flag "casually" without thinking about.

Now that I have added such a flag, and I believe I have tested pretty
thoroughly (tiers 1-4), I think the VM indeed supports these flags.
So now I feel it's safe to remove check_all_flag_declarations().

But the check was checking two things:

1. That diagnostic/manageable/experimental are mutually exclusive
2. That they only apply to product flags

I believe both of these rules still stand based on what we defined
such attributes to mean. And even if you think #2 should not apply, #1
still does and so could still be checked. And if #2 is no longer our
rule then the documentation in globals.hpp should be updated - though
IMHO #2 should remain as-is.

I think neither #1 and #2 make sense. These were limitation introduced
by the old flags implementation, where you had to declare a flag using
one of these macros

??? diagnostic(type, name, ....
??? manageable(type, name, ....
??? experimental(type, name, ....

That's why you have #1 (mutual exclusion).

#2 was also due to the implementation. It makes no sense that you can't
have an develop flag for an experimental feature.

I don't agree with either of those claims. This is about semantics not
implementation. diagnostic/manageable/experimental are distinct kinds of
product flags with different levels of "support" based on their intended
use in the product. We don't need such distinctions with non-product
flags because the level of "support" is all the same and all flags are
equal.

David
-----

However, in the old implementation, adding more variations would cause
macro explosion. See
https://github.com/openjdk/jdk/blame/7d8519fffe46b6b5139b3aa51b18fcf0249a9e14/src/hotspot/share/runtime/flags/jvmFlag.cpp#L776

Anyway, changing this should be done in a separate RFE. I have reverted
[v2] from this PR, so we are back to [v1].

Thanks
- Ioi

David
-----

Thanks
- Ioi

David
-----

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants