search

CobbleSword / NachoSpigot

NachoSpigot is a fork of TacoSpigot 1.8.9 that offers several enhancements to performance as well as bug fixes.
GNU General Public License v3.0
238 stars 89 forks source link

Slow packet sending #270

Closed andreasdc closed 2 years ago

andreasdc commented 2 years ago

Describe the feature

image

Technical implementation

Why don't we remove usage of getProtocolForPacket and set it to every packet once and then just take it? getProtocolForPacket and setProtocol are more expensive than sending actual packet/passing it to netty.

Other

No response

Agreements

andreasdc commented 2 years ago

Also 13 days ago Paper released this patch: https://github.com/PaperMC/Paper/blob/master/patches/server/0310-Optimize-Network-Manager-and-add-advanced-packet-sup.patch

ghost commented 2 years ago

Also 13 days ago Paper released this patch: https://github.com/PaperMC/Paper/blob/master/patches/server/0310-Optimize-Network-Manager-and-add-advanced-packet-sup.patch

That will not help too much

ghost commented 2 years ago

Describe the feature

image

Technical implementation

Why don't we remove usage of getProtocolForPacket and set it to every packet once and then just take it? getProtocolForPacket and setProtocol are more expensive than sending actual packet/passing it to netty.

Other

No response

Agreements

  • [x] You have confirmed that this feature does not yet exist in the latest version of NachoSpigot.
  • [x] You have confirmed that there aren’t any issues open regarding this feature.

If you can test that something like ProtocolLib or minecraft itself doesn't crash and works correctly, fine 😳

andreasdc commented 2 years ago

I don't use PLIb, as you can see getProtocolForPacket is on top of packet sending, which is not acceptable.

ghost commented 2 years ago

I don't use PLIb, as you can see getProtocolForPacket is on top of packet sending, which is not acceptable.

You're saying that we should remove the usage of getProtocolForPacket without considering if it would break plugins, that's what im talking

andreasdc commented 2 years ago

I don't use PLIb, as you can see getProtocolForPacket is on top of packet sending, which is not acceptable.

You're saying that we should remove the usage of getProtocolForPacket without considering if it would break plugins, that's what im talking

Do whatever you want with some weird plugins, I'm reporting big performance leak in this method, it can be done without having any hashmap as I believe.

ghost commented 2 years ago

I don't use PLIb, as you can see getProtocolForPacket is on top of packet sending, which is not acceptable.

You're saying that we should remove the usage of getProtocolForPacket without considering if it would break plugins, that's what im talking

Do whatever you want with some weird plugins, I'm reporting big performance leak in this method, it can be done without having any hashmap as I believe.

Then the changes that you proposed aren't acceptable. Nacho focus on performance improvements, new api and all these things but not on breaking plugins

andreasdc commented 2 years ago

s

I don't know how can this break plugins, but if you want to keep slow methods for weird plugins do it, it will be the best to make this change to have better performance.

Sculas commented 2 years ago

Some code was left in there for compatibility with plugins such as ProtocolLib. I could definitely make an EnumProtocol interface, then have one implementing the current one and a completely new one from scratch. But IMO, that's a time waste, since pretty much everyone uses ProtocolLib. I'm not going to spend hours or days for a couple of people to use it.

I'm going to close this for now, but I might look into it later on.