RFR: 7903597: Slim down RuntimeHelper

[Maurizio Cimadamore]

On Fri, 1 Dec 2023 16:58:12 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

> Remove some of the helper methods in RuntimeHelper. They can be moved inline in the generated code.
> 
> Also: I've elected to move the `System.load(Library)` directives we generated inside RuntimeHelper into the first header class, in order to have it more localized to the actual code that a user sees. That also makes RuntimeHelper more 'dumb'. Only the package declaration is now injected into the template, which makes it easier for users to combined multiple extraction runs (they can just discard one of the generated RuntimeHelpers).

Looks very good. I think I'm mildly leaning for moving the common code in the header class (which is out true main point), and perhaps make some of the code generation conditional. Note also that, now that we only have few methods to add, we could use string templates/text blocks rather than a template file, which will simplify things a bit (no more reading files etc.)

src/main/java/org/openjdk/jextract/impl/HeaderFileBuilder.java line 192:

> 190:                 public static \{invokerName} \{invokerFactoryName}(MemoryLayout... layouts) {
> 191:                     FunctionDescriptor baseDesc$ = \{descriptorString(2, descriptor)};
> 192:                     var mh$ = Linker.nativeLinker().downcallHandle(

If we keep other handy generation wrappers, we might keep this as well. It's not super trivial after all.

src/main/java/org/openjdk/jextract/impl/HeaderFileBuilder.java line 248:

> 246:     }
> 247: 
> 248:     void emitLoadLibraries(List<String> libraries) {

To be fair, moving forward I'm not too convinced about  keeping this section. That is, perhaps it would be better to give up trying to use static jextract options to configure which library is loaded at runtime, and use a loader lookup. The client code can then side-effect the classloader by making sure the correct library is loaded.

src/main/java/org/openjdk/jextract/impl/HeaderFileBuilder.java line 299:

> 297:         appendLines(STR."""
> 298:             \{MEMBER_MODS} MemorySegment \{mangledName}() {
> 299:                 class Holder {

I was ambivalent on this - but it's probably better to make it consistent with the strategy we use for functions, as you have done.

src/main/resources/org/openjdk/jextract/impl/resources/RuntimeHelper.java.template line 27:

> 25:     static {
> 26:         SymbolLookup loaderLookup = SymbolLookup.loaderLookup();
> 27:         Linker linker = Linker.nativeLinker();

Question: with RuntimeHelper being so small now, and given one of the stated goal is to allow for better composition of different extractions, would it not be convenient to move all these definitions in the header class and ditch `RuntimeHelper` completely? If we do that, we might also avoid generating the varargs definitions unless we need them, so that the header class would only include minimal additional code.

-------------

Marked as reviewed by mcimadamore (Committer).

PR Review: https://git.openjdk.org/jextract/pull/150#pullrequestreview-1760642302
PR Review Comment: https://git.openjdk.org/jextract/pull/150#discussion_r1412618023
PR Review Comment: https://git.openjdk.org/jextract/pull/150#discussion_r1412617275
PR Review Comment: https://git.openjdk.org/jextract/pull/150#discussion_r1412616149
PR Review Comment: https://git.openjdk.org/jextract/pull/150#discussion_r1412614694