Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v18]
On Mon, 17 Jun 2024 11:21:37 GMT, Adam Sotona wrote: >> LMF will BMH and `ClassSpecializer`, but does not call into this code >> normally as the simple bound method handle needed by LMF is statically >> defined (`BoundMethodHandle.Species_L`). To be perfectly safe - and to keep >> lambda/indy/condy bootstrap overheads to a minumum - I'll continue >> recommending the desugaring of lambdas in java.lang.invoke > > It results in not very nice code, but makes sense. I'll note that replacing a lambda with an anonymous class has a very marginal effect on startup when amortizing the cost of setting up the infrastructure for spinning classes. What can have a decent effect is if we can replace a few lambdas/anon classes with more imperative code that spins up/loads fewer classes, e.g., desugaring streams to for loops. - PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1644202229
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v18]
On Wed, 12 Jun 2024 20:27:05 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - reverted static initialization of ConstantPoolBuilder and CP entries
>> - fixed naming conventions
>
> src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
> line 1148:
>
>> 1146: }
>> 1147:
>> 1148: private void emitPopInsn(CodeBuilder cob, BasicType type) {
>
> We need a TypeKind-aware pop in CodeBuilder too :(
Yes, it would be helpful.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1642870775
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v18]
On Thu, 6 Jun 2024 13:17:20 GMT, Chen Liang wrote: >> Adam Sotona has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - reverted static initialization of ConstantPoolBuilder and CP entries >> - fixed naming conventions > > src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java > line 470: > >> 468: MethodVisitor mv = cw.visitMethod(ACC_PRIVATE + ACC_FINAL, >> 469: NAME_METHOD_WRITE_OBJECT, >> DESCR_METHOD_WRITE_OBJECT, >> 470: null, SER_HOSTILE_EXCEPTIONS); > > The exceptions attribute is now lost on the migrated methods. Is that ok? > It's private and only accessed by reflection so I think there's no real > impact. Good catch, they were lost in translation. I think we should keep them. - PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1642800017
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v18]
On Thu, 6 Jun 2024 12:22:41 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - reverted static initialization of ConstantPoolBuilder and CP entries
>> - fixed naming conventions
>
> src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
> line 96:
>
>> 94: }
>> 95: };
>> 96: record MethodBody(Consumer code) implements
>> Consumer {
>
> Why do we have these 2 instead of a noop record field builder consumer (flags
> is already set in withField, and MethodBody should just be direct usage of
> withMethodBody)
>
> Seems the problem is in CF implemetnation side. Then these should be part of
> CF implementation details.
There is no problem if you can rely on lambdas ;)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1642793485
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v18]
On Thu, 6 Jun 2024 12:17:14 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - reverted static initialization of ConstantPoolBuilder and CP entries
>> - fixed naming conventions
>
> src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java
> line 564:
>
>> 562: clb.withFlags(ACC_PRIVATE | ACC_FINAL | ACC_SUPER)
>> 563:
>> .withSuperclass(InvokerBytecodeGenerator.INVOKER_SUPER_DESC)
>> 564:
>> .with(SourceFileAttribute.of(className.substring(className.lastIndexOf('/')
>> + 1)));
>
> Maybe we can keep the classDesc from ofInternalName and use its displayName.
I don't see any benefits, both ways are sub-stringing.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1642789436
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v18]
On Thu, 6 Jun 2024 12:13:44 GMT, Chen Liang wrote: >> Adam Sotona has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - reverted static initialization of ConstantPoolBuilder and CP entries >> - fixed naming conventions > > src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java line 784: > >> 782: // mix them up and load them for the >> transform helper: >> 783: List helperArgs = >> speciesData.deriveTransformHelperArguments(transformMethods.get(whichtm), >> whichtm, targs, tfields); >> 784: List> helperTypes = new >> ArrayList<>(helperArgs.size()); > > Can we convert helperTypes here to List so we construct > invokeBasicType as MethodTypeDesc below? This part can be simplified to a directly used validated array of ClassDesc and many conversions can be skipped. - PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1642756354
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v18]
On Thu, 13 Jun 2024 09:27:44 GMT, Claes Redestad wrote:
>> src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java line 616:
>>
>>> 614: final ClassDesc classDesc = ClassDesc.of(className0);
>>> 615: final ClassDesc superClassDesc =
>>> classDesc(speciesData.deriveSuperClass());
>>> 616: return ClassFile.of().build(classDesc, clb -> {
>>
>> We need a confirmation here that these BMH species are only initialized
>> after LMF is ready. @cl4es Is a dependency on LMF from BMH safe?
>
> LMF will BMH and `ClassSpecializer`, but does not call into this code
> normally as the simple bound method handle needed by LMF is statically
> defined (`BoundMethodHandle.Species_L`). To be perfectly safe - and to keep
> lambda/indy/condy bootstrap overheads to a minumum - I'll continue
> recommending the desugaring of lambdas in java.lang.invoke
It results in not very nice code, but makes sense.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1642653369
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v18]
On Thu, 6 Jun 2024 11:41:05 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - reverted static initialization of ConstantPoolBuilder and CP entries
>> - fixed naming conventions
>
> src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java line 616:
>
>> 614: final ClassDesc classDesc = ClassDesc.of(className0);
>> 615: final ClassDesc superClassDesc =
>> classDesc(speciesData.deriveSuperClass());
>> 616: return ClassFile.of().build(classDesc, clb -> {
>
> We need a confirmation here that these BMH species are only initialized after
> LMF is ready. @cl4es Is a dependency on LMF from BMH safe?
LMF will BMH and `ClassSpecializer`, but does not call into this code normally
as the simple bound method handle needed by LMF is statically defined
(`BoundMethodHandle.Species_L`). To be perfectly safe - and to keep
lambda/indy/condy bootstrap overheads to a minumum - I'll continue recommending
the desugaring of lambdas in java.lang.invoke
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1637893254
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v18]
On Thu, 6 Jun 2024 13:24:06 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - reverted static initialization of ConstantPoolBuilder and CP entries
>> - fixed naming conventions
>
> src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
> line 566:
>
>> 564:
>> 565: static ClassDesc implClassDesc(Class cls) {
>> 566: return cls.isHidden() ?
>> ReferenceClassDescImpl.ofValidatedBinaryName(cls.getName())
>
> I recommend just returning null or a dummy value if the cls is hidden; in
> this case, implMethodClassDesc can safely be null, as implementation must go
> through condy.
Relevant issue:
- https://github.com/openjdk/jdk/pull/18810
[JDK‑8330467]: https://bugs.openjdk.org/browse/JDK-8330467
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1637204601
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v18]
On Thu, 6 Jun 2024 10:16:14 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 two additional
> commits since the last revision:
>
> - reverted static initialization of ConstantPoolBuilder and CP entries
> - fixed naming conventions
src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java line 616:
> 614: final ClassDesc classDesc = ClassDesc.of(className0);
> 615: final ClassDesc superClassDesc =
> classDesc(speciesData.deriveSuperClass());
> 616: return ClassFile.of().build(classDesc, clb -> {
We need a confirmation here that these BMH species are only initialized after
LMF is ready. @cl4es Is a dependency on LMF from BMH safe?
src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java line 771:
> 769: final intTMODS = TRANSFORM_MODS.get(whichtm);
> 770: clb.withMethod(TNAME, methodDesc(TTYPE), (TMODS &
> ACC_PPP) | ACC_FINAL, mb -> {
> 771: mb.withFlags((TMODS & ACC_PPP) | ACC_FINAL)
Isn't this redundant as we've specified it in initial withMethod call already?
src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java line 784:
> 782: // mix them up and load them for the
> transform helper:
> 783: List helperArgs =
> speciesData.deriveTransformHelperArguments(transformMethods.get(whichtm),
> whichtm, targs, tfields);
> 784: List> helperTypes = new
> ArrayList<>(helperArgs.size());
Can we convert helperTypes here to List so we construct
invokeBasicType as MethodTypeDesc below?
src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java line
564:
> 562: clb.withFlags(ACC_PRIVATE | ACC_FINAL | ACC_SUPER)
> 563:
> .withSuperclass(InvokerBytecodeGenerator.INVOKER_SUPER_DESC)
> 564:
> .with(SourceFileAttribute.of(className.substring(className.lastIndexOf('/') +
> 1)));
Maybe we can keep the classDesc from ofInternalName and use its displayName.
src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
line 87:
> 85:
> 86: private static final String[] EMPTY_STRING_ARRAY = new String[0];
> 87: private static final ClassDesc[] EMPTY_CLASSDESC_ARRAY = new
> ClassDesc[0];
Suggestion:
private static final ClassDesc[] EMPTY_CLASSDESC_ARRAY =
ConstantUtils.EMPTY_CLASSDESC;
Need an import.
src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
line 96:
> 94: }
> 95: };
> 96: record MethodBody(Consumer code) implements
> Consumer {
Why do we have these 2 instead of a noop record field builder consumer (flags
is already set in withField, and MethodBody should just be direct usage of
withMethodBody)
Seems the problem is in CF implemetnation side. Then these should be part of CF
implementation details.
src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
line 214:
> 212: for (int i = 0; i < parameterCount; i++) {
> 213: argNames[i] = "arg$" + (i + 1);
> 214: argDescs[i] = classDesc(factoryType.parameterType(i));
We should declare global ad-hoc classDesc and methodDesc methods in
ConstantUtils, that way if we speed up our conversions all users benefit and
the generators don't need to define these conversions everywhere.
src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
line 470:
> 468: MethodVisitor mv = cw.visitMethod(ACC_PRIVATE + ACC_FINAL,
> 469: NAME_METHOD_WRITE_OBJECT,
> DESCR_METHOD_WRITE_OBJECT,
> 470: null, SER_HOSTILE_EXCEPTIONS);
The exceptions attribute is now lost on the migrated methods. Is that ok? It's
private and only accessed by reflection so I think there's no real impact.
src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
line 566:
> 564:
> 565: static ClassDesc implClassDesc(Class cls) {
> 566: return cls.isHidden() ?
> ReferenceClassDescImpl.ofValidatedBinaryName(cls.getName())
I recommend just returning null or a dummy value if the cls is hidden; in this
case, implMethodClassDesc can safely be null, as implementation must go through
condy.
src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line
74:
> 72: private static final ClassDesc CD_LambdaForm_Name =
> ReferenceClassDescImpl.ofValidated("Ljava/lang/invoke/LambdaForm$Name;");
> 73: private static final ClassDesc
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v18]
> 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 two additional commits since the last revision: - reverted static initialization of ConstantPoolBuilder and CP entries - fixed naming conventions - Changes: - all: https://git.openjdk.org/jdk/pull/17108/files - new: https://git.openjdk.org/jdk/pull/17108/files/e814749a..f870a8df Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17108=17 - incr: https://webrevs.openjdk.org/?repo=jdk=17108=16-17 Stats: 43 lines in 2 files changed: 7 ins; 13 del; 23 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
