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

8327636: [lworld] Make primitive wrappers be value class if preview feature is enabled #1040

Closed
wants to merge 3 commits into from

Conversation

mlchung
Copy link
Member

@mlchung mlchung commented Mar 7, 2024

As specified in JEP 401, when preview features are enabled, the following standard library classes are considered to be value classes:

    java.lang.Byte
    java.lang.Short
    java.lang.Integer
    java.lang.Long
    java.lang.Float
    java.lang.Double
    java.lang.Boolean
    java.lang.Character
    java.util.Optional
    java.lang.Number
    java.lang.Record

These value classes are loaded only if --enable-preview is set. All valhalla tests should have preview enabled to ensure testing with the proper set of library classes.

Testing: Run jdk_valhalla and hotspot_valhalla_runtime with -Xint with and without --enable-preview. The result is pretty good. JDK-8327637 and JDK-8327639 need further investigation.


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8327636: [lworld] Make primitive wrappers be value class if preview feature is enabled (Bug - P3)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1040

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

Using diff file

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

Webrev

Link to Webrev Comment

Sorry, something went wrong.

…eature is enabled
@bridgekeeper
Copy link

bridgekeeper bot commented Mar 7, 2024

👋 Welcome back mchung! 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.

@openjdk
Copy link

openjdk bot commented Mar 7, 2024

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

8327636: [lworld] Make primitive wrappers be value class if preview feature is enabled

Reviewed-by: rriggs

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

  • 9c21d4c: 8326990: [lworld] make value classes a preview feature
  • 676fe48: 8327498: [lworld] Remove Q-descriptor change from ASM and other clean up

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.

@mlbridge
Copy link

mlbridge bot commented Mar 7, 2024

Webrevs

Copy link
Member

@liach liach left a comment

Choose a reason for hiding this comment

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

https://bugs.openjdk.org/browse/JDK-8327481 can be closed as a duplicate of this.

java/lang/Boolean.java \
java/lang/Character.java \
java/lang/Number.java \
java/lang/Record.java \
Copy link
Member

Choose a reason for hiding this comment

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

Record is already included in the end of the list.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Fixed.

@@ -208,6 +208,13 @@ public static boolean isClassAccessible(Class<?> refc,
return true;
}

// exports are not setup during early VM initialization
if (!jdk.internal.misc.VM.isModuleSystemInited()) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we merge this and the lookupModule == null early startup cases, and assert lookupModule == refModule && refModule == Object.class.getModule() can become assert lookupModule == refModule && (refModule == null || refModule == Object.class.getModule())?

Copy link
Member Author

Choose a reason for hiding this comment

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

It can be simpler:

            // early VM startup case, java.base not defined or module system not initialized
            if (lookupModule == null || !jdk.internal.misc.VM.isModuleSystemInited()) {
                assert lookupModule == refModule;
                return true;
            }

Copy link
Member

Choose a reason for hiding this comment

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

Can Class::getModule ever return null after module system is initialized? If not we can omit the lookupModule == null check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can Class::getModule ever return null after module system is initialized?

No.

lookupModule == null when module system is being initialized before java.base is defined. So the null check needs to be there.

Copy link
Collaborator

@RogerRiggs RogerRiggs left a comment

Choose a reason for hiding this comment

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

LGTM

@mlchung
Copy link
Member Author

mlchung commented Mar 8, 2024

/integrate

@openjdk
Copy link

openjdk bot commented Mar 8, 2024

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

  • 9c21d4c: 8326990: [lworld] make value classes a preview feature
  • 676fe48: 8327498: [lworld] Remove Q-descriptor change from ASM and other clean up

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label Mar 8, 2024
@openjdk openjdk bot closed this Mar 8, 2024
@openjdk openjdk bot removed ready rfr labels Mar 8, 2024
@openjdk
Copy link

openjdk bot commented Mar 8, 2024

@mlchung Pushed as commit 3ee4244.

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

@mlchung mlchung deleted the JDK-8327636 branch April 3, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants