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

8273656: Improve java.lang.invoke.MethodType.parameterList() and its usage #5489

Closed
wants to merge 1 commit into from

Conversation

@stsypanov
Copy link
Contributor

@stsypanov stsypanov commented Sep 13, 2021

Currently the method is implemented like

public List<Class<?>> parameterList() {
  return Collections.unmodifiableList(Arrays.asList(ptypes.clone()));
}

This seems to be excessive, as three objects are allocated here. Instead we can use List.of(ptypes) which doesn't allocate anything for empty array and for one of length 1 and 2 it allocates lightweight objects with 2 fields, still copying longer arrays. This is likely to be fruitful as most of methods have 0-2 parameters.

Also there is a couple of cases when MethodType.parameterLis() is called to get its size, which is excessive either as we can use MethodType.parameterCount() instead.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8273656: Improve java.lang.invoke.MethodType.parameterList() and its usage

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5489/head:pull/5489
$ git checkout pull/5489

Update a local copy of the PR:
$ git checkout pull/5489
$ git pull https://git.openjdk.java.net/jdk pull/5489/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5489

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5489.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 13, 2021

👋 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.

Loading

@openjdk openjdk bot added the rfr label Sep 13, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 13, 2021

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

Loading

@openjdk openjdk bot added the core-libs label Sep 13, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 13, 2021

Webrevs

Loading

Copy link
Member

@JornVernee JornVernee left a comment

LGTM

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 13, 2021

⚠️ @stsypanov the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout 8273656
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 13, 2021

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

8273656: Improve java.lang.invoke.MethodType.parameterList() and its usage

Reviewed-by: jvernee, vlivanov, 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 3 new commits pushed to the master branch:

  • 4cfa230: 8273259: Character.getName doesn't follow Unicode spec for ideographs
  • f9b2507: 8271834: TestStringDeduplicationAgeThreshold intermittent failures on Shenandoah
  • 261cb44: 8273629: compiler/uncommontrap/TestDeoptOOM.java fails with release VMs

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.

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 (@JornVernee, @iwanowww, @mlchung) 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).

Loading

@openjdk openjdk bot added the ready label Sep 13, 2021
@stsypanov
Copy link
Contributor Author

@stsypanov stsypanov commented Sep 13, 2021

/integrate

Loading

@openjdk openjdk bot added the sponsor label Sep 13, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 13, 2021

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

Loading

@JornVernee
Copy link
Member

@JornVernee JornVernee commented Sep 13, 2021

I'm also running a test job for this change. If that comes back green I'll sponsor the PR after the 24 hour review period (in case anybody else wants to comment).

Loading

Copy link

@iwanowww iwanowww left a comment

Looks good.

BTW it can be improved even further by caching the immutable List view of parameters.

Loading

Copy link
Member

@mlchung mlchung left a comment

Looks good to me.

Loading

@JornVernee
Copy link
Member

@JornVernee JornVernee commented Sep 15, 2021

/sponsor

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 15, 2021

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

  • 8fbcc82: 8273494: Zero: Put libjvm.so into "zero" folder, not "server"
  • 92c30c9: 8273599: Remove cross_threshold method usage around GC
  • 02af541: 8273605: VM Exit does not abort concurrent mark
  • febcc72: 8273366: [testbug] javax/swing/UIDefaults/6302464/bug6302464.java fails on macOS12
  • 1017a2c: 8273135: java/awt/color/ICC_ColorSpace/MTTransformReplacedProfile.java crashes in liblcms.dylib with NULLSeek+0x7
  • 6cf70f5: 8273638: javax/swing/JTable/4235420/bug4235420.java fails in GTK L&F
  • e66bf47: 8273414: ResourceObj::operator delete should handle nullptr in debug builds
  • 16c3ad1: 8272574: C2: assert(false) failed: Bad graph detected in build_loop_late
  • e7ab372: 8273641: (bf) Buffer subclasses documentation contains template strings
  • 22a7191: 8273040: Turning off JpAllowDowngrades (or Upgrades)
  • ... and 24 more: https://git.openjdk.java.net/jdk/compare/b0d04976bd334f840cb91e3f6dfa2ea680948a39...master

Your commit was automatically rebased without conflicts.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 15, 2021

@JornVernee @stsypanov Pushed as commit 4c673df.

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

Loading

@stsypanov stsypanov deleted the 8273656 branch Sep 15, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 22, 2021

Mailing list message from John Rose on core-libs-dev:

On Sep 13, 2021, at 10:24 AM, Vladimir Ivanov <vlivanov at openjdk.java.net<mailto:vlivanov at openjdk.java.net>> wrote:

BTW it can be improved even further by caching the immutable List view of parameters.

I would go further: If I were writing MethodType.java today
I would probably use List.of as the backing store for the
parameters, instead of the private array. So ptypes should
be List<Class<?>> not Class<?>[]. I don?t think the footprint
or polymorphism effects would be dealbreakers, and the
code would (I think) be simpler overall. But that?s a messy
change, since the array representation is dug in.

Loading

@cl4es
Copy link
Member

@cl4es cl4es commented Sep 22, 2021

This change introduces a subtle behavior change in that List.of produce a null-hostile list, for example list.contains(null) will throw NPE. Does this need to be spelled out? (FTR I think such null-hostile behavior should be reconsidered)

Loading

@stsypanov
Copy link
Contributor Author

@stsypanov stsypanov commented Sep 22, 2021

@cl4es null-issue was the one I was afraid most of all in this case. I think that the initial array ptypes never contains null elements due to it's business meaning (we cannot have a param without type). Also we do explicit null-check in checkPtypes() method, called from makeImpl().

Loading

@JornVernee
Copy link
Member

@JornVernee JornVernee commented Sep 22, 2021

Mailing list message from John Rose on core-libs-dev:

On Sep 13, 2021, at 10:24 AM, Vladimir Ivanov <vlivanov at openjdk.java.net<mailto:vlivanov at openjdk.java.net>> wrote:

BTW it can be improved even further by caching the immutable List view of parameters.

I would go further: If I were writing MethodType.java today
I would probably use List.of as the backing store for the
parameters, instead of the private array. So ptypes should
be List<Class> not Class[]. I don?t think the footprint
or polymorphism effects would be dealbreakers, and the
code would (I think) be simpler overall. But that?s a messy
change, since the array representation is dug in.

Another thing that makes this change hard is that MethodType is mapped directly in HotSpot, so that HotSpot can retrieve for instance the parameter types [1]. The VM already knows how to dig into arrays, but adding support for List as well seems more cumbersome, since the list internals can change, and AFAIK most VM code can not just call List::get.

[1] : https://github.com/openjdk/jdk/blob/master/src/hotspot/share/classfile/javaClasses.cpp#L4135-L4137

Loading

@cl4es
Copy link
Member

@cl4es cl4es commented Sep 22, 2021

@cl4es null-issue was the one I was afraid most of all in this case. I think that the initial array ptypes never contains null elements due to it's business meaning (we cannot have a param without type). Also we do explicit null-check in checkPtypes() method, called from makeImpl().

No, on the producer side there's no issue since a parameter type can't be null. The compatibility issue I have in mind is only the (very minor) issue that the returned list would now throw NPE if you call mt.parameterList().contains(null) where it before would just return false. I don't think this is a real issue, but the subtle behavior change might warrant a CSR to document the stricter behavior.

Loading

@JornVernee
Copy link
Member

@JornVernee JornVernee commented Sep 22, 2021

The null-hostile-ness of contains is not something I thought about. It would have been good to mention that in the PR body as well. FWIW, I did a search on grep.app for parameterList() calls in java [1] and don't really see any calls to contains.

I think process-wise creating a CSR would be good, and shouldn't be too much work. Though, I'm not familiar with the process of filing a CSR after a patch has already been merged.

[1] : https://grep.app/search?q=.parameterList%28%29&filter[lang][0]=Java

Loading

@cl4es
Copy link
Member

@cl4es cl4es commented Sep 22, 2021

I think it's OK and even expected to file a CSRs retroactively when you realize post integration that there's a behavior change. I recall doing so at least once in the past.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 29, 2021

Mailing list message from Joseph D. Darcy on core-libs-dev:

On 9/22/2021 4:53 PM, Claes Redestad wrote:

On Mon, 13 Sep 2021 11:06:15 GMT, ?????? ??????? <github.com+10835776+stsypanov at openjdk.org> wrote:

Currently the method is implemented like

public List<Class<?>> parameterList() {
return Collections.unmodifiableList(Arrays.asList(ptypes.clone()));
}

This seems to be excessive, as three objects are allocated here. Instead we can use `List.of(ptypes)` which doesn't allocate anything for empty array and for one of length 1 and 2 it allocates lightweight objects with 2 fields, still copying longer arrays. This is likely to be fruitful as most of methods have 0-2 parameters.

Also there is a couple of cases when `MethodType.parameterLis()` is called to get its size, which is excessive either as we can use `MethodType.parameterCount()` instead.
I think it's OK and even expected to file a CSRs retroactively when you realize post integration that there's a behavior change. I recall doing so at least once in the past.

This scenario is discussed in the CSR FAQ:

Q: Timing wise, when do I need to file a CSR request?
A: A CSR request needs to be filed and approved /before/ the
corresponding change is pushed to a JDK release line of development.
In exceptional circumstances, the need for a CSR review may be
recognized only after a push has already occurred. In such cases, a
retroactive CSR review can be conducted. The results of such a
retroactive review may require updates to the change, up to and
including complete removal of the change.

https://wiki.openjdk.java.net/display/csr/CSR+FAQs

For the change in question, are there any interactions with class file
redefinition effects?

-Joe

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants