Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Brotli4J uncoditionally brought in native executables #43556

Closed
zakkak opened this issue Sep 27, 2024 · 4 comments
Closed

Brotli4J uncoditionally brought in native executables #43556

zakkak opened this issue Sep 27, 2024 · 4 comments
Labels

Comments

@zakkak
Copy link
Contributor

zakkak commented Sep 27, 2024

Describe the bug

As unveiled in #43538 com.aayushatharva.brotli4j:brotli4j is being unconditionally included in native executables built with the quarkus-netty extension.

This is not causing any issues, but increases bloat with no apparent reason. The aim of this issue is to explore whether we can avoid always including brotli4j.

Expected behavior

Brotli4J should only be included when necessary (i.e. depending on the configuration)

Actual behavior

com.aayushatharva.brotli4j:brotli4j is being unconditionally included in native executables built with the quarkus-netty extension.

How to Reproduce?

./mvnw -Dnative -pl integration-tests/main -Dnative.surefire.skip -Dformat.skip -Dno-descriptor-tests clean package

Output of uname -a or ver

Linux fedora 6.10.10-200.fc40.x86_64 #1 SMP PREEMPT_DYNAMIC Thu Sep 12 18:26:09 UTC 2024 x86_64 GNU/Linux

Output of java -version

21.0.4+7-LTS

Mandrel or GraalVM version (if different from Java)

Mandrel-23.1.4.0-Final

Quarkus version or git rev

main 10ff88f

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.9.9 (8e8579a9e76f7d015ee5ec7bfcdc97d260186937)

Additional information

Applying the following patch seems to resolve the issue, but as expected it breaks integration-tests/vertx-http-compressors/all which depends on the brotli compressor.

diff --git a/extensions/netty/runtime/pom.xml b/extensions/netty/runtime/pom.xml
index 96b924e0ada..a0c59483ce2 100644
--- a/extensions/netty/runtime/pom.xml
+++ b/extensions/netty/runtime/pom.xml
@@ -61,15 +61,6 @@
             <classifier>osx-x86_64</classifier>
             <optional>true</optional>
         </dependency>
-
-        <!--
-        The recent version of Netty have a hard dependency on brotli,
-        without this dependency, it's not possible to compile to native
-        -->
-        <dependency>
-            <groupId>com.aayushatharva.brotli4j</groupId>
-            <artifactId>brotli4j</artifactId>
-        </dependency>
     </dependencies>
 
     <build>
diff --git a/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/options/HttpServerOptionsUtils.java b/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/options/HttpServerOptionsUtils.java
index 36fe46a9d3a..fce340e3662 100644
--- a/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/options/HttpServerOptionsUtils.java
+++ b/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/options/HttpServerOptionsUtils.java
@@ -16,7 +16,6 @@
 
 import org.jboss.logging.Logger;
 
-import io.netty.handler.codec.compression.BrotliOptions;
 import io.netty.handler.codec.compression.DeflateOptions;
 import io.netty.handler.codec.compression.GzipOptions;
 import io.netty.handler.codec.compression.StandardCompressionOptions;
@@ -294,13 +293,6 @@ public static void applyCommonOptions(HttpServerOptions httpServerOptions,
                     httpServerOptions.addCompressor(StandardCompressionOptions
                             .deflate(httpServerOptions.getCompressionLevel(), defaultOps.windowBits(), defaultOps.memLevel()));
                 } else if ("br".equalsIgnoreCase(compressor)) {
-                    final BrotliOptions o = StandardCompressionOptions.brotli();
-                    // The default compression level for brotli as of Netty Codec 4.1 is 4,
-                    // so we don't pick up Vert.x Core 4.5.7's default of 6. User can override:
-                    if (buildTimeConfig.compressionLevel.isPresent()) {
-                        o.parameters().setQuality(buildTimeConfig.compressionLevel.getAsInt());
-                    }
-                    httpServerOptions.addCompressor(o);
                 } else {
                     Logger.getLogger(HttpServerOptionsUtils.class).errorf("Unknown compressor: %s", compressor);
                 }

The comment:

The recent version of Netty have a hard dependency on brotli,
without this dependency, it's not possible to compile to native

Seems to no longer hold.

My expectation is that since quarkus.http.compressors is a build time configuration Quarkus should be able to achieve the desired functionality by depending on Brotli4J only when br is passed to quarkus.http.compressors.

An immediate improvement IMHO is to move the brotli4j dependency from netty to vertx-http for which I will prepare a PR (Update: see #43557).

@zakkak zakkak added kind/bug Something isn't working area/native-image labels Sep 27, 2024
Copy link

quarkus-bot bot commented Sep 27, 2024

/cc @Karm (mandrel), @galderz (mandrel)

zakkak added a commit to zakkak/quarkus that referenced this issue Sep 27, 2024
Brotli4J is necessary only when using `vertx-http` (for now), no need to
bring it in for every app using `quarkus-netty`

See quarkusio#43556 for more details
@Karm
Copy link
Member

Karm commented Sep 27, 2024

@zakkak It would be very valuable 🙏 , especially because when you don't configure br, it doesn't work anyway, so it's kinda useless to be there. At least that was the status the last time I've seen that parts of the woods in brotli config It didn't occur to me to fight it back then as I understood it's baked as Netty dependency and there is not much to be done about that (which is false...).

@gsmet gsmet added kind/enhancement New feature or request and removed kind/bug Something isn't working labels Sep 27, 2024
@zakkak
Copy link
Contributor Author

zakkak commented Sep 27, 2024

FWIW The size overhead of this issue is ~30KB so it might not be worth persuading after all. The main benefit from brotli exclusion would probably be that it won't be easy to accidentally bring in more things.

The classes being compiled in are:

1:com.aayushatharva.brotli4j.encoder.BrotliEncoderChannel
2:com.aayushatharva.brotli4j.encoder.Encoder
3:com.aayushatharva.brotli4j.encoder.Encoder$Parameters
4:com.aayushatharva.brotli4j.encoder.EncoderJNI
5:com.aayushatharva.brotli4j.encoder.EncoderJNI$Wrapper

and the methods:

1:com.aayushatharva.brotli4j.encoder.BrotliEncoderChannel.<init>(WritableByteChannel, Encoder$Parameters, int):void
2:com.aayushatharva.brotli4j.encoder.BrotliEncoderChannel.close():void
3:com.aayushatharva.brotli4j.encoder.BrotliEncoderChannel.write(ByteBuffer):int
4:com.aayushatharva.brotli4j.encoder.Encoder$Parameters.<init>():void
5:com.aayushatharva.brotli4j.encoder.Encoder$Parameters.access$000(Encoder$Parameters):int
6:com.aayushatharva.brotli4j.encoder.Encoder$Parameters.access$100(Encoder$Parameters):int
7:com.aayushatharva.brotli4j.encoder.Encoder$Parameters.access$200(Encoder$Parameters):Encoder$Mode
8:com.aayushatharva.brotli4j.encoder.Encoder$Parameters.setQuality(int):Encoder$Parameters
9:com.aayushatharva.brotli4j.encoder.Encoder.<init>(WritableByteChannel, Encoder$Parameters, int):void
10:com.aayushatharva.brotli4j.encoder.Encoder.close():void
11:com.aayushatharva.brotli4j.encoder.Encoder.encode(EncoderJNI$Operation):boolean
12:com.aayushatharva.brotli4j.encoder.Encoder.fail(String):void
13:com.aayushatharva.brotli4j.encoder.Encoder.flush():void
14:com.aayushatharva.brotli4j.encoder.Encoder.pushOutput(boolean):boolean
15:com.aayushatharva.brotli4j.encoder.EncoderJNI$Wrapper.<init>(int, int, int, Encoder$Mode):void
16:com.aayushatharva.brotli4j.encoder.EncoderJNI$Wrapper.destroy():void
17:com.aayushatharva.brotli4j.encoder.EncoderJNI$Wrapper.getInputBuffer():ByteBuffer
18:com.aayushatharva.brotli4j.encoder.EncoderJNI$Wrapper.hasMoreOutput():boolean
19:com.aayushatharva.brotli4j.encoder.EncoderJNI$Wrapper.hasRemainingInput():boolean
20:com.aayushatharva.brotli4j.encoder.EncoderJNI$Wrapper.isSuccess():boolean
21:com.aayushatharva.brotli4j.encoder.EncoderJNI$Wrapper.pull():ByteBuffer
22:com.aayushatharva.brotli4j.encoder.EncoderJNI$Wrapper.push(EncoderJNI$Operation, int):void
23:com.aayushatharva.brotli4j.encoder.EncoderJNI.nativeCreate(long[]):ByteBuffer
24:com.aayushatharva.brotli4j.encoder.EncoderJNI.nativeDestroy(long[]):void
25:com.aayushatharva.brotli4j.encoder.EncoderJNI.nativePull(long[]):ByteBuffer
26:com.aayushatharva.brotli4j.encoder.EncoderJNI.nativePush(long[], int):void

zakkak added a commit to zakkak/quarkus that referenced this issue Sep 30, 2024
Brotli4J is necessary only when using `vertx-http` (for now), no need to
bring it in for every app using `quarkus-netty`

See quarkusio#43556 for more details
mskacelik pushed a commit to mskacelik/quarkus that referenced this issue Oct 2, 2024
Brotli4J is necessary only when using `vertx-http` (for now), no need to
bring it in for every app using `quarkus-netty`

See quarkusio#43556 for more details
@zakkak zakkak changed the title Brotli4J uncoditionally brough it native executables Brotli4J uncoditionally brought it native executables Oct 4, 2024
@zakkak zakkak changed the title Brotli4J uncoditionally brought it native executables Brotli4J uncoditionally brought in native executables Oct 4, 2024
@zakkak
Copy link
Contributor Author

zakkak commented Oct 15, 2024

The only way I can think about fixing this would be to implicitly add the brotli dependency only when br is set in the compressors but AFAIK there is no mechanism for something like this at the moment. Removing the dependency when br is not there is not safe as users might need it for other things.

I am closing it as not planned since as mentioned in #43556 (comment) the overhead is insignificant.

@zakkak zakkak closed this as not planned Won't fix, can't repro, duplicate, stale Oct 15, 2024
bschuhmann pushed a commit to bschuhmann/quarkus that referenced this issue Nov 16, 2024
Brotli4J is necessary only when using `vertx-http` (for now), no need to
bring it in for every app using `quarkus-netty`

See quarkusio#43556 for more details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants