Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v21]
On Tue, 18 Jun 2024 13:22:45 GMT, Adam Sotona wrote: >> java.base java.lang.invoke package heavily uses ASM to generate lambdas and >> method handles. >> >> This patch converts ASM calls to Classfile API. >> >> This PR is continuation of https://github.com/openjdk/jdk/pull/12945 >> >> Any comments and suggestions are welcome. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with four additional > commits since the last revision: > > - Merge pull request #8 from cl4es/serialization_hostile > >SerializationHostileMethod > - Reduce gratuitous code movement > - Inline Consumer into generateSer.. method, move seldom-used > serialization support constants to new holder > - SerializationHostileMethod src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java line 509: > 507: } > 508: > 509: Suggestion: - PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1644860876
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v21]
On Tue, 18 Jun 2024 13:30:39 GMT, Claes Redestad wrote: >> Adam Sotona has updated the pull request incrementally with four additional >> commits since the last revision: >> >> - Merge pull request #8 from cl4es/serialization_hostile >> >>SerializationHostileMethod >> - Reduce gratuitous code movement >> - Inline Consumer into generateSer.. method, move >> seldom-used serialization support constants to new holder >> - SerializationHostileMethod > > src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java > line 108: > >> 106: >> 107: // condy to load implMethod from class data >> 108: implMethodCondy = DynamicConstantDesc.ofNamed(BSM_CLASS_DATA, >> DEFAULT_NAME, CD_MethodHandle); > > Pre-existing tiny wart, but this one seem to be used only exceptionally (see > the comment/code around line 183) so it's probably better to inline the code > at the usage site rather than have a constant. If we aren't caching this descriptor, maybe it's even more simple for us to just construct the ConstantDynamicEntry from the pool builder at the use site; we can extract a method to keep this condy visible. - PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1644501219
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v21]
On Tue, 18 Jun 2024 13:22:45 GMT, Adam Sotona wrote: >> java.base java.lang.invoke package heavily uses ASM to generate lambdas and >> method handles. >> >> This patch converts ASM calls to Classfile API. >> >> This PR is continuation of https://github.com/openjdk/jdk/pull/12945 >> >> Any comments and suggestions are welcome. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with four additional > commits since the last revision: > > - Merge pull request #8 from cl4es/serialization_hostile > >SerializationHostileMethod > - Reduce gratuitous code movement > - Inline Consumer into generateSer.. method, move seldom-used > serialization support constants to new holder > - SerializationHostileMethod I've done a final pass over this. Code changes overall look good - great even - and the only caveat is that there's going to be some added startup overhead in some apps from integrating this. A minimal app with a lambda expression might see a 2-3ms hit, for example. While there are ideas on how to trim down such overheads further I think this is a good time to draw a line in the sand and get this change integrated and exposed to a larger set of testing. - Marked as reviewed by redestad (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17108#pullrequestreview-2125569570
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v21]
On Tue, 18 Jun 2024 13:22:45 GMT, Adam Sotona wrote: >> java.base java.lang.invoke package heavily uses ASM to generate lambdas and >> method handles. >> >> This patch converts ASM calls to Classfile API. >> >> This PR is continuation of https://github.com/openjdk/jdk/pull/12945 >> >> Any comments and suggestions are welcome. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with four additional > commits since the last revision: > > - Merge pull request #8 from cl4es/serialization_hostile > >SerializationHostileMethod > - Reduce gratuitous code movement > - Inline Consumer into generateSer.. method, move seldom-used > serialization support constants to new holder > - SerializationHostileMethod src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java line 108: > 106: > 107: // condy to load implMethod from class data > 108: implMethodCondy = DynamicConstantDesc.ofNamed(BSM_CLASS_DATA, > DEFAULT_NAME, CD_MethodHandle); Pre-existing tiny wart, but this one seem to be used only exceptionally (see the comment/code around line 183) so it's probably better to inline the code at the usage site rather than have a constant. - PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1644473614
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v21]
> java.base java.lang.invoke package heavily uses ASM to generate lambdas and > method handles. > > This patch converts ASM calls to Classfile API. > > This PR is continuation of https://github.com/openjdk/jdk/pull/12945 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with four additional commits since the last revision: - Merge pull request #8 from cl4es/serialization_hostile SerializationHostileMethod - Reduce gratuitous code movement - Inline Consumer into generateSer.. method, move seldom-used serialization support constants to new holder - SerializationHostileMethod - Changes: - all: https://git.openjdk.org/jdk/pull/17108/files - new: https://git.openjdk.org/jdk/pull/17108/files/3aaf246e..d3367487 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17108=20 - incr: https://webrevs.openjdk.org/?repo=jdk=17108=19-20 Stats: 86 lines in 1 file changed: 43 ins; 38 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/17108.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17108/head:pull/17108 PR: https://git.openjdk.org/jdk/pull/17108
