[foreign-jextract] RFR: 8253725: Jextract fails to extract big monolithic headers

[Maurizio Cimadamore]

On Mon, 28 Sep 2020 15:20:27 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> This patch adds support for splitting the symbols in the main header class into multiple classes; the header class is
>> then rewrited to extend all these additional classes, so that all symbols would be exposed to users in the classic way.
>> I've toyed with some ways to split on a by-header basis, but found that in certain cases we would generate near-empty
>> classes - and that would still not prevent issues from occurring in the corner case a single very big header file
>> contained so many symbols.  I've reorganized the code a bit - I noted that we were generating several nested classes,
>> but in some cases (structs) we were using a builder, in other cases (functional interfaces, typedef) we weren't. Now we
>> use builders for classes, and I've added an intermediate nested builder abstraction which does the right thing for all
>> nested cases.  I've also converted the source constant generator to be a JavaSourceBuilder too - since it too is a
>> class, after all, with a classBegin/End and build methods.  Finally, I moved the string builder/indentation logic out
>> to a different class (StringSourceBuilder); since some builders want to have their own string buffer, while others (the
>> nested ones) don't.  The organization of the builder code is still far from optimal, but I think that, moving forward,
>> this consolidation should make it easier to make changes.
>
> src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/OutputFactory.java line 308:
> 
>> 306:                 }
>> 307:                 MethodType fitype = typeTranslator.getMethodType(f, false);
>> 308:                 toplevelBuilder.addFunctionalInterface(name, fitype,
> 
> Should `header()` be used here instead of toplevelBuilder as well?

No - this one of the (many) tricky things. Struct/Functional interface/Typedef nested classes always end up in the
toplevel (the API the user can see). They don't end up increasing the size of the toplevel classfile, as they are
separate classes.

> src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/OutputFactory.java line 356:
> 
>> 354:                              */
>> 355:                             if (structDefinitionSeen(s)) {
>> 356:                                 toplevelBuilder.addTypeDef(tree.name(), structDefinitionName(s), tree.type());
> 
> Same here; toplevelBuilder -> header() ?

Same as above

> src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/SourceConstantHelper.java line 234:
> 
>> 232:         builder.append(OutputFactory.C_LANG_CONSTANTS_HOLDER);
>> 233:         builder.append(".*;\n\n");
>> 234:     }
> 
> Isn't this the same as in JavaSourceBuilder? Is the override needed?

Likely forgot to remove, I'll try and see.

> src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/ToplevelBuilder.java line 75:
> 
>> 73:         FunctionalInterfaceBuilder builder = new FunctionalInterfaceBuilder(this, name, mtype, desc, type);
>> 74:         builder.classBegin();
>> 75:         builder.classEnd();
> 
> This looks a little suspicious at first glance. It seems like it would just be a class open and close like:
> class MyClass {
> }
> But actually the classEnd() method is adding some other things as a side effect. In other words, it looks like this
> code _should_ tell me what is being added by the builder, but actually part of it is hidden.
> I think this should either be made explicit here, e.g. having
> builder.classBegin();
> builder.addStuff();
> builder.classEnd();
> Or, we say that the entire building process is the responsibility of the FunctionalInterfaceBuilder, and here we just
> have:
> builder.build(); // does all 3
> WDYT?

I see what you mean. Right now, one piece of messiness is that the class contents are attached to either class
begin/end, w/o much discipline. I'll see what I can do.

> src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/ToplevelBuilder.java line 103:
> 
>> 101:         } else {
>> 102:             declCount++;
>> 103:             return lastHeader().orElse(this);
> 
> Does this mean the first 1000 decls always go in the top level interface? That makes sense, since for small headers
> this still means you only get the top-level interface.

yes - that leads to a CP fill of about half - we could probably increase it further, but seems a slippery slope

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

PR: https://git.openjdk.java.net/panama-foreign/pull/357