-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Issue #6974 - improvements & fixes to ByteBufferPool implementations #7017
Issue #6974 - improvements & fixes to ByteBufferPool implementations #7017
Conversation
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>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
…g of bucket size. 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>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the changes are good, except for the concurrency model which feels broken and should be reviewed.
jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java
Outdated
Show resolved
Hide resolved
jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java
Outdated
Show resolved
Hide resolved
jetty-io/src/main/java/org/eclipse/jetty/io/MappedByteBufferPool.java
Outdated
Show resolved
Hide resolved
jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java
Outdated
Show resolved
Hide resolved
jetty-io/src/main/java/org/eclipse/jetty/io/AbstractByteBufferPool.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java
Outdated
Show resolved
Hide resolved
jetty-io/src/main/java/org/eclipse/jetty/io/LogArrayByteBufferPool.java
Outdated
Show resolved
Hide resolved
jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java
Outdated
Show resolved
Hide resolved
jetty-io/src/main/java/org/eclipse/jetty/io/LogArrayByteBufferPool.java
Outdated
Show resolved
Hide resolved
jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java
Outdated
Show resolved
Hide resolved
@gregw @lachlan-roberts overall I feel that these changes are too much for 9.4.x. |
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@sbordet @lachlan-roberts let's hangout tonight(tomorrow morning) |
... but note that whilst I don't like changes in 9.4, I also don't like the unlimited memory commitment that we currently allow with the default pool with no easy way to change it. |
Latest review with @lachlan-roberts. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Latest review with @lachlan-roberts. Fixed typo. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide a bytebufferpool-log
modules that provides bytebufferpool
and uses an alternative xml that configures the log byte buffers
jetty-io/src/main/java/org/eclipse/jetty/io/AbstractByteBufferPool.java
Outdated
Show resolved
Hide resolved
…74-WebSocketBufferPool
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
…7017) - WebSocket should user server ByteBufferPool if possible - fix various bugs ByteBufferPool implementations - add heuristic for maxHeapMemory and maxDirectMemory - Add dump for ByteBufferPools - add LogArrayByteBufferPool that does exponential scaling of bucket size. - ByteBufferPools should default to use maxMemory heuristic - Add module jetty-bytebufferpool-logarithmic Signed-off-by: Lachlan Roberts <lachlan@webtide.com> Co-authored-by: Simone Bordet <simone.bordet@gmail.com>
Issue #6974 - improvements to ByteBufferPool implementations
ByteBufferPool Changes
ArrayByteBufferPool
andMappedByteBufferPool
with non zero settings formaxHeapMemory
andmaxDirectMemory
.maxHeapMemory
andmaxDirectMemory
mean that a default is calculated based on the total max memory of the JVM.ArrayByteBufferPool
andMappedByteBufferPool
extensible to change the bucket scaling.LogArrayByteBufferPool
which doubles the size of each consecutive bucket instead of scaling linearly.Dumpable
for the ByteBufferPool implementations.WebSocket Changes
WebSocketUpgradeFilter
always waits until it is started before it gets theNativeWebSocketConfiguration
so it can be created when theByteBufferPool
is ready.ByteBufferPool
instead of creating its ownMappedByteBufferPool
every time.