Issue #5198 - update gzip handler #5248
Conversation
…sitive Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
…Pool Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
The CRC32 checksum may need to convert the ByteBuffer to an array anyway so we are better off not setting the deflater input with ByteBuffer directly. Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpOutputInterceptor.java
Show resolved
Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHandler.java
Outdated
Show resolved
Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHandler.java
Outdated
Show resolved
Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHandler.java
Show resolved
Hide resolved
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
| { | ||
| return new DeflaterPool(capacity, Deflater.DEFAULT_COMPRESSION, true); | ||
| return new DeflaterPool(CompressionPool.INFINITE_CAPACITY, Deflater.DEFAULT_COMPRESSION, true); |
There was a problem hiding this comment.
What does "INFINITE_CAPACITY" mean?
There was a problem hiding this comment.
It means there is no limit on the number of objects allowed to be pooled.
This is now changed later by the setter, as I have made the capacity of the pool configurable after it is constructed. This lets us make the pool final and add it as a bean to the GzipHandler.
There was a problem hiding this comment.
We want to eventually switch the pools over to using the Pool class, which has it's size set when constructed. So we need to not assume that the pool size can be configured after construction.
There was a problem hiding this comment.
Opened PR #5295, which uses the new Pool class for the CompressionPool and still allows the pool size to be changed before it is started.
|
@gregw this PR is ready for review. |
|
@lachlan-roberts OK looking... but can you start a second PR that will replace the inflater and deflater pools with a Pool. This can be in 9.4 |
| { | ||
| return new DeflaterPool(capacity, Deflater.DEFAULT_COMPRESSION, true); | ||
| return new DeflaterPool(CompressionPool.INFINITE_CAPACITY, Deflater.DEFAULT_COMPRESSION, true); |
There was a problem hiding this comment.
We want to eventually switch the pools over to using the Pool class, which has it's size set when constructed. So we need to not assume that the pool size can be configured after construction.
| _deflater.setInput(array, off, len); // TODO use ByteBuffer API in Jetty-10 | ||
| // Ideally we would want to use the ByteBuffer API for Deflaters. However due the the ByteBuffer implementation | ||
| // of the CRC32.update() it is less efficient for us to use this rather than to convert to array ourselves. | ||
| _deflater.setInput(array, off, len); |
There was a problem hiding this comment.
Good comment, but can we raise a bug on the JVM to provide an efficient implementation of CRC32 that does not copy ByteBuffers?
There was a problem hiding this comment.
It only does the copying if it is not a DirectBuffer and does not have an array.
I don't think I can raise JVM bugs anyway, @sbordet could do this.
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
|
@gregw I think we should merge this before considering the further changes to make |
Fixes #5198 - We now use the ByteBuffer API for inflaters/deflaters for
GzipHandler.Fixes #4988 - The GzipHandlers
IncludeExcludefor the MIME types now uses case insensitive set.Fixes #1761 - Added extra configuration in the .ini file for
gzip.mod.Fixes #5246 - add deflater/inflater pools to server dump.