8333749: Consolidate ConstantDesc conversion in java.base #19585

liach · 2024-06-06T18:35:01Z

In java.base, especially in bytecode generators, we have many different methods converting known valid Class and MethodType into ClassDesc and MethodTypeDesc. These conversions should be consolidated into the same utility method for the ease of future maintenance.

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-8333749: Consolidate ConstantDesc conversion in java.base (Enhancement - P4)

Reviewers

Claes Redestad (@cl4es - Reviewer)
Jorn Vernee (@JornVernee - Reviewer) ⚠️ Review applies to e9ddc398

Contributors

Claes Redestad <redestad@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19585

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-06-06T18:35:35Z

👋 Welcome back liach! 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-06-06T18:36:44Z

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

8333749: Consolidate ConstantDesc conversion in java.base

Co-authored-by: Claes Redestad <redestad@openjdk.org>
Reviewed-by: redestad, jvernee

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

cf677c9: 8333823: Update --release 23 symbol information for JDK 23 build 26
18e7d7b: 8333716: Shenandoah: Check for disarmed method before taking the nmethod lock
c37d02a: 8312412: Uninitialized klassVtable::_verify_count field
17bd483: 8333680: com/sun/tools/attach/BasicTests.java fails with "SocketException: Permission denied: connect"
512b2b4: 8330420: Inverted use of DisplayVMOutputToStderr in ostream_exit
8e72d7c: 8320448: Accelerate IndexOf using AVX2
25ad862: 8332161: Test restoring echo in the Console implementation (java.base)
ee82346: 8325984: 4 jcstress tests are failing in Tier6 4 times each
d744059: 8333774: Avoid eagerly loading various EmptySpliterator classes
d130d2f: 8333477: Delete extra empty spaces in Makefiles
... and 11 more: https://git.openjdk.org/jdk/compare/6238bc8da2abe7a1f0cdd98c0af01e9ba1869ec3...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@cl4es, @JornVernee) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2024-06-06T18:37:07Z

@liach The following label will be automatically applied to this pull request:

core-libs

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

mlbridge · 2024-06-06T18:40:03Z

Webrevs

cl4es

Looks good to me. Probably should get someone to OK changes in foreign/abi (@JornVernee perhaps?).

There are some pre-existing places where we call ReferenceClassDescImpl.ofValidated directly that could probably be switched over to ConstantUtils.classDesc for slightly nicer code. Or - if it matters - add a referenceClassDesc which avoids the type.isPrimitive test.

cl4es · 2024-06-06T18:48:37Z

src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java

@@ -249,14 +253,14 @@ private static Class<?> newProxyClass(Class<?> intfc) {

            // the field name holding the method handle for this method
            String fieldName = "m" + count++;
-            var mt = methodType(m.getReturnType(), JLRA.getExecutableSharedParameterTypes(m), true);
+            var mt = methodDesc(m.getReturnType(), JLRA.getExecutableSharedParameterTypes(m));


Suggested change

var mt = methodDesc(m.getReturnType(), JLRA.getExecutableSharedParameterTypes(m));

var md = methodDesc(m.getReturnType(), JLRA.getExecutableSharedParameterTypes(m));

cl4es · 2024-06-06T18:56:51Z

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java

-                                    .wrapperType()
-                                    .describeConstable()
-                                    .orElseThrow());
+                            cb.instanceOf(classDesc(Wrapper.forBasicType(classLabel)


I have a patch somewhere to cache the wrapper class desc in sun.invoke.util.Wrapper, both as a micro-optimization and to help disambigutate the unfortunately named (my bad) Wrapper::classDescriptor method. Maybe we can roll that into this..?

Feel free, or you can get your patch merged first and then I base off yours if yours is ready.

Created a PR you could fold into this, if you'd like: liachmodded#1

cl4es · 2024-06-06T19:01:02Z

src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java

@@ -466,9 +468,9 @@ private void doBindings(List<Binding> bindings) {
                case Copy copy                   -> emitCopyBuffer(copy);
                case Allocate allocate           -> emitAllocBuffer(allocate);
                case BoxAddress boxAddress       -> emitBoxAddress(boxAddress);
-                case SegmentBase unused          -> emitSegmentBase();
+                case SegmentBase _               -> emitSegmentBase();


Seem a bit too far detached from the changes at hand for a drive-by code cleanup?

My uncontrollable urges to fix these minor IDE warnings... Please allow these fixes, as I have similar changes in ProxyGenerator as well.

src/java.base/share/classes/jdk/internal/constant/ConstantUtils.java

liach · 2024-06-06T19:43:46Z

There are some pre-existing places where we call ReferenceClassDescImpl.ofValidated directly that could probably be switched over to ConstantUtils.classDesc for slightly nicer code. Or - if it matters - add a referenceClassDesc which avoids the type.isPrimitive test.

I think the main advantage of ofValidated is that it avoids descriptor string computation from class objects and should be kept in performance-sensitive code.

referenceClassDesc would be feasible for less performance-demanding code's initialization and known safe conversions (a few APIs in CF API expects non-primitive CDs already). But should its implementation be ofValidated or ofValidatedBinaryName (same question for ClassDesc)? We used ofValidatedBinaryName here instead of a toClassDesc (which calls ofValidated via descriptorString)

…Name to ConstantUtils.binaryNameToDesc

…consolidate-todesc

ExE-Boss · 2024-06-07T09:26:10Z

This could probably also update Class::describeConstable() to use ReferenceClassDesc::ofValidated(String) for non‑hidden reference types:

jdk/src/java.base/share/classes/java/lang/Class.java

Lines 4709 to 4713 in 40b2fbd

    
            public Optional<ClassDesc> describeConstable() { 
        
                Class<?> c = isArray() ? elementType() : this; 
        
                return c.isHidden() ? Optional.empty() 
        
                                    : Optional.of(ClassDesc.ofDescriptor(descriptorString())); 
        
           }

cl4es

Good incremental improvements.

liach · 2024-06-07T17:09:35Z

/contributor add redestad

openjdk · 2024-06-07T17:10:16Z

@liach
Contributor Claes Redestad <redestad@openjdk.org> successfully added.

ExE-Boss · 2024-06-07T18:28:00Z

ClassDesc::of(String), ClassDesc::of(String, String), and ClassDesc::ofInternalName(String) can probably call ReferenceClassDescImpl::ofValidated(String) directly as those methods already perform validation before forwarding to ClassDesc::ofDescriptor(String).

liach · 2024-06-07T19:18:47Z

No: they are missing checks against initial, trailing, or consecutive . or / separators. We can do that in a future patch, with proper test additions.

liach · 2024-06-07T22:03:13Z

Thanks for the reviews!

/integrate

openjdk · 2024-06-07T22:04:15Z

@liach
Your change (at version d3938b4) is now ready to be sponsored by a Committer.

cl4es · 2024-06-08T13:04:50Z

/sponsor

openjdk · 2024-06-08T13:05:37Z

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

a6fc2f8: 8333412: [s390x] Add support for branch on count instruction
cf677c9: 8333823: Update --release 23 symbol information for JDK 23 build 26
18e7d7b: 8333716: Shenandoah: Check for disarmed method before taking the nmethod lock
c37d02a: 8312412: Uninitialized klassVtable::_verify_count field
17bd483: 8333680: com/sun/tools/attach/BasicTests.java fails with "SocketException: Permission denied: connect"
512b2b4: 8330420: Inverted use of DisplayVMOutputToStderr in ostream_exit
8e72d7c: 8320448: Accelerate IndexOf using AVX2
25ad862: 8332161: Test restoring echo in the Console implementation (java.base)
ee82346: 8325984: 4 jcstress tests are failing in Tier6 4 times each
d744059: 8333774: Avoid eagerly loading various EmptySpliterator classes
... and 12 more: https://git.openjdk.org/jdk/compare/6238bc8da2abe7a1f0cdd98c0af01e9ba1869ec3...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-06-08T13:05:44Z

@cl4es @liach Pushed as commit 8d2f9e5.

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

liach added 2 commits June 6, 2024 12:21

Consolidate class/mt to desc operations

4026687

Missed license header

cb4f6d1

openjdk bot added the rfr Pull request is ready for review label Jun 6, 2024

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jun 6, 2024

cl4es approved these changes Jun 6, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 6, 2024

mt -> md (desc)

3e28306

JornVernee reviewed Jun 6, 2024

View reviewed changes

src/java.base/share/classes/jdk/internal/constant/ConstantUtils.java Outdated Show resolved Hide resolved

liach added 2 commits June 6, 2024 15:53

Add referenceClassDesc, move ReferenceClassDescImpl.ofValidatedBinary…

45ba938

…Name to ConstantUtils.binaryNameToDesc

Merge branch 'master' of https://github.com/openjdk/jdk into cleanup/…

89bc5d8

…consolidate-todesc

cl4es and others added 2 commits June 7, 2024 07:21

Wrapper.basic/wrapperClassDescriptor (#1)

69af091

One other place can use wrapperClassDescriptor

e9ddc39

JornVernee approved these changes Jun 7, 2024

View reviewed changes

ExE-Boss review, go through validated path in Class

d3938b4

cl4es approved these changes Jun 7, 2024

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Jun 7, 2024

openjdk bot added the integrated Pull request has been integrated label Jun 8, 2024

openjdk bot closed this Jun 8, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jun 8, 2024

liach mentioned this pull request Jun 20, 2024

8333854: IllegalAccessError with proxies after JDK-8332457 #19815

Closed

3 tasks

liach deleted the cleanup/consolidate-todesc branch July 11, 2024 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8333749: Consolidate ConstantDesc conversion in java.base #19585

8333749: Consolidate ConstantDesc conversion in java.base #19585

liach commented Jun 6, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 6, 2024

openjdk bot commented Jun 6, 2024 •

edited

Loading

openjdk bot commented Jun 6, 2024

mlbridge bot commented Jun 6, 2024 •

edited

Loading

cl4es left a comment

cl4es Jun 6, 2024

cl4es Jun 6, 2024

liach Jun 6, 2024

cl4es Jun 7, 2024

cl4es Jun 6, 2024

liach Jun 6, 2024

liach commented Jun 6, 2024

ExE-Boss commented Jun 7, 2024

cl4es left a comment

liach commented Jun 7, 2024

openjdk bot commented Jun 7, 2024

ExE-Boss commented Jun 7, 2024

liach commented Jun 7, 2024

liach commented Jun 7, 2024

openjdk bot commented Jun 7, 2024

cl4es commented Jun 8, 2024

openjdk bot commented Jun 8, 2024

openjdk bot commented Jun 8, 2024

	var mt = methodDesc(m.getReturnType(), JLRA.getExecutableSharedParameterTypes(m));
	var md = methodDesc(m.getReturnType(), JLRA.getExecutableSharedParameterTypes(m));

8333749: Consolidate ConstantDesc conversion in java.base #19585

8333749: Consolidate ConstantDesc conversion in java.base #19585

Conversation

liach commented Jun 6, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Contributors

Reviewing

Webrev

bridgekeeper bot commented Jun 6, 2024

openjdk bot commented Jun 6, 2024 • edited Loading

openjdk bot commented Jun 6, 2024

mlbridge bot commented Jun 6, 2024 • edited Loading

Webrevs

cl4es left a comment

Choose a reason for hiding this comment

cl4es Jun 6, 2024

Choose a reason for hiding this comment

cl4es Jun 6, 2024

Choose a reason for hiding this comment

liach Jun 6, 2024

Choose a reason for hiding this comment

cl4es Jun 7, 2024

Choose a reason for hiding this comment

cl4es Jun 6, 2024

Choose a reason for hiding this comment

liach Jun 6, 2024

Choose a reason for hiding this comment

liach commented Jun 6, 2024

ExE-Boss commented Jun 7, 2024

cl4es left a comment

Choose a reason for hiding this comment

liach commented Jun 7, 2024

openjdk bot commented Jun 7, 2024

ExE-Boss commented Jun 7, 2024

liach commented Jun 7, 2024

liach commented Jun 7, 2024

openjdk bot commented Jun 7, 2024

cl4es commented Jun 8, 2024

openjdk bot commented Jun 8, 2024

openjdk bot commented Jun 8, 2024

liach commented Jun 6, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jun 6, 2024 •

edited

Loading

mlbridge bot commented Jun 6, 2024 •

edited

Loading