Skip to content
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

Use ByteBuf#copy only when entity rewrite is actually used #799

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BoomEaro
Copy link

@BoomEaro BoomEaro commented Apr 3, 2023

This pull request returns the ability to slice ByteBuf when decoding a packet in the MinecraftDecoder class as it did before, but only if entity rewrite was never used.
If I understand correctly, the original reason for using copy is that entity rewrite can write more bytes than is possible in the slice itself.
Therefore, most likely there is no point in constantly copying bytes even when we are not going to change them.

I would like to know your opinion on this, what it could possibly break and whether it makes any sense at all, since I have not had much experience with netty yet.

@electronicboy
Copy link
Member

This was long on the list of stuff that I wanted to look into, given that we're not rewriting this stuff, I did wanna look into adding a mechanism to literally just skip packets and let the proxy not care about them, even if registered

@Janmm14
Copy link
Contributor

Janmm14 commented Apr 3, 2023

There's no such thing as skipping packets, you always have to check packet id. And slicing instead of copying is already a big progress.
Not sure what else can be done here to optimize performance.

@electronicboy
Copy link
Member

I mean skip the decode + handle + encode chain for unneeded packets in certain configs; if we're not rewriting entity metadata, then we can skip a bunch of work

@Janmm14
Copy link
Contributor

Janmm14 commented Apr 3, 2023

I mean skip the decode + handle + encode chain for unneeded packets in certain configs; if we're not rewriting entity metadata, then we can skip a bunch of work

Bungee doesn't use decoding into object fields for most entity rewriting stuff.

@electronicboy
Copy link
Member

True, but part of it was for other stuff like scoreboards, etc; it's probably not a big deal, and probs not something I'm ever going to care to get to considering the state of this project

@Setter
private boolean supportsForge = false;
+ @Setter
+ private boolean slice = true; // Waterfall
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we default to the safe side, copy instead of slice?

Copy link
Author

Choose a reason for hiding this comment

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

Shouldn't we default to the safe side, copy instead of slice?

As far as I remember, the only situation where the proxy actually changes the buffer is when an entity map instance appears in the UserConnection.
Before the proxy creates the UserConnection, the proxy decodes the Handshake, Login, and a couple of other packets. (when the state is not yet FINISHING in InitialHandler)
It doesn't seem to make sense at this stage specifically to copy the buffer for these packets.

Copy link
Contributor

@Janmm14 Janmm14 Apr 8, 2023

Choose a reason for hiding this comment

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

btw why you don't set this directly at initialization of minecraftdecoder to the config value of rewriting disabled?

if we don't want to clone packets given the game phase, this should somewhere be explicitely mentioned - either in code or a simple comment

@bob7l
Copy link

bob7l commented Apr 21, 2023

This commit would likely break several plugins like ViaVersion that are expecting a grow-able, copied buffer. If anything, it should be made into a configurable option for the user to decide.

@Janmm14
Copy link
Contributor

Janmm14 commented Apr 22, 2023

This commit would likely break several plugins like ViaVersion that are expecting a grow-able, copied buffer. If anything, it should be made into a configurable option for the user to decide.

Statement from someone knowing viaverson-bungee is needed. Don't want some useless config option or startup flag.

@xism4
Copy link
Contributor

xism4 commented Jun 18, 2023

This would only theoretically occur if the option were disabled

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants