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

Mon Mar 15 13:59:03 UTC 2021

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
>
>