Consolidate flushes

[Outfluencer]

Inspired by @Janmm14 #3393

I impled a simple flush consolidation without modifing the netty pipeline this change consists of tweaks in PacketHandlers, channelWrapper and HandlerBoss

There is a system variable to set the max consolidate amount

Consolidating is enabled after the backend receives the login packet to ensure all packets in login and config are sent without delay and cannot get lost.

And is disabled while switching to another server

It would be nice if it would be tested.

I will probably test it on network of a friend but more is better (i tested it on my localhost everything was working fine for me)

Closes #3392

[Janmm14]

( #3393 )

[md-5]

Any pros/cons compared to the Janm's? One pro obviously is that this is much simpler

[Outfluencer]

The functionallity should be the same, i don‘t see any cons and other than its simpler no pros.

Only functonality differents is that i disabled consolidating while the player might be in config state. To ensure the server switches are as fast as possible. (

But maybe this could be removed.

[md-5]

The functionallity should be the same, i don‘t see any cons and other than its simpler no pros.

Only functonality differents is that i disabled consolidating while the player might be in config state. To ensure the server switches are as fast as possible. (

But maybe this could be removed.

Nah that sounds like a good idea

[Outfluencer]

The functionallity should be the same, i don‘t see any cons and other than its simpler no pros.
Only functonality differents is that i disabled consolidating while the player might be in config state. To ensure the server switches are as fast as possible. (
But maybe this could be removed.

Nah that sounds like a good idea

good idea to remove it? or to have it disabled while config state? xd

[md-5]

No keep as is

[Outfluencer]

Alright

[Outfluencer]

Here is a direct download link to the compiled binary with this patch applied
https://outfluencer.dev/BungeeCord-Consolidate-Flushes.jar

If someone is intrested in testing it

[Outfluencer]

@andreasdc would you like to test it?

[Janmm14]

Would this then be best placed in impl of ProxiedPlayer.Unsafe.sendPacket?

[Outfluencer]

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?

[Janmm14]

I would put in the signalFlush call directly after both write calls instead of putting it below.
Or is there something I didn't notice

Or maybe add in a write0 method in ChannelWrapper taking care of potentially flushing?

[Outfluencer]

why?

signalFlush is called guaranteed at the end of the method. Only if the encoder protocol is changed it will forceFlush and return.

[Janmm14]

Looking at my impl copied form netty, we are not flushing on disconnect/close/exception/writeability change/handlerRemoved here. I think these should be added.

[Janmm14]

Also I think a value of 20 is kinda low. We might not even fill up a whole tcp packet of 1500 bytes with 20 of the most common packets being very small (delta move-rot <=14 / single block change <= 13 /block destroy stage <= 14 / animation <= 5 / exporb <=31 / statistic <= 15).

As we flush anyway once reading from the other connection is finished temporarily, the counting for flushes during read is only to free up internal buffers of the connection. Therefore it might be more beneficial if we use packet size instead of packet amount for this matter.
We could use the original packet size on forwarding and maybe estimate packets created on bungee to be like 50 or 100 bytes in size (I'm thinking of command message size as most common).

[Outfluencer]

Also I think a value of 20 is kinda low. We might not even fill up a whole tcp packet of 1500 bytes with 20 of the most common packets being very small (delta move-rot <=14 / single block change <= 13 /block destroy stage <= 14 / animation <= 5 / exporb <=31 / statistic <= 15).

As we flush anyway once reading from the other connection is finished temporarily, the counting for flushes during read is only to free up internal buffers of the connection. Therefore it might be more beneficial if we use packet size instead of packet amount for this matter. We could use the original packet size on forwarding and maybe estimate packets created on bungee to be like 50 or 100 bytes in size (I'm thinking of command message size as most common).

i used 20 because you also did

[Outfluencer]

Looking at my impl copied form netty, we are not flushing on disconnect/close/exception/writeability change/handlerRemoved here. I think these should be added.

as far as i know we don't need to flush on close exception or disconnect.

But we should flush on writeability change, also bungee never remove something from the pipe at time i enable consolidation so handlerRemoved is also not required i guess

[Outfluencer]

Also I think a value of 20 is kinda low. We might not even fill up a whole tcp packet of 1500 bytes with 20 of the most common packets being very small (delta move-rot <=14 / single block change <= 13 /block destroy stage <= 14 / animation <= 5 / exporb <=31 / statistic <= 15).

As we flush anyway once reading from the other connection is finished temporarily, the counting for flushes during read is only to free up internal buffers of the connection. Therefore it might be more beneficial if we use packet size instead of packet amount for this matter. We could use the original packet size on forwarding and maybe estimate packets created on bungee to be like 50 or 100 bytes in size (I'm thinking of command message size as most common).

isnt there a risk of higher latency if we do so?

its very important to me that we ensure no latency increase (20 flushed to 1 flush is already a big performance increase)

[Janmm14]

Also I think a value of 20 is kinda low. We might not even fill up a whole tcp packet of 1500 bytes with 20 of the most common packets being very small (delta move-rot <=14 / single block change <= 13 /block destroy stage <= 14 / animation <= 5 / exporb <=31 / statistic <= 15).
As we flush anyway once reading from the other connection is finished temporarily, the counting for flushes during read is only to free up internal buffers of the connection. Therefore it might be more beneficial if we use packet size instead of packet amount for this matter. We could use the original packet size on forwarding and maybe estimate packets created on bungee to be like 50 or 100 bytes in size (I'm thinking of command message size as most common).

i used 20 because you also did

Oh, well that number was pulled out of thin air (i did think only a very short time about that number). Should've done better at that time. Looking at the common packet sizes I mentioned, it might not be optimal.

isnt there a risk of higher latency if we do so?

its very important to me that we ensure no latency increase (20 flushed to 1 flush is already a big performance increase)

I would guess that might only be if there is some large chunk packet which we are now waiting to get processed with the new threshold, keeping smaller packets from before to get sent.
In such cases, a flush before writing a large packet might be the better strategy (if there is something which could be flushed).
I would guess we wouldn't add any visible latency wheter we're processing 20, 40 or 60 small packets.

In general we're both just doing a bunch of guesswork, some measurements are needed, e.g. on write, save nanoTime into a long[22] array (i think threshold 20 leads to up to 21 packets until flush + 1 spare if i messed up), compare with nanoTime just before flush call & print to console or so.
At best done with a backend server impl which is doing the flush-only-once-per-tick, although it'd be interesting to see the actual behaviour of instant flushing of older servers maybe still leading to continuous packet streams due to potetial lower level shenanigans.
As I do not have some server operator friend and also don't operate one myself, measurements i'd do won't reflect reality well - not enough packets, too small sample size.

[Outfluencer]

Oh, well that number was pulled out of thin air (i did think only a very short time about that number). Should've done better at that time. Looking at the common packet sizes I mentioned, it might not be optimal.

i understand, i also just put 20 there without much thinking about it, but it seemed using 40 instead of 20 makes no differents in latency (maybe more also has no differents)

In general we're both just doing a bunch of guesswork, some measurements are needed, e.g. on write, save nanoTime into a long[22] array (i think threshold 20 leads to up to 21 packets until flush + 1 spare if i messed up), compare with nanoTime just before flush call & print to console or so. At best done with a backend server impl which is doing the flush-only-once-per-tick, although it'd be interesting to see the actual behaviour of instant flushing of older servers maybe still leading to continuous packet streams due to potetial lower level shenanigans. As I do not have some server operator friend and also don't operate one myself, measurements i'd do won't reflect reality well - not enough packets, too small sample size.

I fully agree this needs more testing.
I will try to put his change on some big servers and see if the users notice any difference. i think pvp could be the biggest problem (delayed hits or something)