[foreign] RFR 8212560 : jextract should generate a static forwarder class

[Maurizio Cimadamore]

Hi Samuel,
I'm really confused here; if the various annotated interfaces/artifacts 
are generated in a jar bundle by jextract, what is the problem with 
layout annotations? It's not like the user will see them?

So, if you are concerned about usabilty (e.g. what the client see, as in 
your 'main' method) using a different kind of annotation won't change 
anything.

If you are concerned with the (one time) runtime cost to parse the 
layout annotations, we have already covered how the best way to address 
that concern is by running jextract as a jlink at module install time 
(as we do for other similar tasks in the JDK build itself). That will 
create binding implementations for all the required interfaces 
_statically_ which should help with startup.

Maurizio

On 17/10/2018 19:02, Samuel Audet wrote:
> And while we're at it, I think the runtime component could also take 
> care of abstracting away the details of the layouts, so gettimeofday() 
> could look something like this, again without having to parse anything 
> for simple cases:
>
>     @Struct
>     interface Timeval {
>         @SizeT long tv_sec();
>         Timeval tv_sec(long tv_sec);
>
>         long tv_usec();
>         Timeval tv_usec(long tv_usec);
>     }
>
>     interface Dummy {
>         int gettimeofday(Timeval tv, Pointer timezone);
>     }
>
>     import static Dummy.*;
>
>     public static void main(String[] args) {
>     InitMagicBindings(Dummy.class, Timeval.class);
>         Timeval t = Timeval.create()
>     gettimeofday(t, null);
>     }
> }
>
> Now that would be awesome :)
>
> Samuel
>
> On 10/18/2018 09:16 AM, Samuel Audet wrote:
>> That looks good, thanks Sundar! Though, eventually, it would be great 
>> to have those generated at runtime somehow, so that we don't need to 
>> start parsing stuff when we just need to call single functions, like 
>> we can do with JNA or JavaCPP, for example:
>> https://gist.github.com/saudet/1bf14a000e64c245675cf5d4e9ad6e69#file-nativebenchmark-java 
>>
>>
>> Samuel
>>
>> On 10/18/2018 02:35 AM, Sundararajan Athijegannathan wrote:
>>> Updated: 
>>> http://cr.openjdk.java.net/~sundar/8212560/webrev.02/index.html
>>>
>>> Added --static-forwarder boolean option (default true)
>>>
>>> -Sundar
>>>
>>> On 17/10/18, 9:55 PM, Sundararajan Athijegannathan wrote:
>>>>
>>>> comments below..
>>>>
>>>> On 17/10/18, 9:13 PM, Jorn Vernee wrote:
>>>>> Sundararajan Athijegannathan schreef op 2018-10-17 14:37:
>>>>>> On option: do we need an explicit option? given that one file extra
>>>>>> and that too only when -l is specified. (too many options 
>>>>>> already? ;)
>>>>>> )
>>>>>
>>>>> Maybe having an option is nice, but have it be turned on by 
>>>>> default? That way if someone really wanted to not generate the 
>>>>> forwarder they could, and for the general use case they don't have 
>>>>> to bother with setting an option every time.
>>>>>
>>>> okay.
>>>>
>>>>>> On naming: we've derived automatic names elsewhere. I think we could
>>>>>> revisit all naming options by another round - perhaps even java
>>>>>> convention for header interface name.
>>>>>>
>>>>>> On subclassing: Nice suggestion. I made it as AsmCodeFactoryExt 
>>>>>> class
>>>>>> and extended it from AsmCodeFactory. visit methods now return 
>>>>>> Boolean
>>>>>> to tell if the particular tree was handled or not. This is needed so
>>>>>> that subclass can avoid the same tree if super class avoided it (for
>>>>>> eg. function-like macros, repeated definitions..)
>>>>>
>>>>> I'm wondering if this runs into trouble later on when adding more 
>>>>> code generation extensions/options. Let's say you add `foo` and 
>>>>> `bar` options later on. You'd have to make an extension subclass 
>>>>> for every combination of options (StaticFooBar, StaticFoo, 
>>>>> StaticBar, FooBar, Static, Foo, Bar).
>>>>>
>>>>> Maybe you could have something like a `CodeFactory`, which 
>>>>> consists of an `AsmCodeFactory` and a list of `CodeFactoryExt`, of 
>>>>> which the static forwarder generator would be one. And have this 
>>>>> kind of pattern:
>>>>>
>>>>> ```
>>>>> @Override
>>>>> public Void visitVar(VarTree varTree, JType jt) {
>>>>>     if(asmCodeFactory.visitVar(varTree, jt)) {
>>>>>         for(CodeFactoryExt cfe : extensions) {
>>>>>             cfe.visitVar(varTree, jt);
>>>>>         }
>>>>>     }
>>>>>     return null;
>>>>> }
>>>>> ```
>>>>>
>>>>> I think that also allows you to get rid of the if/else statements 
>>>>> in the extension class.
>>>>>
>>>>> It's not really needed right now since there is only 1 extension, 
>>>>> but it might be nice idea for the future.
>>>>
>>>> Right. We don't want to go there yet - but yes, if we happened to 
>>>> add one more, we can revisit & refactor the code.
>>>>
>>>> -Sundar
>>>>>
>>>>> Jorn
>>>>>
>>>>>> Updated: 
>>>>>> http://cr.openjdk.java.net/~sundar/8212560/webrev.01/index.html
>>>>>>
>>>>>> Thanks,
>>>>>> -Sundar
>>>>>>
>>>>>> On 17/10/18, 3:25 PM, Maurizio Cimadamore wrote:
>>>>>>> I like it! Few questions/comments:
>>>>>>>
>>>>>>> * should it be enabled/disabled with explicit option
>>>>>>> * should the name of the statics class be customizable
>>>>>>> * I like the code organization - have you thought of pushing it 
>>>>>>> further and make StaticForwarderGenerator a _subclass_ of 
>>>>>>> AsmCodeFactory - each visitor could maybe delegate to 
>>>>>>> super.visitXyz() first and then do its own bit? Then when you 
>>>>>>> setup the pipeline, depending on what info is available on the 
>>>>>>> context (e.g. presence of -l) you can decide whether to use a 
>>>>>>> 'bare' ASMCodeFactory or the 'embellished' one. That should 
>>>>>>> remove all the 'if staticFwd != null ...' sections from 
>>>>>>> AsmCodeFactory.
>>>>>>>
>>>>>>> Maurizio
>>>>>>>
>>>>>>>
>>>>>>> On 17/10/2018 06:35, Sundararajan Athijegannathan wrote:
>>>>>>>> Please review.
>>>>>>>>
>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8212560
>>>>>>>> Webrev: 
>>>>>>>> http://cr.openjdk.java.net/~sundar/8212560/webrev.00/index.html
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> -Sundar
>>
>