8271959: [lworld] Convert Optional and VBS classes to value class #816

RogerRiggs · 2023-01-26T01:38:03Z

Make copies of classes to be made value classes in src/java.base/valueclasses/classes
Modify CompileJavaModules.gmk to trigger builds of values classes and construct a .jar for each module.
Add the jar files to $JAVA_HOME/lib/valueclasses/<module>-valueclasses.jar

Modify hotspot arguments.cpp to scan for patch jar files when --enable-preview and -XX:+EnableValhalla.
For each jar, the equivalent of --patch-module = is added and the system properties jdk.module.patch.<n> include jar file paths.

Progress

Change must not contain extraneous whitespace

Issue

JDK-8271959: [lworld] Convert Optional and VBS classes to value class

Reviewers

Erik Joelsson (@erikj79 - no project role) ⚠️ Review applies to 8ed83988
Mandy Chung (@mlchung - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 816

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/816.diff

Make copies of classes to be made value classes in src/java.base/valhalla/classes Modify Main.gmk to trigger Valhalla builds of classes Modify Modules.gmk to conditionally override java.base classes with Valhalla/classes Modify hotspot arguments.cpp to force --patch-module with overriddden classes if --enable-preview

bridgekeeper · 2023-01-26T01:38:48Z

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

vova7878 · 2023-01-26T12:39:00Z

Are these changes part of JEP 402? Will java.lang.Integer etc. replaced in a similar way?

RogerRiggs · 2023-01-26T15:49:53Z

Are these changes part of JEP 402? Will java.lang.Integer etc. replaced in a similar way?

I think that's unlikely, at least as sketched here. The wrapper types are used before the module system is initialized.
Likely, something more specific and low level will be needed for those.

erikj79

There are two major ways you could approach this.

Define new top level targets for compiling the value classes and the subsequent jar files.
Modify make/CompileJavaModules.gmk to add a second optional compilation and jar file generation which would be part of the top level -java target.

You went with option 1. Both have pros and cons, so it kind of depends on what you expect to need to do, as a developer. Do you think you need the fine granular top level target support for selective building?

Another aspect is that if you reuse the CompileJavaModules.gmk and run it again with a different outputdir, I think you will be rebuilding quite a lot of things unnecessarily. My understanding is that you only really need the value classes from the this second round of compilation.

make/Main.gmk

Override classes reside in <module>/valueclasses/classes They are compiled separately after the regular module build. Only the class files compiled from <module>/valueclasses/.. are inserted into a jar file The jar file is placed in lib/valueclasses/<module>-valueclasses.jar The jar is included in the module's jmod The Hotspot arguments.cpp is hard coded to patch the java.base module when the --enable-preview option is selected.

for per module jar files with names like <module>-valueclasses.jar.. For each jar add the equivalent of --patch-module <module>=<jar>.

value classes when --enable-preview

…re classes edited to remove super() and change "class" to "value class". See Modules.gmk for the list of classes. Add test for 16 classes in java.util and java.time to check they are value classes when --enable-preview and otherwise identity classes.

and --enable-preview together. Only a single --patch-module for a given module is legal. The --patch-module options are accumulated in a list, rejecting duplicates. If the --enable-preview is present, jar files for each module with value classes are appended to the path for the respective module. Finally, a property is defined for each module that communicates the module and path to ModuleBootstrap.

Remove "super()" from java.time classes (not needed and also not allowed for value classes)

Cleanup memory allocation in arguments to use the heap (not stack) Add test for duplication of --patch-module other than java.base

openjdk · 2023-02-21T21:31:54Z

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

8271959: [lworld] Convert Optional and VBS classes to value class

Reviewed-by: erikj, mchung

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 4 new commits pushed to the lworld branch:

0baf5d1: 8293800: [lworld] Remove TypeInlineType and related code
5e871a4: 8303137: [lworld] Fix ArchivedEnumTest fail caused by changed Enum static field
21c557a: 8293702: [lworld] C1 does not properly handle unloaded inline type field loads
a578572: 8284164: [lworld] Inline type elements of autobox cache should be casted to non-null

Please see this link for an up-to-date comparison between the source branch of this pull request and the lworld branch.
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 lworld branch, type /integrate in a new comment.

RogerRiggs · 2023-02-21T21:32:27Z

The initial set of value classes (listed in make/modules/java.base/gensrc/GensrcValueClasses.gmk) are generated from the regular versions of those classes with the value keyword inserted in the class declaration.

java/util/Optional.java \
java/util/OptionalInt.java \
java/util/OptionalLong.java \
java/util/OptionalDouble.java \
java/time/LocalDate.java \
java/time/LocalDateTime.java \
java/time/LocalTime.java \
java/time/Duration.java \
java/time/Instant.java \
java/time/MonthDay.java \
java/time/ZonedDateTime.java \
java/time/OffsetDateTime.java \
java/time/OffsetTime.java \
java/time/YearMonth.java \
java/time/Year.java \
java/time/Period.java \

mlbridge · 2023-02-21T21:35:47Z

Webrevs

RogerRiggs · 2023-02-21T21:46:14Z

/labels core-libs, build, hotspot

openjdk · 2023-02-21T21:47:43Z

@RogerRiggs Unknown command labels - for a list of valid commands use /help.

openjdk · 2023-02-21T21:49:32Z

@RogerRiggs
The label core-libs is not a valid label.
The label build is not a valid label.
The label hotspot is not a valid label.
These labels are valid:

openjdk · 2023-02-21T21:51:22Z

@RogerRiggs Available commands:

backport - create a backport
cc - add or remove an additional classification label
clean - Mark the backport pull request as a clean backport
contributor - adds or removes additional contributors for a PR
covered - used when employer has signed the OCA
csr - require a compatibility and specification request (CSR) for this pull request
help - shows this text
integrate - performs integration of the changes in the PR
issue - edit the list of issues that this PR solves
jep - require a JDK Enhancement Proposal (JEP) for this pull request
label - add or remove an additional classification label
open - Set the pull request state to "open"
reviewer - manage additional reviewers for a PR
reviewers - set the number of additional required reviewers for this PR
signed - used after signing the OCA
solves - edit the list of issues that this PR solves
sponsor - performs integration of a PR that is authored by a non-committer
summary - updates the summary in the commit message
test - used to run tests

openjdk · 2023-02-21T21:53:03Z

@RogerRiggs Usage: /label <add|remove> [label[, label, ...]] or /label [<+|->label[, <+|->label, ...]] where label is an additional classification that should be applied to this PR. These labels are valid:

… the launcher instead of ExceptionInInitilizer.

dholmes-ora

Mostly seems reasonable - though I'm not that familiar with the existing module patching logic. One issue with tracking patch_mod_javabase.

src/hotspot/share/runtime/arguments.cpp

…-modules. Other review comment cleanup.

src/hotspot/share/runtime/arguments.cpp

RogerRiggs · 2023-02-23T20:42:55Z

@erikj79 When you have time can you look at the build changes. Thanks

mlchung · 2023-02-24T16:10:11Z

A side note: the change to remove the call to super() from the constructors in the jdk master repo.

erikj79

Except for some minor cosmetics, the build changes are looking really good.

make/CompileJavaModules.gmk

dholmes-ora

Sorry for the delay (I don't get email notifications for this PR).

The hotspot changes are looking good.

One minor nit.

dholmes-ora · 2023-02-27T21:32:57Z

src/hotspot/share/runtime/arguments.cpp

+      if (strcmp(module_name, JAVA_BASE_NAME) == 0) {
+        vm_exit_during_initialization("Cannot specify " JAVA_BASE_NAME " more than once to --patch-module");
+      } else {
+        vm_exit_during_initialization("Cannot specify a module more than once to --patch-module", module_name);
+      }


This just reduces to:

else { vm_exit_during_initialization(err_msg("Cannot specify %s more than once to --patch-module", module_name)); }

or even:

vm_exit_during_initialization("Cannot specify a module more than once to --patch-module", module_name);

Right, for some reason there are several tests that expect different error messages due to the prior implementation split between hotspot argument parsing and ModuleBootstrap.
I couldn't see clearly far enough in the EA/Preview/release cycle to be sure of the final disposition and so punted on correcting everything everywhere.
Eventually, the provision to replace some classes with value classes unnecessary. The classes will become permanent.
But that can only happen after the feature is no-longer preview and is fully integrated.

mlchung

LGTM

RogerRiggs · 2023-03-08T21:58:15Z

/integrate

openjdk · 2023-03-08T21:59:26Z

Going to push as commit 4f71167.
Since your change was applied there have been 4 commits pushed to the lworld branch:

0baf5d1: 8293800: [lworld] Remove TypeInlineType and related code
5e871a4: 8303137: [lworld] Fix ArchivedEnumTest fail caused by changed Enum static field
21c557a: 8293702: [lworld] C1 does not properly handle unloaded inline type field loads
a578572: 8284164: [lworld] Inline type elements of autobox cache should be casted to non-null

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-08T21:59:39Z

@RogerRiggs Pushed as commit 4f71167.

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

erikj79 reviewed Jan 26, 2023

View reviewed changes

make/Main.gmk Outdated Show resolved Hide resolved

RogerRiggs added 9 commits January 28, 2023 15:48

When --enable-preview, scan the $HOME/lib/valueclasses/ directory

01dbcb6

for per module jar files with names like <module>-valueclasses.jar.. For each jar add the equivalent of --patch-module <module>=<jar>.

Added test that Valhalla classes modified to be value classes are

dd7197c

value classes when --enable-preview

Refactor to smaller methods and some cleanup.

5d49127

Merge branch 'lworld' into try-double-build

d962ced

Remove incomplete test ValueClassDepsTest.java.

9c1ec78

Remove "super()" from java.time classes (not needed and also not allowed for value classes)

Cleanup make files generation of value classes

c2033ba

Cleanup memory allocation in arguments to use the heap (not stack) Add test for duplication of --patch-module other than java.base

RogerRiggs changed the title ~~Draft: Valhalla double-build to patch value classes into java.base if enable preview~~ Draft: 8271959 [lworld] Convert Optional and VBS classes to value class Feb 21, 2023

Merge branch 'lworld' into try-double-build

9fc43ba

RogerRiggs changed the title ~~Draft: 8271959 [lworld] Convert Optional and VBS classes to value class~~ 8271959: [lworld] Convert Optional and VBS classes to value class Feb 21, 2023

openjdk bot changed the title ~~8271959: [lworld] Convert Optional and VBS classes to value class~~ 8271959: [lworld] Convert Optional and VBS classes to value class Feb 21, 2023

RogerRiggs marked this pull request as ready for review February 21, 2023 21:29

openjdk bot added ready rfr labels Feb 21, 2023

Modify hotspot test for duplicate modules to check error message from…

15b8caf

… the launcher instead of ExceptionInInitilizer.

dholmes-ora reviewed Feb 22, 2023

View reviewed changes

src/hotspot/share/runtime/arguments.cpp Outdated Show resolved Hide resolved

src/hotspot/share/runtime/arguments.cpp Outdated Show resolved Hide resolved

src/hotspot/share/runtime/arguments.cpp Outdated Show resolved Hide resolved

Replaced patch_mod_javabase with a check for an empty list of --patch…

f7eb632

…-modules. Other review comment cleanup.

dholmes-ora reviewed Feb 23, 2023

View reviewed changes

src/hotspot/share/runtime/arguments.cpp Outdated Show resolved Hide resolved

Restore disabling CDS if java.base is patched.

8ed8398

erikj79 approved these changes Feb 24, 2023

View reviewed changes

make/CompileJavaModules.gmk Outdated Show resolved Hide resolved

make/CompileJavaModules.gmk Show resolved Hide resolved

Correct indentation/space usage in makefiles.

02453b1

dholmes-ora reviewed Feb 27, 2023

View reviewed changes

RogerRiggs added 2 commits February 27, 2023 18:30

Revert unnecessary removals of super() in java.time.format classes.

fffc49e

Revert unnecessary removals of super() in java.time.zone

e9ac22e

mlchung approved these changes Mar 8, 2023

View reviewed changes

openjdk bot added the integrated label Mar 8, 2023

openjdk bot closed this Mar 8, 2023

openjdk bot removed ready rfr labels Mar 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8271959: [lworld] Convert Optional and VBS classes to value class #816

8271959: [lworld] Convert Optional and VBS classes to value class #816

RogerRiggs commented Jan 26, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 26, 2023

vova7878 commented Jan 26, 2023 •

edited

Loading

RogerRiggs commented Jan 26, 2023

erikj79 left a comment

openjdk bot commented Feb 21, 2023 •

edited

Loading

RogerRiggs commented Feb 21, 2023

mlbridge bot commented Feb 21, 2023 •

edited

Loading

RogerRiggs commented Feb 21, 2023

openjdk bot commented Feb 21, 2023

openjdk bot commented Feb 21, 2023

openjdk bot commented Feb 21, 2023

openjdk bot commented Feb 21, 2023

dholmes-ora left a comment

RogerRiggs commented Feb 23, 2023

mlchung commented Feb 24, 2023

erikj79 left a comment

dholmes-ora left a comment

dholmes-ora Feb 27, 2023

RogerRiggs Feb 27, 2023

mlchung left a comment

RogerRiggs commented Mar 8, 2023

openjdk bot commented Mar 8, 2023

openjdk bot commented Mar 8, 2023

8271959: [lworld] Convert Optional and VBS classes to value class #816

8271959: [lworld] Convert Optional and VBS classes to value class #816

Conversation

RogerRiggs commented Jan 26, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 26, 2023

vova7878 commented Jan 26, 2023 • edited Loading

RogerRiggs commented Jan 26, 2023

erikj79 left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 21, 2023 • edited Loading

RogerRiggs commented Feb 21, 2023

mlbridge bot commented Feb 21, 2023 • edited Loading

Webrevs

RogerRiggs commented Feb 21, 2023

openjdk bot commented Feb 21, 2023

openjdk bot commented Feb 21, 2023

openjdk bot commented Feb 21, 2023

openjdk bot commented Feb 21, 2023

dholmes-ora left a comment

Choose a reason for hiding this comment

RogerRiggs commented Feb 23, 2023

mlchung commented Feb 24, 2023

erikj79 left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora Feb 27, 2023

Choose a reason for hiding this comment

RogerRiggs Feb 27, 2023

Choose a reason for hiding this comment

mlchung left a comment

Choose a reason for hiding this comment

RogerRiggs commented Mar 8, 2023

openjdk bot commented Mar 8, 2023

openjdk bot commented Mar 8, 2023

RogerRiggs commented Jan 26, 2023 •

edited by openjdk bot

Loading

vova7878 commented Jan 26, 2023 •

edited

Loading

openjdk bot commented Feb 21, 2023 •

edited

Loading

mlbridge bot commented Feb 21, 2023 •

edited

Loading