8341755: Optimize argNames in InnerClassLambdaMetafactory by wenshao · Pull Request #21399 · openjdk/jdk

wenshao · 2024-10-08T00:47:27Z

A simple optimization that eliminates the allocation of the MethodTypeDescImpl object when parameterCount is equal to 0 and eliminates the allocation of argNames when parameterCount is equal to 1

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-8341755: Optimize argNames in InnerClassLambdaMetafactory (Enhancement - P4)

Reviewers

Chen Liang (@liach - Reviewer)
Claes Redestad (@cl4es - Reviewer) Review applies to 191bba08

Contributors

Chen Liang <liach@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21399

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-08T00:47:58Z

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

liach · 2024-10-08T01:13:02Z

argNames[i] is always "arg$" + (i + 1). Maybe we can change all argNames[x] usage to a method like:

public static String argName(int i) {
    if (i < 8)
        return ARG_NAME_CACHE[i];
    return "arg$" + (i + 1);
}

openjdk · 2024-10-08T01:23:56Z

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

8341755: Optimize argNames in InnerClassLambdaMetafactory

Co-authored-by: Chen Liang <liach@openjdk.org>
Reviewed-by: liach, 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 3 new commits pushed to the master branch:

de90204: 8341588: Remove CollectionUsageThreshold.java from ProblemList-Xcomp for debugging
f276f58: 8341803: ProblemList containers/docker/TestJcmdWithSideCar.java on linux-x64
7eab0a5: 8337066: Repeated call of StringBuffer.reverse with double byte string returns wrong result

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

openjdk · 2024-10-08T01:25:20Z

@wenshao 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.

…_lamba_meta_factory_202410

wenshao · 2024-10-08T15:15:15Z

/contributor add liach

openjdk · 2024-10-08T15:16:36Z

@wenshao
Contributor Chen Liang <liach@openjdk.org> successfully added.

liach · 2024-10-08T15:19:04Z

Can we change the name to "Optimize argNames in InnerClassLambdaMetafactory"?

mlbridge · 2024-10-08T15:33:12Z

Webrevs

liach

Looks good. Since I have contributed I think we need another review, such as @cl4es.

/reviewers 2 reviewer

openjdk · 2024-10-08T16:00:43Z

@liach
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

cl4es

Looks reasonable

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

…etafactory.java Co-authored-by: Claes Redestad <claes.redestad@oracle.com>

liach

Trivial merge.

/reviewers 1 reviewer

openjdk · 2024-10-08T23:18:44Z

@liach
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 1 (with at least 1 Reviewer).

cl4es · 2024-10-09T03:47:25Z

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

+    }
+
+    private static String argName(int i) {
+        String argName = i < ARG_NAME_CACHE_SIZE ? ARG_NAME_CACHE[i] : null;


Just an idea, but perhaps could constant init ARG_NAME_CACHE = { "arg$0", "arg$1", .. , "arg$7" }; and simplify this?

liach

Back to precomputed constants :)

wenshao · 2024-10-09T05:16:20Z

Back to precomputed constants :)

Manual shift computation! The code is also simplified based on precomputed constants.

wenshao · 2024-10-09T23:49:10Z

/integrate

openjdk · 2024-10-09T23:50:19Z

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

172f744: 8340985: Open source some Desktop related tests
e7045e9: 8341684: Typo in External Specifications link of java.util.Currency
49c7148: 8341366: Suspicious check in Locale.getDisplayName(Locale inLocale)
52eded4: 8341170: Open source several Choice related tests (part 2)
a45abf1: 8341860: ProblemList applications/ctw/modules/java_base_2.java on linux-x64
593c27e: 8341535: sun/awt/font/TestDevTransform.java fails with RuntimeException: Different rendering
3180aaa: 8341832: Incorrect continuation address of synthetic SIGSEGV for APX in product builds
3ab519f: 8341424: GHA: Collect hs_errs from build time failures
a24525b: 8339320: Optimize ClassFile Utf8EntryImpl#inflate
fcc9c8d: 8341854: Incorrect clearing of ZF in fast_unlock_lightweight on x86
... and 15 more: https://git.openjdk.org/jdk/compare/62acc9c174f23acd2807c8214ffc28d73799da16...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-09T23:50:31Z

@wenshao Pushed as commit c850ecb.

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

optimize InnerClassLambdaMetafactory init

5669121

openjdk bot added the core-libs core-libs-dev@openjdk.org label Oct 8, 2024

wenshao and others added 3 commits October 8, 2024 10:03

suggestion from @liach

1a00315

Merge remote-tracking branch 'upstream/master' into optim_inner_class…

a83fe06

…_lamba_meta_factory_202410

Use global cache, don't need per-instance array

a28f856

wenshao changed the title ~~Optimize InnerClassLambdaMetafactory initialization~~ Optimize argNames in InnerClassLambdaMetafactory Oct 8, 2024

wenshao changed the title ~~Optimize argNames in InnerClassLambdaMetafactory~~ 8341755: Optimize argNames in InnerClassLambdaMetafactory Oct 8, 2024

openjdk bot added the rfr Pull request is ready for review label Oct 8, 2024

Lazy initialization of ARG_NAME_CACHE

9624433

liach approved these changes Oct 8, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 8, 2024

openjdk bot removed the ready Pull request is ready to be integrated label Oct 8, 2024

cl4es approved these changes Oct 8, 2024

View reviewed changes

src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java Outdated Show resolved Hide resolved

openjdk bot added the ready Pull request is ready to be integrated label Oct 8, 2024

Update src/java.base/share/classes/java/lang/invoke/InnerClassLambdaM…

191bba0

…etafactory.java Co-authored-by: Claes Redestad <claes.redestad@oracle.com>

openjdk bot removed the ready Pull request is ready to be integrated label Oct 8, 2024

liach approved these changes Oct 8, 2024

View reviewed changes

cl4es approved these changes Oct 8, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 8, 2024

wenshao and others added 2 commits October 9, 2024 06:10

Merge branch 'master' into optim_inner_class_lamba_meta_factory_202410

6cdc01a

fix merge error

f905cc6

liach approved these changes Oct 8, 2024

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Oct 8, 2024

openjdk bot added the ready Pull request is ready to be integrated label Oct 8, 2024

cl4es reviewed Oct 9, 2024

View reviewed changes

suggestion from @cl4es

fc8b71e

openjdk bot removed the ready Pull request is ready to be integrated label Oct 9, 2024

suggestion from @cl4es

3d69661

liach approved these changes Oct 9, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 9, 2024

openjdk bot added the integrated Pull request has been integrated label Oct 9, 2024

openjdk bot closed this Oct 9, 2024

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

Conversation

wenshao commented Oct 8, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Contributors

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Oct 8, 2024

Uh oh!

liach commented Oct 8, 2024

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Oct 8, 2024

Uh oh!

wenshao commented Oct 8, 2024

Uh oh!

openjdk bot commented Oct 8, 2024

Uh oh!

liach commented Oct 8, 2024

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Oct 8, 2024

Uh oh!

cl4es left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Oct 8, 2024

Uh oh!

cl4es Oct 9, 2024

Choose a reason for hiding this comment

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

wenshao commented Oct 9, 2024

Uh oh!

wenshao commented Oct 9, 2024

Uh oh!

openjdk bot commented Oct 9, 2024

Uh oh!

openjdk bot commented Oct 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

wenshao commented Oct 8, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 8, 2024 •

edited

Loading

mlbridge bot commented Oct 8, 2024 •

edited

Loading