-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Consolidate flushes #3785
base: master
Are you sure you want to change the base?
Consolidate flushes #3785
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ | |
|
||
public class ChannelWrapper | ||
{ | ||
private static final int MAX_CONSOLIDATION = Integer.getInteger( "net.md_5.bungee.flush-consolidation-limit", 20 ); | ||
|
||
private final Channel ch; | ||
@Getter | ||
|
@@ -33,13 +34,21 @@ public class ChannelWrapper | |
private volatile boolean closed; | ||
@Getter | ||
private volatile boolean closing; | ||
private boolean consolidate; | ||
private int consolidationCounter; | ||
|
||
public ChannelWrapper(ChannelHandlerContext ctx) | ||
{ | ||
this.ch = ctx.channel(); | ||
this.remoteAddress = ( this.ch.remoteAddress() == null ) ? this.ch.parent().localAddress() : this.ch.remoteAddress(); | ||
} | ||
|
||
public void setConsolidate(boolean enabled) | ||
{ | ||
consolidate = enabled; | ||
forceFlush(); | ||
} | ||
|
||
public Protocol getDecodeProtocol() | ||
{ | ||
return getMinecraftDecoder().getProtocol(); | ||
|
@@ -57,6 +66,8 @@ public Protocol getEncodeProtocol() | |
|
||
public void setEncodeProtocol(Protocol protocol) | ||
{ | ||
// before changing the encoder protocol we should always flush to ensure no wrong states | ||
forceFlush(); | ||
getMinecraftEncoder().setProtocol( protocol ); | ||
} | ||
|
||
|
@@ -89,18 +100,27 @@ public int getEncodeVersion() | |
|
||
public void write(Object packet) | ||
{ | ||
// ensure netty context to for less context schedules | ||
// by default we are mostly in netty context anyway, but plugins can use ProxiedPlayer.unsafe().sendPacket() | ||
// in non netty context | ||
if ( !ch.eventLoop().inEventLoop() ) | ||
{ | ||
ch.eventLoop().execute( () -> write( packet ) ); | ||
return; | ||
} | ||
|
||
if ( !closed ) | ||
{ | ||
DefinedPacket defined = null; | ||
if ( packet instanceof PacketWrapper ) | ||
{ | ||
PacketWrapper wrapper = (PacketWrapper) packet; | ||
wrapper.setReleased( true ); | ||
ch.writeAndFlush( wrapper.buf, ch.voidPromise() ); | ||
ch.write( wrapper.buf, ch.voidPromise() ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would put in the signalFlush call directly after both write calls instead of putting it below. Or maybe add in a write0 method in ChannelWrapper taking care of potentially flushing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why? signalFlush is called guaranteed at the end of the method. Only if the encoder protocol is changed it will forceFlush and return. |
||
defined = wrapper.packet; | ||
} else | ||
{ | ||
ch.writeAndFlush( packet, ch.voidPromise() ); | ||
ch.write( packet, ch.voidPromise() ); | ||
if ( packet instanceof DefinedPacket ) | ||
{ | ||
defined = (DefinedPacket) packet; | ||
|
@@ -113,11 +133,32 @@ public void write(Object packet) | |
if ( nextProtocol != null ) | ||
{ | ||
setEncodeProtocol( nextProtocol ); | ||
return; | ||
} | ||
} | ||
signalFlush(); | ||
} | ||
} | ||
|
||
|
||
|
||
private void signalFlush() | ||
{ | ||
if ( !consolidate || consolidationCounter++ >= MAX_CONSOLIDATION ) | ||
{ | ||
forceFlush(); | ||
} | ||
|
||
} | ||
|
||
public void forceFlush() | ||
{ | ||
ch.flush(); | ||
consolidationCounter = 0; | ||
} | ||
|
||
|
||
|
||
public void markClosed() | ||
{ | ||
closed = closing = true; | ||
|
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.
Would this then be best placed in impl of ProxiedPlayer.Unsafe.sendPacket?
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.
if i remove it there i need to put it at multiple positions as ServerConnection.unsafe.sendPacket InitialHandler.unsafe.sendpacket and ProxiedPlayer.unsafe.sendPacket
It was just the most quick way to do it there for me because i thought there is no disadvantage of putting it there?
Why dou you think it shouldnt be there?