[#4615] Customizable estimation for messages written outside the EventLoop #4620
Conversation
|
This is for the 4.1 branch, I'll cross-port to 4.0/5.0 once you approve. Default task size is hardcoded, I could do something more fancy like detecting the JVM bitness and |
| buffer.incrementPendingOutboundBytes(task.size); | ||
|
|
||
| if (ESTIMATE_TASK_SIZE_ON_SUBMIT) { | ||
| task.size = ((AbstractChannel) ctx.channel()).estimatorHandle().size(msg) + WRITE_TASK_OVERHEAD; |
There was a problem hiding this comment.
@technocoreai just one nit.. can you move this in the if (buffer != null) { ... } block and if the buffer == null set the size = 0 ?
|
@technocoreai only one tiny comment. No need to port this to other branches, I will just cherry-pick it from 4.1 once in there. |
technocoreai
force-pushed
the
size-estimation-fix
branch
from
09e4c32
to
c5051c8
Compare
|
@normanmaurer I've updated the patch, it now initializes the size to 0 and then updates it if the estimation is enabled and channel is still open, is that fine? |
| // Check for null as it may be set to null if the channel is closed already | ||
| if (buffer != null) { | ||
| buffer.incrementPendingOutboundBytes(task.size); | ||
| task.size = 0; |
There was a problem hiding this comment.
not need to set this to 0 if ESTIMATE_TASK_SIZE_ON_SUBMIT is true. move this to an else block please.
There was a problem hiding this comment.
@normanmaurer Fixed, although I don't think that the size of a task written to a closed channel matters much.
… EventLoop Motivation: Estimation algorithm currently used for WriteTasks is complicated and wrong. Additionally, some code relies on outbound buffer size incremented only on actual writes to the outbound buffer. Modifications: - Throw away the old estimator and replace with a simple algorithm that uses the client-provided estimator along with a statically configured WriteTask overhead (io.netty.transport.writeTaskSizeOverhead system property with the default value of 48 bytes) - Add a io.netty.transport.estimateSizeOnSubmit boolean system property allowing the clients to disable the message estimation outside the event loop Result: Task estimation is user controllable and produces better results by default
technocoreai
force-pushed
the
size-estimation-fix
branch
from
c5051c8
to
ac9c1d9
Compare
|
Cherry-picked into 4.0 (d2ddb52) and 4.1 (cfd6793) @technocoreai thanks again! |
|
@normanmaurer The commit looks the same for branches and I don't see it in the log. Do you intend to push the 4.0 fix later? |
|
@technocoreai sorry I somehow thought I pushed to 4.0 as well. Just updated the comment to link to the correct commit sha and pushed it. d2ddb52 |
|
@normanmaurer thanks! |
Motivation:
Estimation algorithm currently used for WriteTasks is complicated and wrong. Additionally, some code relies on outbound buffer size incremented only on actual writes to the outbound buffer.
Modifications:
io.netty.transport.writeTaskSizeOverheadsystem property with the default value of 48 bytes)io.netty.transport.estimateSizeOnSubmitboolean system property allowing the clients to disable the message estimation outside the event loopResult:
Task estimation is user controllable and produces better results bydefault