Skip to content
This repository has been archived by the owner on Jun 19, 2021. It is now read-only.

New network system #340

Merged
merged 18 commits into from
Jan 23, 2021
Merged

New network system #340

merged 18 commits into from
Jan 23, 2021

Conversation

HookWoods
Copy link
Contributor

Hello, I'm porting a new patch to yatopia about the network.
The network is a part of spigot that has never been too much modified and can be optimized with some kernel parameters (I may add TCP_FASTOPEN in the future or other params like this).

Actually this patch only enhanced the code readability of the ServerConnection and add a new beta feature:
IOUring, it's a new async interface for IO introduce in Kernel 5.4 in Linux (So only work on linux with kernel >= 5.4)
Netty is actually working on a project to port IOUring on it (https://github.com/netty/netty-incubator-transport-io_uring).
This is actually in beta so that's why there is a config to enable it or disable it, but can offer some performance on the network.

There is also a known issue with Zlib in Bungeecord that makes IOUring unusable cause it can decompress packet from it
(netty/netty-incubator-transport-io_uring#40).

Copy link
Contributor

@MrIvanPlays MrIvanPlays left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no readability changes. minimal diff

@MrIvanPlays MrIvanPlays added the enhancement New feature or request label Jan 18, 2021
@MrIvanPlays
Copy link
Contributor

overall try to minimise the diff as much as possible

@duplexsystem
Copy link
Collaborator

I'd like to implement velocity's VelocityNettyThreadFactory and some minor formatting improvements before merge

@Titaniumtown
Copy link
Collaborator

Hmm very interesting PR.

@MrIvanPlays
Copy link
Contributor

chunks of code are still not commented out properly

MrIvanPlays
MrIvanPlays previously approved these changes Jan 20, 2021
Copy link
Contributor

@MrIvanPlays MrIvanPlays left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like these changes.
I am aware of channel factories for like ages but I'm just too afraid to touch anything with this code. Looks like you've done it.

@YatopiaMC/review-team I need 1 more person to review it.

MrIvanPlays
MrIvanPlays previously approved these changes Jan 20, 2021
Copy link
Collaborator

@duplexsystem duplexsystem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I cannot connect at all with IOUring enabled the first or current build
  2. I think we should port Velocity's VelocityNettyThreadFactory because it uses a netty thread pool which will probably work better with netty
  3. we need to drop the log level of trying to using different network types to info

@MrIvanPlays
Copy link
Contributor

I think we should port Velocity's VelocityNettyThreadFactory because it uses a netty thread pool which will probably work better with netty

um this pr also uses netty pool. the thread factory quite literally doesn't matter. velocity's is just changing the name. same for ours.

we need to drop the log level of trying to using different network types to info

ill remove it

@duplexsystem
Copy link
Collaborator

duplexsystem commented Jan 20, 2021

um this pr also uses netty pool. the thread factory quite literally doesn't matter. velocity's is just changing the name. same for ours.

No it uses com.google.common.util.concurrent.ThreadFactoryBuilder. VelocityNettyThreadFactory uses io.netty.util.concurrent.FastThreadLocalThread and java.util.concurrent.ThreadFactory and I frankly trust astei's judgement here more than yours.

@MrIvanPlays
Copy link
Contributor

things left:
fix io uring
update patches.md

duplexsystem
duplexsystem previously approved these changes Jan 21, 2021
Copy link
Contributor

@MrIvanPlays MrIvanPlays left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking cuz i want to do some changes before merge

MrIvanPlays
MrIvanPlays previously approved these changes Jan 21, 2021
duplexsystem
duplexsystem previously approved these changes Jan 21, 2021
@MrIvanPlays MrIvanPlays force-pushed the ver/1.16.5 branch 2 times, most recently from c232045 to ecf1662 Compare January 22, 2021 14:33
@HookWoods HookWoods dismissed stale reviews from duplexsystem and MrIvanPlays via 538a94c January 22, 2021 17:42
@Titaniumtown
Copy link
Collaborator

Titaniumtown commented Jan 22, 2021

Crap, it seems I had old review pending, lmao. Seems they all still apply though xD

patches/server/0067-Port-krypton.patch Outdated Show resolved Hide resolved
patches/server/0067-Port-krypton.patch Outdated Show resolved Hide resolved
patches/server/0067-Port-krypton.patch Outdated Show resolved Hide resolved
MrIvanPlays
MrIvanPlays previously approved these changes Jan 22, 2021
Titaniumtown
Titaniumtown previously approved these changes Jan 22, 2021
@duplexsystem duplexsystem dismissed stale reviews from Titaniumtown and MrIvanPlays via 0fe50f5 January 23, 2021 18:57
@duplexsystem duplexsystem merged commit b27d77c into ver/1.16.5 Jan 23, 2021
@duplexsystem duplexsystem deleted the dev/network branch January 23, 2021 18:57
@krolik-exe
Copy link
Contributor

how do I check if IOUring is working, because i enabled it in the config and when i boot up it shows using EPOLL network type and showing that it's using velocity compression and chipher, and doesn't show any errors

@HookWoods
Copy link
Contributor Author

Actually you need to set network compression to -1 and bungee to false to make it works, after you can enable the debug that will tell you why io uring is not working after you did all that thing before

@krolik-exe
Copy link
Contributor

ok i will try this

@krolik-exe
Copy link
Contributor

Thanks, it works now

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants