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

8294698: Remove unused 'checkedExceptions' param from MethodAccessorGenerator.generateMethod() #10526

Closed
wants to merge 1 commit into from

Conversation

stsypanov
Copy link
Contributor

@stsypanov stsypanov commented Oct 2, 2022

checkedExceptions param of MethodAccessorGenerator.generateMethod() is unused and should be removed in order to prevent allocations from Method.getExceptionTypes()


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-8294698: Remove unused 'checkedExceptions' param from MethodAccessorGenerator.generateMethod()

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10526

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 2, 2022

👋 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 openjdk bot added the rfr Pull request is ready for review label Oct 2, 2022
@openjdk
Copy link

openjdk bot commented Oct 2, 2022

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

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label Oct 2, 2022
@mlbridge
Copy link

mlbridge bot commented Oct 2, 2022

Webrevs

@cl4es
Copy link
Member

cl4es commented Oct 2, 2022

Seems reasonable, although these generators should only rarely be used when doing reflection. I'm curious if you have some test or micro where the improvement shows?

@stsypanov
Copy link
Contributor Author

@cl4es this code is frequently invoked in Spring-based applications at start-up time. I couldn't design the benchmark for this particular case, assuming that in majority of cases Method.getExceptionTypes() returns a copy of an empty array I think we save about 12 ns and 16 bytes:

@State(Scope.Thread)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@BenchmarkMode(value = Mode.AverageTime)
public class ArrayCloneBenchmark {
  private final Object[] array = new Object[0];

  @Benchmark
  public Object[] cloneArray() {
    return array.clone();
  }
}
Benchmark                                                        Mode  Cnt    Score    Error   Units
ArrayCloneBenchmark.cloneArray                                   avgt   20   12,713 ±  0,484   ns/op
ArrayCloneBenchmark.cloneArray:·gc.alloc.rate                    avgt   20  800,247 ± 29,584  MB/sec
ArrayCloneBenchmark.cloneArray:·gc.alloc.rate.norm               avgt   20   16,004 ±  0,001    B/op
ArrayCloneBenchmark.cloneArray:·gc.churn.G1_Eden_Space           avgt   20  804,623 ± 38,424  MB/sec
ArrayCloneBenchmark.cloneArray:·gc.churn.G1_Eden_Space.norm      avgt   20   16,091 ±  0,455    B/op
ArrayCloneBenchmark.cloneArray:·gc.churn.G1_Survivor_Space       avgt   20    0,006 ±  0,002  MB/sec
ArrayCloneBenchmark.cloneArray:·gc.churn.G1_Survivor_Space.norm  avgt   20   ≈ 10⁻⁴             B/op
ArrayCloneBenchmark.cloneArray:·gc.count                         avgt   20  286,000           counts
ArrayCloneBenchmark.cloneArray:·gc.time                          avgt   20  171,000               ms

@cl4es
Copy link
Member

cl4es commented Oct 3, 2022

I agree that getting rid of the clone can help -- but since JEP 416 the generators modified here is mostly a fallback and the bulk of the use will use MethodHandles (unless you disable JEP 416 and fall back to the legacy impl). I was mostly curious if you had a startup or other benchmark running on mainline where the change you're doing here could be observed.

@cl4es
Copy link
Member

cl4es commented Oct 3, 2022

Approving since changes are trivial and having less code never hurts

@openjdk
Copy link

openjdk bot commented Oct 3, 2022

⚠️ @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 8294698
$ git commit --author='Preferred Full Name <you@example.com>' --allow-empty -m 'Update full name'
$ git push

@openjdk
Copy link

openjdk bot commented Oct 3, 2022

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

8294698: Remove unused 'checkedExceptions' param from MethodAccessorGenerator.generateMethod()

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

  • ccc1d31: 8294529: IGV: Highlight the current graphs in the Outline
  • 08a7ecf: 8294671: Remove unused CardValues::last_card
  • 5fe837a: 8294236: [IR Framework] CPU preconditions are overriden by regular preconditions
  • 8e9cfeb: 8294431: jshell reports error on initialisation of static final field of anonymous class
  • 6e8f038: 8294567: IGV: IllegalStateException in search
  • bc668b9: 8293099: JFR: Typo in TestRemoteDump.java
  • 03f25a9: 8293562: blocked threads with KeepAliveCache.get
  • a69ee85: 8292336: JFR: Warn users if -XX:StartFlightRecording:disk=false is specified with maxage or maxsize

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

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 3, 2022
@stsypanov
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 3, 2022
@openjdk
Copy link

openjdk bot commented Oct 3, 2022

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

@cl4es
Copy link
Member

cl4es commented Oct 3, 2022

/sponsor

@openjdk
Copy link

openjdk bot commented Oct 3, 2022

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

  • f2a32d9: 8293691: converting a defined BasicType value to a string should not crash the VM
  • ccc1d31: 8294529: IGV: Highlight the current graphs in the Outline
  • 08a7ecf: 8294671: Remove unused CardValues::last_card
  • 5fe837a: 8294236: [IR Framework] CPU preconditions are overriden by regular preconditions
  • 8e9cfeb: 8294431: jshell reports error on initialisation of static final field of anonymous class
  • 6e8f038: 8294567: IGV: IllegalStateException in search
  • bc668b9: 8293099: JFR: Typo in TestRemoteDump.java
  • 03f25a9: 8293562: blocked threads with KeepAliveCache.get
  • a69ee85: 8292336: JFR: Warn users if -XX:StartFlightRecording:disk=false is specified with maxage or maxsize

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Oct 3, 2022
@openjdk openjdk bot closed this Oct 3, 2022
@openjdk 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 Oct 3, 2022
@openjdk
Copy link

openjdk bot commented Oct 3, 2022

@cl4es @stsypanov Pushed as commit 46633e6.

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

@stsypanov stsypanov deleted the 8294698 branch October 3, 2022 13:11
@mlchung
Copy link
Member

mlchung commented Oct 3, 2022

I agree that getting rid of the clone can help -- but since JEP 416 the generators modified here is mostly a fallback and the bulk of the use will use MethodHandles (unless you disable JEP 416 and fall back to the legacy impl). I was mostly curious if you had a startup or other benchmark running on mainline where the change you're doing here could be observed.

@stsypanov like @cl4es said, I expect these dynamically generated methods should only be generated during very early VM startup. Once the module system is initialized, it will use method handles.

How do you run the spring-based applications when you observe these methods being called? While this change is trivial and no harm to include this change, this code is planned to be removed in a future release.

@stsypanov
Copy link
Contributor Author

@mlchung I can put here a stack trace of the application invoking the code if you are interested

@cl4es
Copy link
Member

cl4es commented Oct 4, 2022

@stsypanov would be helpful to understand when this code still gets executed. Also note that JEP 416 was integrated in JDK 18 so some details about the runtime would be necessary to get a complete picture.

@stsypanov
Copy link
Contributor Author

@cl4es @mlchung here's the stacktrace of my app running on Java 19:

generate:136, MethodAccessorGenerator (jdk.internal.reflect)
generateSerializationConstructor:107, MethodAccessorGenerator (jdk.internal.reflect)
generateConstructor:420, ReflectionFactory (jdk.internal.reflect)
newConstructorForSerialization:412, ReflectionFactory (jdk.internal.reflect)
getSerializableConstructor:1444, ObjectStreamClass (java.io)
run:412, ObjectStreamClass$2 (java.io)
run:384, ObjectStreamClass$2 (java.io)
executePrivileged:778, AccessController (java.security)
doPrivileged:319, AccessController (java.security)
<init>:384, ObjectStreamClass (java.io)
computeValue:110, ObjectStreamClass$Caches$1 (java.io)
computeValue:107, ObjectStreamClass$Caches$1 (java.io)
computeValue:73, ClassCache$1 (java.io)
computeValue:70, ClassCache$1 (java.io)
getFromHashMap:229, ClassValue (java.lang)
getFromBackup:211, ClassValue (java.lang)
get:117, ClassValue (java.lang)
get:84, ClassCache (java.io)
lookup:363, ObjectStreamClass (java.io)
lookup:246, ObjectStreamClass (java.io)
<clinit>:27, Result (org.junit.runner)
run:132, JUnitCore (org.junit.runner)
startRunnerWithArgs:69, JUnit4IdeaTestRunner (com.intellij.junit4)
execute:38, IdeaTestRunner$Repeater$1 (com.intellij.rt.junit)
repeat:11, TestsRepeater (com.intellij.rt.execution.junit)
startRunnerWithArgs:35, IdeaTestRunner$Repeater (com.intellij.rt.junit)
prepareStreamsAndStart:235, JUnitStarter (com.intellij.rt.junit)
main:54, JUnitStarter (com.intellij.rt.junit)

image

@mlchung
Copy link
Member

mlchung commented Oct 4, 2022

Thanks. This is for serialization and not the use of the core reflection which is what JEP 416 covered.

@stsypanov
Copy link
Contributor Author

stsypanov commented Oct 4, 2022

Is it going to be covered with new reflection sometimes? You mentioned above that "this code is planned to be removed in a future release" so some substitution must be provided, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
3 participants