Minecraftforge: Player.dat File Get's Parsed Twice for one login

[jredfox]

version:1.12.2 - 14.23.4.2705

first here: FMLHandshakeServerState where it gets the dimension it parses the entire file without storing it
once here: PlayerList.initializeConnectionToPlayer()

This is unoptimized and causes lag on login. Please store the file once parsed once and then grab it later in PlayerList.initializeConnectionToPlayer()

FMLHandshakeServerState.accept() calls
int overrideDim = dispatcher.serverInitiateHandshake();

this calls PlayerList.getPlayerNBT() which parses without checks

Not much later before player has fully logged in
PlayerList.onInitializeConnection() calls
NBTTagCompound nbttagcompound = this.readPlayerDataFromFile(playerIn);

that method there parses the player file without checks count is 2

[diesieben07]

This is unoptimized and causes lag on login

Numbers or gtfo. Random "this is unoptimized" claims help nobody.

[Cloudhunter]

Given the largest player.dat will be smaller than most chunks I doubt it causes any kind of lag. Whilst it could be optimised for sure, please back up your claims.

[jredfox]

all I am saying is on big servers and inpatient people this is bad. Not too difficult to fix. You can confirm this by printlines

[TehNut]

The burden of proof is on you. You need to show that this actually causes a problem.

[jredfox]

did you look at the code I sent? FMLHandshakeServerState calls PlayerList.getPlayerNBT() just to grab the dimension later on PlayerList.onInitializeConnection() it calls this.readPlayerDataFromFile() which parses the file yet again thus parsing the file twice

[diesieben07]

Yes, which in itself is not a problem. If it bothers your OCD, that is not a Forge issue.

[jredfox]

files shouldn't be parsed multiple times on one entity. Not a huge issue but, needs to be fixed

[KingLemming]

But...why?

Believe me, I'm all for optimization - it's one of the big things I do. But in this particular instance, what is the ACTUAL overhead cost and what problem does it create?

[jredfox]

because it's unoptimized and wrong. It's like the world saving 2 times rather then one.

A bug is a bug not too hard to fix. I already made a patched method for PlayerList.class that fixes this on ASM I also fix several other issues relating to your uuid isn't what the server thinks it should be so It changes the gameprofile uuid to the servers cache. This prevents data wipes from occurring even though how very rare it might be.

[iTitus]

If you can actually reproduce the issue...

[covers1624]

You are still missing the point, provide some performance statistics, literelly everyone here has said it, yet you still fight the 'it seems wrong and it should be changed because i said so!' battle.

[jredfox]

@iTitus issue is reproducible every time especially if you read the code rather then just writing a comment. It gets parsed once in one class another slightly later

No need for performance statistics it's a bug. Shouldn't have to debate on why a bug should be fixed even if it's minor

[KnightMiner]

Its not a bug, its unoptimized code. So its up to you to say how unoptimized or make a PR that fixes the issue. What most people are saying is this is pretty minor and probably has little impact, so its probably not worth the time to fix. Prove that it is worth the time or use your own time to fix it.

[KingLemming]

Okay well @jredfox you should submit PRs then to fix the myriad of bugs and terrible code that Forge has allowed to linger. Don't simply throw up an issue and scream for it to be fixed, especially if you have apparently solved it with ASM in your own hacky patch.

Or as it's more commonly stated, "put up or shut up."

[covers1624]

I actually cant find the references you are referring to, some line numbers, or better yet, use github's snippets.

[jredfox]

wait I will post code snips to pastebin showing the design error of it parsing the files twice

[micahlaster]

confirmed

[jredfox]

FMLHandshakeServerState.accept() calls
int overrideDim = dispatcher.serverInitiateHandshake();

this calls PlayerList.getPlayerNBT() which parses without checks

Not much later before player has fully logged in
PlayerList.onInitializeConnection() calls
NBTTagCompound nbttagcompound = this.readPlayerDataFromFile(playerIn);

that method there parses the player file without checks count is 2

You can view your code yourself in your ide. I also pointed the classes and methods it is linked to in the initial report.

[covers1624]

That is a completely different code path from FMLHandshakeServerState than what you explained initially, well done. If its really bothering you, submit a PR.

[jredfox]

I must have stated something confusing derp

[covers1624]

So, ill lay out the codepath a little better. FMLHandshakeServerState.START#accept calls NetworkDispatcher#serverInitiateHandshake that sets up some state data and ultimately calls PlayerList#getPlayerNBT, does some checks, then returns the dimension id or 0.

PlayerList#initializeConnectionToPlayer /basically/ directly calls PlayerList#readPlayerDataFromFile. This is where i think the loading twice makes sense, readPlayerDataFromFile does some stuff we dont care about, but ultimately either uses the player data stored in the world data to asks the playerDataManger for the player data, but either way this fires an event, PlayerEvent.LoadFromFile, where as the earlier call to PlayerList#getPlayerNBT does not fire this event, likely due to it being wayy to early in the loading cycle for it.

So, yes the dual loading of the player data is a non optimized thing, but it would be extremely messy to read the player data once, store it /somewhere/ then wait for the second call to fire the event, No, just no. Not to mention that there are checks in both load paths for loading the player data from the world's nbt for the single player 'host'.

[jredfox]

Already done it here is my Player list class:
https://github.com/jredfox/evilnotchlib/blob/master/src/main/java/net/minecraft/server/management/PlayerList.java

I got that replacing methods from my modifications via ASM. The entity util temporarly stores the nbt until on login where it removes it after grabbing it.

My code also fixes several reading issues with level.dat when you give your world to another player as well as making the readPlayerDataFromFile or something like that using the getPlayerNBT()

[covers1624]

What? Why are you shipping a minecraft class? That's not how the game works outside your dev environment.. Regardless of your 'fix', the login code is messy enough, hacking something like this to cache a questionable amount of data is pointless. Who knows what repercussions this will have on top of the login code, furthermore how are you supposed to clear that map, wait for the player to logout? periodically? just opening up another memory leak if they aren't cleared.

[jredfox]

No It's compiled and it's the only way to replace a method easily is to use a compiled srg class then say Transformer.replaceMethod() and you send in the right input stream so it can find the method nodes. Otherwise you got to remove everything and manually via asm style code what you had before and change alot of code per mc version. The Decompiled class is for dev testing only. I also patch the uuid to what the server thinks it should be in case the client sends the wrong info.

The hashmap is cleared on server close and player login removes the entry that it stores so no memory leaks occur unless an exception on server side is thrown

[covers1624]

You seem to pick the first thing anyone has to say to reply to, Are you going to provide any performance statistics? Are you going to submit a pull request?

[LexManos]

This thread is nothing but spam and BS.
We will not support someone who uses ASM hacks because they think they need to and refuse to back their claims up with actual facts.
As for the actual 'issue' off the file being read twice, It doesn't matter! And part of the reason it IS read twice is so we keep compatibility/simplicity with vanilla. Not to mention the fact that it saves the datafixer and event, but no it couldn't possibly be that we wanted too read something without the extra overhead... The cost of maintenance vs the benefit of saving a few rare CPU cycles isn't there.
You have shown that you do not understand the system that is setup, and that you have no intention of actually helping the community but just complaining and making random claims.
Like KL said, Put up or shut up. Until then take your illegal 'EvilNotchLib' and sod right off.