Re: MINA read buffer default size: is it too small ?
On 17/01/2022 15:24, Jonathan Valliere wrote: Emmanuel, The socket receive side buffer is stored for as long as necessary. You are referring to the clear-text application buffer here https://github.com/apache/mina/blob/660ab2375b4b47b5ebe86226c92f3138be4c96e8/mina-core/src/main/java/org/apache/mina/filter/ssl/SSLHandler.java#L277 It would be possible to cache this in a Thread-Local-style however, not all SSL sessions have the same parameters so you risk necessary expansions; I was thinking that we could use a big enough buffer to fit all the needs (like 32kb), assuming the compression method is always NULL, but it may not be the case, so yeah, it might not be such a good idea. additionally you would have to permanently remove that buffer from Thread-Local once anything is written into it for safety of the user-application. Have to be very careful not to maintain any pointers once that or a section of that buffer has been passed onto the next downstream filter. Indeed. Zeroing the buffer after each usage probably makes the gain of having a unique buffer pretty null. All in all, allocating 16Kb of data is quite fast in modern JVM, and even if it has to be zeroed, it won't remain in memory for a long time. Doesn't IoBuffer have a built-in caching mechanism? Not by default. We use a SimpleBufforAllocator. Although there is a CachedBufferAllocator, it's not use anywhere. free() is called when its no longer needed which should make it fully compatible https://github.com/apache/mina/blob/660ab2375b4b47b5ebe86226c92f3138be4c96e8/mina-core/src/main/java/org/apache/mina/filter/ssl/SSLHandlerG0.java#L201 There is some room for improvements on decoding as many messages as possible on the receive side before looping. I will think about that. Ok, thanks ! -- Emmanuel Lécharny - To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org For additional commands, e-mail: dev-h...@mina.apache.org
Re: MINA read buffer default size: is it too small ?
Emmanuel, The socket receive side buffer is stored for as long as necessary. You are referring to the clear-text application buffer here https://github.com/apache/mina/blob/660ab2375b4b47b5ebe86226c92f3138be4c96e8/mina-core/src/main/java/org/apache/mina/filter/ssl/SSLHandler.java#L277 It would be possible to cache this in a Thread-Local-style however, not all SSL sessions have the same parameters so you risk necessary expansions; additionally you would have to permanently remove that buffer from Thread-Local once anything is written into it for safety of the user-application. Have to be very careful not to maintain any pointers once that or a section of that buffer has been passed onto the next downstream filter. Doesn't IoBuffer have a built-in caching mechanism? free() is called when its no longer needed which should make it fully compatible https://github.com/apache/mina/blob/660ab2375b4b47b5ebe86226c92f3138be4c96e8/mina-core/src/main/java/org/apache/mina/filter/ssl/SSLHandlerG0.java#L201 There is some room for improvements on decoding as many messages as possible on the receive side before looping. I will think about that. On Mon, Jan 17, 2022 at 1:30 AM Emmanuel Lécharny wrote: > > > On 14/01/2022 14:46, Jonathan Valliere wrote: > > > https://github.com/apache/mina/blob/c0ee516726c21115b196834ee984be786cea5818/mina-core/src/main/java/org/apache/mina/filter/ssl/SSLHandler.java#L277 > > > > in 2.2.X the decode buffer is automatically calculated based on the input > > size so the entire input buffer can be decoded. These buffers cannot be > > cached anywhere like Thread-Local because the decoded buffer must be > > cleanly passed onto the next filter which may or may not go asynchronous. > > > Hi Jonathan, > > when we receive data, we first allocate a 2048 buffer (default size, can > be changed with config), read the data and propagate it to the chain. > > The Sslhandler first step is to allocate a big (16Kb) buffer before > calling SslEngine.unwrap(). > > This is where I think it might make sense to use a TLS stored buffer: > - we reuse it in SslHandler for the app buffer > - if unwrap does not have any byte produced (HS), then we can simply > reset the buffer > - otherwise we can now allocate the proper buffer (which is likely to be > way smaller) to fit the clear text data into it, instead of passing the > big allocated buffer to the next filter. > > The gain would be: > - avoiding allocating a buffer during the HS when unwrap does not > produce anything > - creating smaller objects in the standard processing (ie data processing) > > Emmanuel > > - > To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org > For additional commands, e-mail: dev-h...@mina.apache.org > >
Re: MINA read buffer default size: is it too small ?
On 14/01/2022 14:46, Jonathan Valliere wrote: https://github.com/apache/mina/blob/c0ee516726c21115b196834ee984be786cea5818/mina-core/src/main/java/org/apache/mina/filter/ssl/SSLHandler.java#L277 in 2.2.X the decode buffer is automatically calculated based on the input size so the entire input buffer can be decoded. These buffers cannot be cached anywhere like Thread-Local because the decoded buffer must be cleanly passed onto the next filter which may or may not go asynchronous. Hi Jonathan, when we receive data, we first allocate a 2048 buffer (default size, can be changed with config), read the data and propagate it to the chain. The Sslhandler first step is to allocate a big (16Kb) buffer before calling SslEngine.unwrap(). This is where I think it might make sense to use a TLS stored buffer: - we reuse it in SslHandler for the app buffer - if unwrap does not have any byte produced (HS), then we can simply reset the buffer - otherwise we can now allocate the proper buffer (which is likely to be way smaller) to fit the clear text data into it, instead of passing the big allocated buffer to the next filter. The gain would be: - avoiding allocating a buffer during the HS when unwrap does not produce anything - creating smaller objects in the standard processing (ie data processing) Emmanuel - To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org For additional commands, e-mail: dev-h...@mina.apache.org
Re: MINA read buffer default size: is it too small ?
On 14/01/2022 14:46, Jonathan Valliere wrote:
https://github.com/apache/mina/blob/c0ee516726c21115b196834ee984be786cea5818/mina-core/src/main/java/org/apache/mina/filter/ssl/SSLHandler.java#L277
in 2.2.X the decode buffer is automatically calculated based on the input
size so the entire input buffer can be decoded.
Same thing in 2.1.X:
https://github.com/apache/mina/blob/e49579069e49d733d8f75bc67b2e16311b619ffe/mina-core/src/main/java/org/apache/mina/filter/ssl/SslHandler.java#L762
private SSLEngineResult unwrap() throws SSLException {
// We first have to create the application buffer if it does
not exist
if (appBuffer == null) {
appBuffer = IoBuffer.allocate(inNetBuffer.remaining());
These buffers cannot be
cached anywhere like Thread-Local because the decoded buffer must be
cleanly passed onto the next filter which may or may not go asynchronous.
Yep, but I think it should be the responsability of the executor to deal
with a buffer copy, otherwise we should pass the buffer up the stream
without any copy. Thus a TLS buffer could be safely used.
Btw, we also copy the incoming buffer *before* passing it to the SslEngine:
https://github.com/apache/mina/blob/e49579069e49d733d8f75bc67b2e16311b619ffe/mina-core/src/main/java/org/apache/mina/filter/ssl/SslHandler.java#L348
SslHandler.messageReceived(NextFilter nextFilter, ByteBuffer buf)
throws SSLException {
...
// append buf to inNetBuffer
if (inNetBuffer == null) {
inNetBuffer =
IoBuffer.allocate(buf.remaining()).setAutoExpand(true);
}
inNetBuffer.put(buf);
...
unless we have more data in the buffer to process:
https://github.com/apache/mina/blob/e49579069e49d733d8f75bc67b2e16311b619ffe/mina-core/src/main/java/org/apache/mina/filter/ssl/SslHandler.java#L365
// prepare to be written again
if (inNetBuffer.hasRemaining()) {
inNetBuffer.compact();
} else {
inNetBuffer.free();
inNetBuffer = null;
}
That is a waste, IMO
On Fri, Jan 14, 2022 at 7:38 AM Jonathan Valliere
wrote:
Please point to the line of code in GitHub. GitHub has a great tool for
linking directly to lines.
On Fri, Jan 14, 2022 at 3:49 AM Emmanuel Lécharny
wrote:
Hi,
I'm reviewing some parts of MINA (more specifically the SSL part), and I
found that we default the read buffer to 2048 bytes, which seems a bit
too small to me.
Typically, when handling TLS messages, we may have up to 16384 PDUs
(max). If the buffer is too small, we havve to do some extra read up to
the point we have received all the needed data.
At this point, I wonder if it wouldn't be a better idea to size the
buffer to at least the max PDU size of a TLS message (ie 16384 bytes).
The drawback is that it may be more demanding on the GC, as we will
allocate bigger buffers for each incoming message.
Incenditally I wonder if it wouldn't be a good idea to use a Thread
Local Storage to keep the allocated buffer, and avoiding the allocation
of such a buffer for every call (assuming we copy it if there is an
executor in the chain).
Thoughts ?
--
Emmanuel Lécharny
-
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For additional commands, e-mail: dev-h...@mina.apache.org
--
*Emmanuel Lécharny - CTO* 205 Promenade des Anglais – 06200 NICE
T. +33 (0)4 89 97 36 50
P. +33 (0)6 08 33 32 61
emmanuel.lecha...@busit.com https://www.busit.com/
-
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For additional commands, e-mail: dev-h...@mina.apache.org
Re: MINA read buffer default size: is it too small ?
https://github.com/apache/mina/blob/c0ee516726c21115b196834ee984be786cea5818/mina-core/src/main/java/org/apache/mina/filter/ssl/SSLHandler.java#L277 in 2.2.X the decode buffer is automatically calculated based on the input size so the entire input buffer can be decoded. These buffers cannot be cached anywhere like Thread-Local because the decoded buffer must be cleanly passed onto the next filter which may or may not go asynchronous. On Fri, Jan 14, 2022 at 7:38 AM Jonathan Valliere wrote: > Please point to the line of code in GitHub. GitHub has a great tool for > linking directly to lines. > > On Fri, Jan 14, 2022 at 3:49 AM Emmanuel Lécharny > wrote: > >> Hi, >> >> I'm reviewing some parts of MINA (more specifically the SSL part), and I >> found that we default the read buffer to 2048 bytes, which seems a bit >> too small to me. >> >> Typically, when handling TLS messages, we may have up to 16384 PDUs >> (max). If the buffer is too small, we havve to do some extra read up to >> the point we have received all the needed data. >> >> At this point, I wonder if it wouldn't be a better idea to size the >> buffer to at least the max PDU size of a TLS message (ie 16384 bytes). >> >> The drawback is that it may be more demanding on the GC, as we will >> allocate bigger buffers for each incoming message. >> >> >> Incenditally I wonder if it wouldn't be a good idea to use a Thread >> Local Storage to keep the allocated buffer, and avoiding the allocation >> of such a buffer for every call (assuming we copy it if there is an >> executor in the chain). >> >> >> Thoughts ? >> >> -- >> Emmanuel Lécharny >> >> - >> To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org >> For additional commands, e-mail: dev-h...@mina.apache.org >> >>
Re: MINA read buffer default size: is it too small ?
Please point to the line of code in GitHub. GitHub has a great tool for linking directly to lines. On Fri, Jan 14, 2022 at 3:49 AM Emmanuel Lécharny wrote: > Hi, > > I'm reviewing some parts of MINA (more specifically the SSL part), and I > found that we default the read buffer to 2048 bytes, which seems a bit > too small to me. > > Typically, when handling TLS messages, we may have up to 16384 PDUs > (max). If the buffer is too small, we havve to do some extra read up to > the point we have received all the needed data. > > At this point, I wonder if it wouldn't be a better idea to size the > buffer to at least the max PDU size of a TLS message (ie 16384 bytes). > > The drawback is that it may be more demanding on the GC, as we will > allocate bigger buffers for each incoming message. > > > Incenditally I wonder if it wouldn't be a good idea to use a Thread > Local Storage to keep the allocated buffer, and avoiding the allocation > of such a buffer for every call (assuming we copy it if there is an > executor in the chain). > > > Thoughts ? > > -- > Emmanuel Lécharny > > - > To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org > For additional commands, e-mail: dev-h...@mina.apache.org > >
MINA read buffer default size: is it too small ?
Hi, I'm reviewing some parts of MINA (more specifically the SSL part), and I found that we default the read buffer to 2048 bytes, which seems a bit too small to me. Typically, when handling TLS messages, we may have up to 16384 PDUs (max). If the buffer is too small, we havve to do some extra read up to the point we have received all the needed data. At this point, I wonder if it wouldn't be a better idea to size the buffer to at least the max PDU size of a TLS message (ie 16384 bytes). The drawback is that it may be more demanding on the GC, as we will allocate bigger buffers for each incoming message. Incenditally I wonder if it wouldn't be a good idea to use a Thread Local Storage to keep the allocated buffer, and avoiding the allocation of such a buffer for every call (assuming we copy it if there is an executor in the chain). Thoughts ? -- Emmanuel Lécharny - To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org For additional commands, e-mail: dev-h...@mina.apache.org
