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

[Duncan Gittins]

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.

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