search

lishid / OpenInv

Open anyone's inventory as a chest, real-time!
GNU General Public License v3.0
120 stars 97 forks source link

Memory Leak #23

Closed aikar closed 9 years ago

aikar commented 9 years ago

I noticed that there are maps using Player objects here: https://github.com/lishid/OpenInv/blob/master/src/main/java/com/lishid/openinv/commands/OpenInvPluginCommand.java#L36 and https://github.com/lishid/OpenInv/blob/master/src/main/java/com/lishid/openinv/commands/OpenEnderPluginCommand.java#L36

Please switch this to UUID as using the Player object causes incredible memory leaks as it appears these values are not cleaned up on logout.

The memory leaks caused by Player objects are massive.

lishid commented 9 years ago

I certainly agree. When I wrote this code, it was about 4 years ago. Now... I am totally occupied and don't have a proper workspace setup... Anyone can send through a pull request?

ShadowRanger commented 9 years ago

lishid Yeah I can do that, ill see if I can get around to it today or tomorrow.

lishid commented 9 years ago

Cool!

ShadowRanger commented 9 years ago

I'm having a lot of trouble with the dependencies with IntelliJ. Might be worth using Maven with OpenInv maybe? By the looks of it you use it in Orebfuscator so I doubt you have anything against it. I can make it use Maven if you'd like.

ShadowRanger commented 9 years ago

Additionally, how would you like target players to be retrieved in the openinv commands? I could use a UUID fetcher which would be a good idea. And then just get the player via their UUID. I might need to change how the PlayerDataManager classes get the players though and make it use UUID's instead of names as well. Just let me know if you're alright with me doing that.

ShadowRanger commented 9 years ago

lishid, how exactly did you have your workspace set up when developing OpenInv. As in how did your IDE handle all the different nms versions. I'm really not sure how to get IntelliJ to compile the plugin without erroring out about the imports.

lishid commented 9 years ago

I just had every jar from previous versions somewhere... It's probably time to change that though, like I did for Orebfuscator in this commit: https://github.com/lishid/Orebfuscator/commit/80f94301b835e73efa0d0926f0b5d600e6a2069c

ShadowRanger commented 9 years ago

I will see if I can do that for you, like you did with Orebfuscator. I'll probably be able to do it today and I'll send in a pull request once done. If you like it, you can push it through I guess.

ShadowRanger commented 9 years ago

Okay I've almost finished with this. I basically did what you did in that update of Orebfuscator for 1.8. There's just one thing I'm stuck on which is pretty much all I have to do left. Basically it happens when you try to open the inventory of a player that hasn't played on the server before and it outputs an error saying something like 'Failed to load data for null'. Not sure if it's because of the change to UUID's instead of names.

aikar commented 9 years ago

As it should. should focus on fixing the issue mentioned here. OpenInv should not be creating player data files from scratch as that would open potential to some really weird bugs.

If they've never played, should not open their inventory.

On Mon, Jun 22, 2015 at 12:59 AM ShadowRanger notifications@github.com wrote:

Okay I've almost finished with this. I basically did what you did in that update of Orebfuscator for 1.8. There's just one thing I'm stuck on which is pretty much all I have to do left. Basically it happens when you try to open the inventory of a player that hasn't played on the server before and it outputs an error saying something like 'Failed to load data for null'. Not sure if it's because of the change to UUID's instead of names.

— Reply to this email directly or view it on GitHub https://github.com/lishid/OpenInv/issues/23#issuecomment-114006292.

ShadowRanger commented 9 years ago

I've already fixed this memory leak problem and have made it use UUID's instead. And yes I agree with your point that the data files shouldn't be created if they haven't played. I'll see what I can do.

aikar commented 9 years ago

this map is using the player issuing the commands object... looking up other players should have no relation to this change. it should be something like changing map.put(player) to be map.put(player.getUniqueId()) and etc for get(player.getUniqueId())

On Mon, Jun 22, 2015 at 3:20 AM ShadowRanger notifications@github.com wrote:

I've already fixed this memory leak problem and have made it use UUID's instead. And yes I agree with your point that the data files shouldn't be created if they haven't played. I'll see what I can do.

— Reply to this email directly or view it on GitHub https://github.com/lishid/OpenInv/issues/23#issuecomment-114026251.

ShadowRanger commented 9 years ago

Sorry, I didn't explain myself properly. Basically I've already done that so it isn't using player objects anymore however I haven't submitted a pull request yet so it's not on this repo yet. I'll be submitting a PR tonight.

ShadowRanger commented 9 years ago

Alright I submitted a pull request (Pull Request #25).