[foreign-jextract] RFR: 8263515: Generate flatter bindings

Mon Mar 15 14:14:48 UTC 2021

Yes, its much better if RuntimeHelper was not needed at all

Duncan

On 15/03/2021 13:59, Maurizio Cimadamore wrote:
>
> On 15/03/2021 13:29, Duncan Gittins wrote:
>> Maurizio
>>
>> I tried this on my sample application which uses jextract on a group 
>> of Windows headers. Previously the generated jar made Eclipse freeze 
>> completely for long periods. Now Eclipse isn't freezing - a huge 
>> improvement. It is slow for first code completion lookup (- not 
>> unreasonable for 8MB jar), and code completions are at normal speed 
>> thereafter.
>>
>> Some observations:
>> 1) Does anyone out there have a struct/typedef called RuntimeHelper? 
>> Jextract handles by renaming the type, but perhaps should also give a 
>> warning message?
>>
>> 2) It's no longer possible to do 2 jextract runs with same -t package 
>> as the different constant$N classes overwrite each other, and maybe 
>> also typedef/struct names. This isn't a problem - it just means we 
>> must remember to use unique package names for each jextract. However 
>> that then means that RuntimeHelper+VarargsInvoker is duplicated per 
>> package whereas previously the same RH+VI would be shared for all the 
>> XYZ_h classes.
>>
>> Is it is worth adding "-rt packagename" parameter to indicate the 
>> package name of the jextract internal RuntimeHelper - defaults to 
>> same package as "-t" ? That helps both points.
>
> I think everything surrounding RuntimeHelper is a bit temporary. The 
> way I see this working is that, eventually, RuntimeHelper (or the most 
> important parts of it) will be moved into some kind of runtime support 
> for jextracted stuff.
>
> I believe there are two things that RuntimeHelper does which are kind 
> of central to the code generation:
>
> 1) Add support for varargs-like invocation when it comes to variadic 
> functions (assuming this is relevant - note that invoking varargs this 
> way is not efficient at all, and only works for downcalls, which is 
> why we have the VaList thingie).
> 2) Add multi-library lookups - e.g.setup a bunch of library lookups 
> given library paths and/or given a bunch of library lookups, lookup a 
> symbol in any lookup object in this bunch
>
> For both I think there is a discussion to be had as to whether they 
> are still relevant enough - for (2) I can definitively seem jextract 
> to just do what it does today w/o relying on RuntimeHelper; as for (1) 
> we need to decide if that kind of variadic call support is what we 
> want to provide, given how inefficient it is (Swift has also gone down 
> the path of only enabling variadic call via VaList).
>
> In other words, as the extracted will stabilize, RuntimeHelper 
> features will be either moved in the API they belong to, or removed 
> (if not deemed worthy enough). And we'd be in a place where there's no 
> RuntimeHelper being generated by jextract.
>
> For that reason, I'd rather not add options which might make 
> RuntimeHelper look more important than it actually is.
>
> Maurizio
>
>>
>> Kind regards
>>
>> Duncan
>>
>> On 12/03/2021 18:54, Maurizio Cimadamore wrote:
>>> As mentioned in [1], while the current generation strategy is handy 
>>> (it only requires a single static import from the user to get 
>>> started), it also generates huge, nested source files. This has a 
>>> cost on tooling (as IDE can sometimes have issues with hugh source 
>>> files) and on class loading, as the generated classfiles are full of 
>>> innerclass/nested attributes. It is also not clear how much the 
>>> current generation strategy will scale once javadoc support will be 
>>> added to generated bindings.
>>>
>>> This patch tweaks jextract so that flatter bindings are generated. 
>>> In other words, all toplevel structs/union/typedef will now be 
>>> emitted in a separate toplevel class. Structs that appear nested in 
>>> the C source code will still be nested in the resulted bindings 
>>> (although that might be revisited - after all in C there's only one 
>>> namespace).
>>>
>>> The implementation had to change in two, main ways; first, the split 
>>> between JavaSourceBuilder and NestedSourceBuilder was problematic, 
>>> since with the flatter scheme a struct can appear both as a nested 
>>> entity, or as a toplevel one.
>>>
>>> The second change is that JavaSourceBuilder has been split into two: 
>>> a general, abstract class (JavaSourceBuilder) and a more concrete 
>>> one (ClassSourceBuilder). The latter is a builder for Java 
>>> classes/interfaces, while the former is the root of all builders - 
>>> and can also be used by builders that do not really map to any 
>>> specific class (e.g. TopLevelBuilder).
>>>
>>> Now, all concrete builders are linked (via the enclosing field) to 
>>> the TopLevelBuilder. This chain is useful, as it allows up to keep 
>>> "nesting" (this is visible in how the code in 
>>> OutputFactory::visitScoped is shaped).
>>>
>>> I've tested this patch on a bunch of libraries on Linux, and also on 
>>> Windows.h and I did not notice any footprint change - in both system 
>>> jextract feels faster and seems to use less memory (likley due to 
>>> the fact that sources generated - stored in a string builder - are 
>>> smaller). Startup seems to have improved too, at least in my OpenGL 
>>> test, where the time to complete the first native call went from 
>>> 80ms to 60ms. The only explanation I have for this fact is that the 
>>> generated classes have much much less inner class/nestmates 
>>> attributes - but could also be a coincidence (e.g. maybe the patch 
>>> accidentally rearranges constants in a "better" way, for this 
>>> particular test).
>>>
>>> [1] - https://bugs.openjdk.java.net/browse/JDK-8263515
>>>
>>> -------------
>>>
>>> Commit messages:
>>>   - Make constant classes package private
>>>   - Move constants to toplevel
>>>   - Further consolidate code
>>>   - Cleanup code
>>>   - Merge branch 'foreign-jextract' into non-nested
>>>   - Merge branch 'foreign-jextract' into non-nested
>>>   - Tweak ToplevelBuilder
>>>   - Clean up nesting logic
>>>   - Fix minor issues
>>>   - All tests pass
>>>   - ... and 3 more: 
>>> https://git.openjdk.java.net/panama-foreign/compare/a11bb7d2...87573577
>>>
>>> Changes: https://git.openjdk.java.net/panama-foreign/pull/470/files
>>>   Webrev: 
>>> https://webrevs.openjdk.java.net/?repo=panama-foreign&pr=470&range=00
>>>    Issue: https://bugs.openjdk.java.net/browse/JDK-8263515
>>>    Stats: 822 lines in 38 files changed: 351 ins; 341 del; 130 mod
>>>    Patch: https://git.openjdk.java.net/panama-foreign/pull/470.diff
>>>    Fetch: git fetch https://git.openjdk.java.net/panama-foreign 
>>> pull/470/head:pull/470
>>>
>>> PR: https://git.openjdk.java.net/panama-foreign/pull/470
>>
>>