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

Inefficient gameprofile lookups while using velocity's modern forwarding #10648

Closed
bridgelol opened this issue May 3, 2024 · 3 comments · Fixed by #10651
Closed

Inefficient gameprofile lookups while using velocity's modern forwarding #10648

bridgelol opened this issue May 3, 2024 · 3 comments · Fixed by #10651
Labels
scope: performance status: accepted Disputed bug is accepted as valid or Feature accepted as desired to be added. version: 1.20.6 Game version 1.20.6

Comments

@bridgelol
Copy link
Contributor

Timings or Profile link

N/A

Description of issue

Upon examining (after getting rate limited lol), I noticed paper nodes are making mojang api calls upon every player join which was interesting to me as velocity should forward all of this info.

Upon digging deeper I realized this is completely redundant.

ServerLoginPacketListenerImpl#callPlayerPreLoginEvents(GameProfile) gets called on all occasions, upon forwarding using velocity, or just generic standalone paper.

What's interesting is that the this call always gets executed even when it shouldn't need to.

image
image

This call ultimately results in either;
#1 The usercache needing to lookup this gameprofile again
#2 A mojang api call (this is the main issue, however saving some CPU usage in case #1 is also nice).

If you have 3 joint sub-servers on your velocity network that all run on the same public ip, it is pretty easy to get rate limited, as I discovered the hard way 👍

What I recommend (and I will be creating a PR for this, however I'd first like some feedback to confirm I'm not entirely delusional and am missing a use case for this call while using velocity):

If this call was made with the origin of velocity modern forwarding and the AsyncPlayerPreLoginEvent didn't modify the player profile, we no longer execute this call.

Plugin and Datapack List

not important

Server config files

Not important

Paper version

All

Other

As you can see here https://github.com/PaperMC/Velocity/blob/368d1a7c1250b4138f683e64ebb6aadfe7a4b1b3/proxy/src/main/java/com/velocitypowered/proxy/connection/PlayerDataForwarding.java#L73

All necessary gameprofile properties are being forwarded

@electronicboy
Copy link
Member

I think that the biggest question to me here is over the behavior of complete, if a profile is already completed, why is it doing lookups? I think that the class was generally designed in mind that the profile would be newly created from the API or server and just doesn't check that

@bridgelol
Copy link
Contributor Author

I think that the biggest question to me here is over the behavior of complete, if a profile is already completed, why is it doing lookups? I think that the class was generally designed in mind that the profile would be newly created from the API or server and just doesn't check that

Yup, I’ll push something in a minute

@bridgelol
Copy link
Contributor Author

#10651

@lynxplay lynxplay added status: accepted Disputed bug is accepted as valid or Feature accepted as desired to be added. version: 1.20.6 Game version 1.20.6 and removed status: needs triage labels May 5, 2024
lynxplay pushed a commit to bridgelol/Paper that referenced this issue May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: performance status: accepted Disputed bug is accepted as valid or Feature accepted as desired to be added. version: 1.20.6 Game version 1.20.6
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants