8236736: Change notproduct JVM flags to develop flags #18541

coleenp · 2024-03-28T22:53:22Z

Remove the notproduct distinction for command line options, rather than trying to wrestle the macros to fix the bug that they've been treated as develop options for some time now. This simplifies the command line option macros.

Tested with tier1-4, tier1 on Oracle platforms. Also built shenandoah.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8236736: Change notproduct JVM flags to develop flags (Enhancement - P4)

Reviewers

Ioi Lam (@iklam - Reviewer) ⚠️ Review applies to 19b8f6b6
Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to 00a241d3
Kim Barrett (@kimbarrett - Reviewer) ⚠️ Review applies to 00a241d3

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18541/head:pull/18541
$ git checkout pull/18541

Update a local copy of the PR:
$ git checkout pull/18541
$ git pull https://git.openjdk.org/jdk.git pull/18541/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18541

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18541.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-03-28T22:54:00Z

👋 Welcome back coleenp! 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 · 2024-03-28T22:54:09Z

@coleenp 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:

8236736: Change notproduct JVM flags to develop flags

Reviewed-by: iklam, kvn, kbarrett

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 39 new commits pushed to the master branch:

db15914: 8328753: Open source few Undecorated Frame tests
925d829: 8329013: StackOverflowError when starting Apache Tomcat with signed jar
dd5d7d0: 8327002: (fs) java/nio/file/Files/CopyMoveVariations.java should be able to test across file systems
6ae1cf1: 8329368: Generational ZGC: Remove the unnecessary friend classes in ZAllocator
7eb78e3: 8320676: Manual printer tests have no Pass/Fail buttons, instructions close set 1
5ac067f: 8329289: Unify SetupJdkExecutable and SetupJdkLibrary
5ae849d: 8329292: Fix missing cleanups in java.management and jdk.management
ed821cb: 8324807: Manual printer tests have no Pass/Fail buttons, instructions close set 2
5cf457b: 8329320: Simplify awt/print/PageFormat/NullPaper.java test
8b934aa: 8329358: Generational ZGC: Remove the unused method ZPointer::set_remset_bits
... and 29 more: https://git.openjdk.org/jdk/compare/991e04e7d7ff3ee682c2b8a9b860d325a176e7a5...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this 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 · 2024-03-28T22:54:32Z

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

graal
hotspot
serviceability
shenandoah

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.

mlbridge · 2024-04-02T13:23:16Z

Webrevs

iklam

LGTM.

For the past 15 years, "notproduct" flags haven't been working as they claim to be in globals.hpp. That doesn't seem to have bothered anyone. This definitely looks like a design that no one needs and should be removed for simplcity.

There are some references to "notproduct" in test/hotspot/jtreg/runtime/CommandLine that need to be removed.

coleenp · 2024-04-02T16:18:27Z

Thank you for pointing out that I missed cleaning up the tests. One failed but I didn't see it.

iklam

LGTM

stefank · 2024-04-02T16:49:19Z

src/hotspot/share/runtime/arguments.cpp


 static void apply_debugger_ergo() {
 #ifndef PRODUCT
-  // UseDebuggerErgo is notproduct


Now that the flag has been changed to a develop flag, it seems wrong that these are guarded by "#ifndef PRODUCT". Shouldn't this be changed to check for ASSERT instead?

Yes, ifdef ASSERT is more appropriate.

stefank · 2024-04-02T16:51:38Z

src/hotspot/share/runtime/flags/jvmFlag.hpp

    DIAGNOSTIC_FLAG_BUT_LOCKED,
    EXPERIMENTAL_FLAG_BUT_LOCKED,
    DEVELOPER_FLAG_BUT_PRODUCT_BUILD,
-    NOTPRODUCT_FLAG_BUT_PRODUCT_BUILD


Should the ',' on the previous line be removed?

Yes, I guess our compilers don't complain about that anymore.

coleenp

Thanks for looking through the changes, Stefan.

coleenp · 2024-04-02T17:07:03Z

src/hotspot/share/runtime/arguments.cpp


 static void apply_debugger_ergo() {
 #ifndef PRODUCT
-  // UseDebuggerErgo is notproduct


Yes, ifdef ASSERT is more appropriate.

coleenp · 2024-04-02T17:12:40Z

src/hotspot/share/runtime/flags/jvmFlag.hpp

    DIAGNOSTIC_FLAG_BUT_LOCKED,
    EXPERIMENTAL_FLAG_BUT_LOCKED,
    DEVELOPER_FLAG_BUT_PRODUCT_BUILD,
-    NOTPRODUCT_FLAG_BUT_PRODUCT_BUILD


Yes, I guess our compilers don't complain about that anymore.

vnkozlov

So you finally decided to take on JDK-8183288

Essentially you are removing "optimized" VM build with these changes. In this case you need to change make files.

All Statistics flags should be product (which will increase product VM size) - it is important. May be need build's variable --enable-jvm-feature-statistcs to include statistics code on demand.

coleenp · 2024-04-02T17:58:47Z

The optimized build still works as before (actually surprised it still builds). Since for a long time the notproduct options acted like develop options, they still do just the same for the optimized build.

For optimized, all the develop and notproduct options are materialized. Now just develop, not distinguishing notproduct from that. The same code enabled in PRODUCT is still enabled. I haven't looked at that removing optimized bug in a while.

vnkozlov · 2024-04-02T18:38:36Z

For optimized, all the develop and notproduct options are materialized.

Okay, I see what you did here. You touched only flags declaration and did not #ifndef PRODUCT which guards statistics code, for example. Optimized VM build will get that code but will not include DEBUG_ONLY and #ifdef ASSERT guarded code.

So we still need to be careful when we use #ifndef PRODUCT and #ifdef ASSERT.

vnkozlov

Good.

coleenp · 2024-04-02T18:42:17Z

Yes, I had to remember what optimized did. It gets all the options, but builds with optimization and doesn't turn on asserts. I only removed the notproduct flag distinction since it hasn't been distinct for years and it's confusing what we actually wanted it to do.

There was some printing code that we used to not materialize in PRODUCT, that eventually a lot of it was then changed to be materialized with PRODUCT builds. But still if you want to only include some printing in non PRODUCT builds, you have to use the #ifndef PRODUCT, and the flag around it either can be inside the not PRODUCT or will just do nothing.

I'm going to add more: When I tried to make notproduct not materialize the option in PRODUCT, there were many compiler flags that failed (ie tested in product), like PrintOpto, PrintOptoInlining, OptoBreakpointOSR .. and some more.

vnkozlov · 2024-04-02T18:59:27Z

Long ago, before JDK-8024545 we did not have not_product flags declared in product build. Only debug flags were declared as constant. We relied on that change since then.
That is why you may see the issue with not materialized flags in product build.

kimbarrett

Looks good. One minor nit.

Also, it seems that develop and nonproduct (before this change) flags have
associated JVMFlag objects even in product builds. (The function
JVMFlag::is_constant_in_binary is evidence for this. I didn't dig through all
the code to verify it.) Probably if one were going to retain nonproduct
options and make them work "properly", they would only have JVMFlag objects in
non-product builds. But it's not obvious to me why we would want such objects
for either category in product builds. I think any change along that line
should be a separate followup.

kimbarrett · 2024-04-02T17:58:16Z

test/hotspot/jtreg/runtime/CommandLine/VMOptionWarning.java

        output = new OutputAnalyzer(pb.start());
        output.shouldNotHaveExitValue(0);
-        output.shouldContain("Error: VM option 'CheckCompressedOops' is notproduct and is available only in debug version of VM.");
+        output.shouldContain("Error: VM option 'CheckCompressedOops' is develop and is available only in debug version of VM.");


Seems like we don't need this test of the develop option CheckCompressedOops at all, since we have
the immediately preceding test of the develop option VerifyStack.

You're right, we've already tested this.

coleenp · 2024-04-02T19:51:03Z

Thanks for reviewing, Kim. Is your suggestion to not have a JVMFlag object for develop flags in PRODUCT builds? Presumably to save some footprint? I'm not sure we would win fighting the macros to accomplish this.

kimbarrett · 2024-04-02T20:18:34Z

Thanks for reviewing, Kim. Is your suggestion to not have a JVMFlag object for develop flags in PRODUCT builds? Presumably to save some footprint? I'm not sure we would win fighting the macros to accomplish this.

Yes, that's the suggestion and the rationale for it. It should also remove the need for is_constant_in_binary. I don't
know how hard it would actually be to accomplish this. I agree it might not be worth the effort, but we won't know
until someone looks, which I haven't done. It might even be easy.

iklam · 2024-04-02T22:05:40Z

Thanks for reviewing, Kim. Is your suggestion to not have a JVMFlag object for develop flags in PRODUCT builds? Presumably to save some footprint? I'm not sure we would win fighting the macros to accomplish this.

Yes, that's the suggestion and the rationale for it. It should also remove the need for is_constant_in_binary. I don't know how hard it would actually be to accomplish this. I agree it might not be worth the effort, but we won't know until someone looks, which I haven't done. It might even be easy.

Currently the VM prints an error message for non-product flags, so we need to keep some information about them. We can probably skip the type information, etc, to save a little space, but the space saving would be minimal.

$ java -XX:+LoomDeoptAfterThaw --version
Error: VM option 'LoomDeoptAfterThaw' is develop and is available only in debug version of VM.
Improperly specified VM option 'LoomDeoptAfterThaw'
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.

coleenp · 2024-04-03T12:20:21Z

Thanks for the reviews, Ioi, Vladimir, Kim and Stefan.
/integrate

openjdk · 2024-04-03T12:21:13Z

Going to push as commit bea493b.
Since your change was applied there have been 49 commits pushed to the master branch:

80c54b4: 8328932: Parallel: Proper partial object setup in fill_dense_prefix_end
d954f3b: 8329493: Parallel: Remove unused ParallelArguments::heap_max_size_bytes
bdd9438: 8328647: TestGarbageCollectorMXBean.java fails with C1-only and -Xcomp
e3e6c2a: 8328278: Do not print the tenuring threshold in AgeTable::print_on
16b842a: 8329355: Test compiler/c2/irTests/TestIfMinMax.java fails on RISC-V
92f5c0b: 8323682: C2: guard check is not generated in Arrays.copyOfRange intrinsic when allocation is eliminated by EA
866e7b6: 8329174: update CodeBuffer layout in comment after constants section moved
f88f31d: 8328137: PreserveAllAnnotations can cause failure of class retransformation
021ed6a: 8328648: Remove applet usage from JFileChooser tests bug4150029
3057dde: 8329421: Native methods can not be selectively printed
... and 39 more: https://git.openjdk.org/jdk/compare/991e04e7d7ff3ee682c2b8a9b860d325a176e7a5...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-03T12:21:20Z

@coleenp Pushed as commit bea493b.

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

8236736: Change notproduct JVM flags to develop flags

b32fbf7

openjdk bot added graal graal-dev@openjdk.org serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Mar 28, 2024

Missed one.

c3d9a1c

coleenp marked this pull request as ready for review April 2, 2024 13:18

openjdk bot added the rfr Pull request is ready for review label Apr 2, 2024

iklam suggested changes Apr 2, 2024

View reviewed changes

Clean up notproduct from tests.

19b8f6b

iklam approved these changes Apr 2, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 2, 2024

stefank reviewed Apr 2, 2024

View reviewed changes

coleenp commented Apr 2, 2024

View reviewed changes

Fix a couple issues pointed out by Stefank.

00a241d

vnkozlov suggested changes Apr 2, 2024

View reviewed changes

vnkozlov approved these changes Apr 2, 2024

View reviewed changes

kimbarrett approved these changes Apr 2, 2024

View reviewed changes

Remove redundant test case.

3b5002d

openjdk bot added the integrated Pull request has been integrated label Apr 3, 2024

openjdk bot closed this Apr 3, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 3, 2024

coleenp deleted the remove-notproduct branch April 3, 2024 12:22

8236736: Change notproduct JVM flags to develop flags #18541

8236736: Change notproduct JVM flags to develop flags #18541

Uh oh!

Conversation

coleenp commented Mar 28, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Mar 28, 2024

Uh oh!

openjdk bot commented Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Mar 28, 2024

Uh oh!

mlbridge bot commented Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

iklam left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp commented Apr 2, 2024

Uh oh!

iklam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp commented Apr 2, 2024

Uh oh!

vnkozlov commented Apr 2, 2024

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp commented Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vnkozlov commented Apr 2, 2024

Uh oh!

kimbarrett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coleenp commented Apr 2, 2024

Uh oh!

kimbarrett commented Apr 2, 2024

Uh oh!

iklam commented Apr 2, 2024

Uh oh!

coleenp commented Apr 3, 2024

Uh oh!

openjdk bot commented Apr 3, 2024

Uh oh!

openjdk bot commented Apr 3, 2024

Uh oh!

coleenp commented Mar 28, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Mar 28, 2024 •

edited

Loading

mlbridge bot commented Apr 2, 2024 •

edited

Loading

coleenp commented Apr 2, 2024 •

edited

Loading