8300237: Minor improvements in MethodHandles #12025

stsypanov · 2023-01-17T08:22:28Z

MethodType.ptypes() can be used instead of MethodType.parameterList() when we don't need a copy
comparison of two lists can be done without Stream.reduce()

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-8300237: Minor improvements in MethodHandles

Reviewers

Claes Redestad (@cl4es - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12025

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

Using diff file

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

bridgekeeper · 2023-01-17T08:23:45Z

👋 Welcome back stsypanov! 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 · 2023-01-17T08:27:29Z

@stsypanov 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 · 2023-01-17T08:31:01Z

Webrevs

cl4es · 2023-01-17T09:51:31Z

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

-        final List<Class<?>> empty = List.of();
-        return lists.stream().reduce((p, q) -> p.size() >= q.size() ? p : q).orElse(empty);
+                        map(MethodType::ptypes).
+                        reduce((p, q) -> p.length >= q.length ? p : q).orElse(EMPTY);


Could avoid the need to introduce EMPTY if you make the stream expression return the longest list directly:

reduce((p, q) -> { var longest = (p.size() >= q.size()) ? p : q; return List.of(Arrays.copyOfRange(longest, skipSize, longest.size()); // checking isEmpty() is redundant here since we filter on `t.parameterCount() > skipSize` }).orElse(List.of());

Using lambdas inside MethodHandles is quite dangerous given that lambdas are initialized using method handles. It may work now because longuestParameterList() is not called when initializing a lambda but it may make any changes in the implementation of lambdas painfull in the future.

Precious little method handle use in lambda bootstrap since JDK 11. Though I agree with the sentiment - having fixed a number of bootstrap issues in the past - MethodHandles is a small step up the abstraction ladder and the code in particular already uses a number of method refs and lambdas.

ok, two small changes,

formatting: usually the method call in a stream are aligned with the '.' at the beginning

stream .filter(...) .map(...)

instead of at the end.

the reduce is a max(),
max(Comparator.comparingInt(List::size))

@forax formatting is fixed. As of max() I think we can improve this even more by hoisting max() before calling ptypes()

cl4es

Remove EMPTY (using my earlier suggestion or by simply inlining new Class<?>[0]) and I think this looks OK if framed as a code cleanup. The affected code isn't very performance sensitive as it's only used in the setup for MHs.loop combinators, which are likely to be rare in practice.

forax · 2023-01-17T20:51:06Z

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

-import java.util.List;
-import java.util.Objects;
-import java.util.Set;
+import java.util.*;


forax · 2023-01-17T20:51:30Z

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

+                .filter(t -> t.parameterCount() > skipSize)
+                .max(Comparator.comparing(MethodType::parameterCount))
+                .map(MethodType::ptypes)
+                .map(longest -> List.of(Arrays.copyOfRange(longest, skipSize, longest.length)))


i think you can fuse these two map() calls

cl4es

LGTM

cl4es · 2023-01-17T22:28:50Z

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

-        return lists.stream().reduce((p, q) -> p.size() >= q.size() ? p : q).orElse(empty);
+                .map(MethodHandle::type)
+                .filter(t -> t.parameterCount() > skipSize)
+                .max(Comparator.comparing(MethodType::parameterCount))


@forax suggested Comparator.comparingInt here, which may or may not help avoid some boxing.

Nice point, done!

openjdk · 2023-01-17T22:33:23Z

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

8300237: Minor improvements in MethodHandles

Reviewed-by: redestad

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

15a9186: 8300169: Build failure with clang-15
d918042: 8300267: Remove duplicated setter/getter for do_not_unlock
7c8b99e: 8300117: Replace use of JNI_COMMIT mode with mode 0
c705673: 8299089: Instrument global jni handles with tag to make them distinguishable
66f7387: 8299074: nmethod marked for deoptimization is not deoptimized
1f438a8: 8282651: ZGC: vmTestbase/gc/ArrayJuggle/ tests fails intermittently with exit code 97
f1194dc: 8300109: RISC-V: Improve code generation for MinI/MaxI nodes
89a032d: 8300002: Performance regression caused by non-inlined hot methods due to post call noop instructions
7071397: 8299224: TestReporterStreams.java has bad indentation for legal header
1d8b87d: 8300321: Use link tags in javax.sql.rowset package-info
... and 204 more: https://git.openjdk.org/jdk/compare/41900b57af084e0cfc1453681b24fe5606e11ab2...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) 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).

forax · 2023-01-18T10:11:13Z

Looks good to me,
i'm not an official reviewer but claes already review this PR so this is fine.

stsypanov · 2023-01-18T11:32:14Z

/integrate

openjdk · 2023-01-18T11:33:55Z

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

cl4es · 2023-01-18T13:28:20Z

/sponsor

openjdk · 2023-01-18T13:56:00Z

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

85d8bac: 8300166: Unused array allocation in ProcessPath.doProcessPath
c464ef1: 8292741: Convert JvmtiTagMapTable to ResourceHashtable
1aded82: 8300488: Incorrect usage of CATCH_BAD_ALLOC as a macro call
bd5ca95: 8300222: Replace NULL with nullptr in share/logging
15a9186: 8300169: Build failure with clang-15
d918042: 8300267: Remove duplicated setter/getter for do_not_unlock
7c8b99e: 8300117: Replace use of JNI_COMMIT mode with mode 0
c705673: 8299089: Instrument global jni handles with tag to make them distinguishable
66f7387: 8299074: nmethod marked for deoptimization is not deoptimized
1f438a8: 8282651: ZGC: vmTestbase/gc/ArrayJuggle/ tests fails intermittently with exit code 97
... and 208 more: https://git.openjdk.org/jdk/compare/41900b57af084e0cfc1453681b24fe5606e11ab2...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-18T13:56:53Z

@cl4es @stsypanov Pushed as commit 754f6e6.

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

stsypanov added 2 commits January 4, 2023 17:49

Improve MH

8f6c470

Fix test

e9e0baa

openjdk bot added the rfr Pull request is ready for review label Jan 17, 2023

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jan 17, 2023

cl4es reviewed Jan 17, 2023

View reviewed changes

cl4es suggested changes Jan 17, 2023

View reviewed changes

Simplify

6d1397c

stsypanov requested review from forax and cl4es and removed request for forax and cl4es January 17, 2023 14:53

Polishing

cab00ae

stsypanov requested review from cl4es and removed request for forax January 17, 2023 18:08

forax reviewed Jan 17, 2023

View reviewed changes

Merge map() calls

f3fc18d

cl4es approved these changes Jan 17, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 17, 2023

Use comparingInt()

8566289

cl4es approved these changes Jan 18, 2023

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Jan 18, 2023

openjdk bot added the integrated Pull request has been integrated label Jan 18, 2023

openjdk bot closed this Jan 18, 2023

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 Jan 18, 2023

stsypanov deleted the mh branch January 18, 2023 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8300237: Minor improvements in MethodHandles #12025

8300237: Minor improvements in MethodHandles #12025

stsypanov commented Jan 17, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 17, 2023

openjdk bot commented Jan 17, 2023

mlbridge bot commented Jan 17, 2023 •

edited

Loading

cl4es Jan 17, 2023 •

edited

Loading

forax Jan 17, 2023

cl4es Jan 17, 2023

forax Jan 17, 2023

stsypanov Jan 17, 2023

cl4es left a comment •

edited

Loading

forax Jan 17, 2023

stsypanov Jan 17, 2023

forax Jan 17, 2023 •

edited

Loading

stsypanov Jan 17, 2023

cl4es left a comment

cl4es Jan 17, 2023

stsypanov Jan 18, 2023

openjdk bot commented Jan 17, 2023 •

edited

Loading

forax commented Jan 18, 2023

stsypanov commented Jan 18, 2023

openjdk bot commented Jan 18, 2023

cl4es commented Jan 18, 2023

openjdk bot commented Jan 18, 2023

openjdk bot commented Jan 18, 2023

8300237: Minor improvements in MethodHandles #12025

8300237: Minor improvements in MethodHandles #12025

Conversation

stsypanov commented Jan 17, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 17, 2023

openjdk bot commented Jan 17, 2023

mlbridge bot commented Jan 17, 2023 • edited Loading

Webrevs

cl4es Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cl4es left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forax Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cl4es left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Jan 17, 2023 • edited Loading

forax commented Jan 18, 2023

stsypanov commented Jan 18, 2023

openjdk bot commented Jan 18, 2023

cl4es commented Jan 18, 2023

openjdk bot commented Jan 18, 2023

openjdk bot commented Jan 18, 2023

stsypanov commented Jan 17, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 17, 2023 •

edited

Loading

cl4es Jan 17, 2023 •

edited

Loading

cl4es left a comment •

edited

Loading

forax Jan 17, 2023 •

edited

Loading

openjdk bot commented Jan 17, 2023 •

edited

Loading