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

Expiremental Perf Code #202

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

Expiremental Perf Code #202

wants to merge 1 commit into from

Conversation

Mythra
Copy link

@Mythra Mythra commented Dec 14, 2018

This commit includes two performance fixes that have been beneficial on our server. However, these changes on one server may not be the experience for everyone (and thus be beneficial to merge into upstream). We hope by opening this PR we can get some more people to test locally, and (maybe even on their server)!

The two changes we provide in this (besides a pom.xml change for ProtocolLib so it can build locally on a fresh box where the dependencies aren't cached) are:

  1. Using Fisher-Yates Shuffle, over Collections.shuffle. The reason for using fisher-yates instead of Collections.shuffle were described originally when I was working on the original 13.1 PR, but I'll copy it here for those who don't want to go find it or didn't follow it):

Previously we were calling out to: Collections.shuffle(Arrays.asList(...)). The problem with this is we construct an object Arrays.asList that we throw away anyway in the next GC Cycle (since it's temporary), and then we call: Collections.shuffle (which besides allocating a random object which is also GC'd next run) is with list size. If our list is over 5 (which it generally is) copies the list into an array, and performs an O(n) shuffle, then copies the array back into the list. Aka we're doing this copy/object creation for really no benefit. So I switched us to a Fisher-Yates shuffle. Which is also O(n) time like Collections.shuffle however avoids all the unnecessary copying.

All in all it means one less place we allocate objects that just get GC'd on the next GC run, while still keeping the O(N) nature of Collections.shuffle (so we're not taking a perf hit in time, just helping GC run faster!)

  1. Using xxHash over crc32. This is the bigger change (and probably the more controversial one seeing as how it brings in an extra dependency), but can certainly help when reading/writing to the cache.

Although CRC-32 is relatively fast when compared to something like MD5, it still isn't really what we're looking for. Which is a non cryptographic high throughput hash. For here we turn to xxHash (specifically xxHash64), which is considered one of the fastest hashes available without sacrificing quality. This statement is specifically coming from multiple SMHasher (the de-facto hash tester these days) results. 1 2 3 4.

Obviously though what everyone else tests with won't map to anyone. Really what we wanna see is if it makes the experience better for a noticeable amount of users. Cause then it might be worth merging it in (or at the very least making it an option).

@ProgrammerDan
Copy link
Collaborator

Good to see another PR from you, friend!

Again, absolutely no opposition from me on the shuffle change. Looks good. Hash seems fine too; I haven't checked out the lib yet, but surface level looks fine. I've flagged Aleksey -- I know he's been very, very busy lately with decreasing time availability but hopefully he'll have a chance to give this a peak.

@Mythra
Copy link
Author

Mythra commented Jan 6, 2019

We recently promoted this to our production server instead of testing (using 1.13.2), and our performance seems to be doing better. It's only been a couple hours, but we'll let you know if anything happens after running this with actual players who aren't testers for a few days.

@ProgrammerDan
Copy link
Collaborator

Thank you so much again!

@ProgrammerDan
Copy link
Collaborator

I'm hoping some of this widespread crashing is now resolved, so will look to merge in this next :)

@NgLoader
Copy link

NgLoader commented Jan 2, 2020

Applied here 5.0.0 Release

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.

3 participants