FlowingCode / FontAwesomeIronIconset

Iron iconset based on FontAwesome
https://www.flowingcode.com/en/open-source/
Apache License 2.0
15 stars 6 forks source link

FontAwesome Pro: 'code too long' despite being on jdk 15+ #65

Closed GrandyB closed 1 year ago

GrandyB commented 1 year ago

Been attempting to compile against FA 6.1.1 Pro.

Their sprite files look a little like this:

<?xml version="1.0" encoding="UTF-8"?>
<!--!
Font Awesome Pro 6.1.1 by @fontawesome - https://fontawesome.com
License - https://fontawesome.com/license (Commercial License)
Copyright 2022 Fonticons, Inc.
-->
<svg xmlns="http://www.w3.org/2000/svg" style="display: none;">
  ...
  <symbol id="hourglass" viewBox="0 0 384 512">
    <path d="..."></path>
  </symbol>
  ...
  <symbol id="notebook" viewBox="0 0 512 512">
    <path d="..."></path>
etc

...whereas the icons.xslt that looks for the icon names during IconsetEnumGenerator#readIconNames seems to be trying to read the names using g tags, which obviously finds none. This has meant that while icons.json is loaded and the aliases are loaded, it fails to include the original icon names in the output.

GrandyB commented 1 year ago

I was also finding that the 3124 icons when generating along with their aliases was resulting in the 'code too long' despite being on jdk 15+... around 8000 enum entries. Removing aliases seemed to keep it a bit lower and compiled through.

javier-godoy commented 1 year ago

Hello. Which branch are you compiling? (versions 3, 4 and 5 of the add-on use FontAwesome 6).

How did you configure the properties in the POM? "codegen.icons"(which is commented out in our POM) is optional: you can either feed the original FA sprites, or individual SVG icons in folders such as icons/solid, icons/regular, etc. If codegen.icons is provided, BuildSprites will generate the sprite using the provided icons (this was needed in some FA Pro 6.0.0 beta versions, which did not include sprites; IIRC later releases include sprites)

Please note that the sprites are first processed by convertIconSet, which adds the g tags to the iconset, among other changes. I don't have access to FontAwesome Pro (thus I haven't generated Pro icons in a while), but the snippet that you copied above looks similar to the free FontAwesome sprites, so it should work in the same way.

Regarding the 'code too long' issue, do you execute maven from console? (if so, double check that JAVA_HOME is set to JDK 15+ version).

GrandyB commented 1 year ago

Just grabbed latest master and attempted again from sprite set.

        <fontawesome.version>6.1.2</fontawesome.version>
        <project.build.generatedResources>${project.basedir}/src/main/javascript</project.build.generatedResources>
        <codegen.sprites>${project.basedir}/src/main/sprites</codegen.sprites>
        <codegen.skipDownload>true</codegen.skipDownload>
        <codegen.embedded>true</codegen.embedded>

src/main/sprites has FA's regular.svg and solid.svg and icons.json (from the original metadata folder)

Yes I am running through cmd but have attempted through eclipse also. mvn -version:

C:\Users\------\Documents\FontAwesomeIronIconset-master>mvn -version
Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
Maven home: C:\-------\maven\apache-maven-3.6.3\bin\..
Java version: 17.0.1, vendor: Oracle Corporation, runtime: C:\---------\java17

java -version

openjdk version "17.0.1" 2021-10-19
OpenJDK Runtime Environment (build 17.0.1+12-39)
OpenJDK 64-Bit Server VM (build 17.0.1+12-39, mixed mode, sharing)

If I look at the generated C:\Users\mbishop\Documents\FontAwesomeIronIconset-master\src\main\generated\com\flowingcode\vaadin\addons\fontawesome\FontAwesome.java it has like 33k lines, which with 4 lines per enum entry... can reasonably estimate.

Looks like the changes since 4.2.0 have fixed parts of the problems I was seeing however. The 'g' tag part at least.

It might be that we switch over to using a tiny subset of icons so I might not be attempting full FA files for much longer anyway.

javier-godoy commented 1 year ago

The requirement of JDK 15+ is because of https://bugs.openjdk.java.net/browse/JDK-8241798, which moves the array initialization into a synthetic method $values() (https://stackoverflow.com/a/66352975/1297272) so that it doesn't count against the 64K bytecode-size limit for the static initializer. That fix gave us enough room for compiling icons for FontAwesome Pro before the introduction of aliases (with JDK 14 and earlier, we were able to compile about 2370 icons. All these limits are per-enum, the fact that all the enums are nested inside of FontAwesome does not matter)

This other comment mentions that "newer compilers split large static initializers", so the static initializer should not be a problem anymore (each constant takes about 15 bytes in the static initializer bytecode, then anything above 4370 icons would require the compiler to split the static initializer).

Barring that, the next limit is in the $values() method, which requires 8 bytes per constant. That imposes a theoretical limit of 8192 icons. I wonder if this issue has been fixed in newer compilers, or if it would be possible to hand-compile a class with ASM, providing a better implementation of $values().

With a sensible implementation of $values() it should support up to 32K icons (which is a hard limit since the constant pool has a limit of 64K entries and each icon requires two entries)

It might be that we switch over to using a tiny subset of icons so I might not be attempting full FA files for much longer anyway.

I have had that in my backlog for a while. (It would be just some intermediate step that removes some icons from the iconset, before feeding it to the later steps of the generator). If you already have such iconset, you can try using https://vaadin.com/docs/latest/components/icons/#icon-set-generator

javier-godoy commented 1 year ago

(note that master currently targets Vaadin 24, because the initialization of iconsets had a breaking change #64)

GrandyB commented 1 year ago

Aye, I had seen some chatter about those limits; would you suggest people try the most recently available jdk in that case?

...and I don't suppose such an easy generator exists for v14? heh

We're on mpr with a v14/7 hybrid currently. The reason for looking into this to begin with is because the 6 bundled FA iconsets are adding a bunch to page load times; it's like 18mb in full. We only use a couple of the sets, so that's why we initially looked at just taking regular and solid, reducing it by two thirds. Though as mentioned, given we can generate from SVGs using this tool, we might subset further to the exact icons we use.

Which version of the codebase should I be using for v14? Edit: We're on Java8 so I've taken 3.0.0 and used that. Seems to be generating, just figuring out why there's still all 6 sets sitting in the DOM.

javier-godoy commented 1 year ago

Aye, I had seen some chatter about those limits; would you suggest people try the most recently available jdk in that case?

I tested with Correto 19 and it compiles the same bytecode as Correto 17.

BTW, the issue is in the static initializer (and not in the synthetic method) because enum fields must be assigned in the static initializer, per putstatic specs: "if the field is final, it must be declared in the current class, and the instruction must occur in the method of the current class.", thus the compiler cannot split that part. (I guess that the comments about splitting the initializer apply to older compilers up to JDK 14)

The static initializer for enum classes, as created by the compiler, has CONSTANT = new Enum("name",ordinal) which takes 15 bytes:

0: new           #1                  // class com/example/application/MyEnum
3: dup
4: ldc           #23                 // String name
6: sipush        12345
9: invokespecial #24                 // Method "<init>":(Ljava/lang/String;I)V
12: putstatic     #26                 // Field CONSTANT:Lcom/example/application/MyEnum;
15: ...

That can be reduced to 11 bytes by avoiding new+dup and replacing invokespecial with invokestatic that leaves the value in the stack, i.e. CONSTANT = init("name",ordinal). I don't see any drawback with this approach, so it may be implemented in some compiler.

0: ldc           #23                 // String name
2: sipush        12345
5: invokestatic  #29                 // Method init:(Ljava/lang/String;I)Lcom/example/application/C;
8: putstatic     #26                 // Field CONSTANT:Lcom/example/application/C;
11: ...

Another optimization would be computing the ordinal in the static method, i.e. CONSTANT = init("name"); which requires 8 bytes (this one is more contrived, so I don't expect this to be implemented by a compiler, but I can try to achieve it with ASM, or by using a class with constants instead of the enum, so that we don't need to care about the ordinal):

0: ldc           #23                 // String name
2: invokestatic  #34                 // Method init:(Ljava/lang/String;)Lcom/example/application/MyEnum;
5: putstatic     #26                 // Field CONSTANT:Lcom/example/application/MyEnum;
8: ...

...and I don't suppose such an easy generator exists for v14? heh

I don't think so, but we can add something to that effect to the add-on pipeline.

Which version of the codebase should I be using for v14?

For Vaadin 14 use version 3.0.x (https://github.com/FlowingCode/FontAwesomeIronIconset/tree/3.0.x) which integrates with iron-icon.

javier-godoy commented 1 year ago

Hello. I renamed this ticket since I understand you no longer have issues with the iconset format. I have a POC based on my comments above, which pushes the enum limit from 4103 to 10921. I'll integrate it with the generator, so that you are able to build against the whole FontAwesome Pro iconset. I also created two issues for the other enhancements that were mentioned.

GrandyB commented 1 year ago

Aye, looks good to me - cheers.

javier-godoy commented 1 year ago

Hello. https://github.com/FlowingCode/FontAwesomeIronIconset/pull/70 (also cherry-picked into 4.x and 3.x branches) introduces an optional mechanism that allow compiling up to 10921 constants. You need to generate the enumerations with mvn -Pbigenum,generate clean package. If you want to run the demo with those settings, use mvn jetty:run -Dmaven.main.skip=true after building the package (in order to avoid javac from compiling the big enums, which will fail)