Usage feedback rewriting jdk.internal.foreign.abi.BindingSpecializer

[Maurizio Cimadamore]

Great work.

IMHO, the nicest bits of the patch are at the very beginning and at the 
very end.

At the very beginning, because, instead of using loose strings for a mix 
of method names, descriptors etc, we can now use appropriate 
abstractions (ClassDesc, MethodTypeDesc).

At the very end because instead of rolling in our own "high-level" 
methods to generate the bytecode to emit e.g. a constant, we can just 
rely on the API to do that for us.

Maurizio

On 14/03/2023 15:31, Jorn Vernee wrote:
> Hello,
>
> I've re-written jdk.internal.foreign.abi.BindingSpecializer to use the 
> new class file API. The patch can be found here: 
> https://github.com/openjdk/panama-foreign/compare/foreign-memaccess+abi...JornVernee:panama-foreign:CFA
>
> I did this mostly based on just the javadoc.
>
> Some things I noted while doing this:
>
> 1. The translation is really straightforward, and there were a lot of 
> places where simplification was possible. Such as, using 
> CodeBuilder::constantInstruction is much nicer than manually having to 
> find the right bytecode for the constant you have. Another nice thing 
> was the CodeBuilder::parameterSlot method. I previously had to 
> translate parameter indices to slots manually. Great work!
>
> 2. I'm not sure what the expected way of mapping a Class<?> (or 
> ClassDesc) into a TypeKind is. I couldn't find a factory for that in 
> TypeKind. The code I have often needs to map Class<?> objects into 
> TypeKinds. I ended up writing a manual 'mapper' method instead [1].
>
> 3. Sometimes I need to emit a constant zero for a particular type. If 
> I know the type is e.g. long, I can use `cb.constantInstruction(0L)`, 
> but sometimes the type is not statically known, and it would be nice 
> if there was a CodeBuilder::constantZero(TypeKind) for that. I've 
> implemented this manually as well [2]
>
> 4. The same goes for `dup`. It would be nice if there was a 
> CodeBuilder::dup(TypeKind) that automatically selects either dup or 
> dup2. See [3]
>
> 5. I'll throw in a bike shed comment as well :) 
> CodeBuilder::labelBinding sounds a bit strange. From the name, I 
> initially wasn't sure whether this was returning an existing binding 
> for a previously bound label, or binding the label I passed it. (it 
> also doesn't help that the method doesn't have any javadoc at the 
> moment). I think the name "bindLabel" (or just "bind", as we have in 
> HotSpot) would be more intuitive.
>
> Cheers,
> Jorn
>
> [1]: 
> https://github.com/openjdk/panama-foreign/compare/foreign-memaccess+abi...JornVernee:panama-foreign:CFA#diff-2a53a53e1a977d7dbd5591e63c3b735d4db31744294625ae0d856d2a902d996eR414
> [2]: 
> https://github.com/openjdk/panama-foreign/compare/foreign-memaccess+abi...JornVernee:panama-foreign:CFA#diff-2a53a53e1a977d7dbd5591e63c3b735d4db31744294625ae0d856d2a902d996eR959
> [3]: 
> https://github.com/openjdk/panama-foreign/compare/foreign-memaccess+abi...JornVernee:panama-foreign:CFA#diff-2a53a53e1a977d7dbd5591e63c3b735d4db31744294625ae0d856d2a902d996eR947
>