Problem/Bug Common mistakes thread in plugin developmemt extremely outdated

@RcExtract
As I explained, the timing of your tests is not even remotely accurate. You say yourself that the execution times of your tests varies by the magnitude of the result itself, which should immediately raise a flag to say that they are not accurate!

Just to settle this, I have done some testing (not on a Bukkit server, because the execution time is so long it would crash the server, but I have done my best to replicate the conditions). After about 5 billion executions, the difference in execution time per execution time is roughly equal to five nanoseconds. To make such a fuss about a five nanosecond difference is almost comic.

Let's say that to have any noticeable effect on the server, something would need to cause a millisecond of delay (according to my testing you would need A LOT more, but let's just call it a millisecond so we don't have to argue). For a difference of 1 millisecond to be caused by five nanoseconds delay, you would need to have 200 000 method calls in a single tick. There are two reasons why this will never ever happen:

1. A Minecraft server will at the very most have a couple thousand players online (and even that is stretching it), so a couple thousand is the upper, upper limit for the number of player actions you need to process in a single tick.
2. That all online players would do something in a single tick is astronomically unlikely, if not impossible. Even if tried to coordinate everyone to do something at the same time, the actions would be spread out over a couple ticks, because of human reflex times and network delays etc. etc. Let's say that the absolute maximum of player actions you could need to process in a single tick would be a thousand. This is nowhere near enough to cause any sort of difference to performance at all.

Also "If you have more people on the server, Bukkit.getPlayer will become more expensive, but weak reference won't." is a total lie. Using WeakReferences/WeakHashMaps/whatever else you come up with will increase just as much as Bukkit.getPlayer(UUID) with the number of players, IE not at all.

This is because the complexity of a get in a HashMap is O(1) for our purposes, meaning that the lookup time stays constant no matter how many players you have. So the difference per call will never (within a rounding error) be greater than the originally measured 5 nanoseconds.

Now we have discussed why using WeakHashMaps for players is pointless, let us now discuss why it is bad. When I say that it is "complex" to do, I mean that knowing how it all works (with the garbage collector and when it does and doesn't reclaim memory) is quite complex and is not something you should have to worry about for such a simple task as keeping track of a Player, especially since the effects of getting it wrong are disastrous (mind you, even the best of programmers make mistakes).

Being a good programmer is also not just about making software that runs fast, it's about choosing competent and sustainable solutions to problems, even if it means sacrificing some performance (and in this case you aren't even sacrificing any significant performance at all). Just because you can save some meaningless performance does not mean you should. After all, having all your code in one method is faster than creating methods, so why not just write all of your plugin's code in one single method? Or even better, why not write your plugin in Assembly? Then you would sure as hell save some performance, right?

There has to be some rime or reason to how far you go to improve the performance of your program, and doing something which requires a quite advanced knowledge of how the JVM operates just to accomplish a simple task with very bad consequences if you get it wrong and especially instructing newcomers to do the same (meaning someone will inevitably get it wrong) all in the name of a performance benefit with doesn't matter anyway is what I'd call very unwise.

 

If multiple players, use Collection.newSetFromMap(new WeakHashMap<Player, Boolean>()).
Bukkit.getPlayer indeed will cause lag if it is called frequently, or there is a lot of player.
WeakReference.get() to get the player.

Storing Player with Weak Reference

In Bukkit, we are always told to store UUIDs but not Player instances. Actually I prefer a better way is to weakly reference the Player instances. It is simple.

To weakly reference to a Player instance:

Code:java

Player player; //Your player instance, strong reference
WeakReference<Player> weakReference = new WeakReference<Player>(player); //weak reference

Thats it.

To obtain the player instance:

Code:java

Player p = weakReference.get();

But note that it could be null, even the value passed into is not.

What is a Weak Reference?

Lets talk about strong reference first.

Code:java

Object object = new Object();

The above is an example of strong reference. The variable object is strongly referencing the object instantiated. This reference will stay forever, until another object is assigned to the object variable, which then the reference of the object variable will change to another object. For example:

Code:java

object = "Another reference";

The variable object will then change to strongly reference "Another reference" instead of the object we created before. The strong reference from the variable object to the object we created before is manually destroyed.

However, if a variable is weakly referencing an object, the garbage collector will still finalize and garbage collect the object if there are no strong references towards the object.

Code:java

Object object = new Object();
WeakReference<Object> weakReference = new WeakReference<Object>(object);
System.out.println(weakReference.get() == null); //prints false
object = null; //Removes the only strong reference to the object created.
System.out.println(weakReference.get() == null); //prints true

This is useful for preventing memory leaks when referencing Player objects. When a player becomes offline, its corresponding Player object should be finalized and garbage collected. If we weakly reference Player object, it still can be finalized and garbage collected, when Bukkit removes their reference to the Player object.

I want to store multiple Player objects

Java API does not come with WeakHashSet, but we can construct it from a WeakHashMap using Collections class:

Code:java

Set<Player> weakHashSet = Collections.newSetFromMap(new WeakHashMap<Player, Boolean>());

Note that the Boolean cannot be changed.

The content above is a tutorial. Please only use it for teaching.

Sorry for saying that more players cause more lag, but indeed Bukkit.getPlayer is expensive, while WeakReference is absolutely not. Its just a weak reference, but Bukkit.getPlayer is a searching.

Well i cant think of any situation that using weak reference can go wrong, unless you want to prevent a Player object from being finalized or garbage collected, but thats rare. Also, Failure is the mother of success. If programmers use weak reference wrongly, their users will tell them errors, then programmers ask here, and someone teach them what to correct.

Also, knowing what weak reference is is enough, just like u wont read JVM code, if possible, for your java program optimization. Also new Java programmers may not even know how Bukkit.getPlayer works, they only know how to use it. Weak reference is extremely more advanced than Bukkit.getPlayer, just probably u are new to it.

Assembly is completely unrelated. We are talking about Bukkit.getPlayer vs weak reference, or actually, Common Mistakes thread outdated.

Here are some posts i found form spigot. CONSIDER IT ON YOUR OWN.

https://www.spigotmc.org/threads/storing-references-in-a-custom-player.69823/#post-771369

https://www.spigotmc.org/threads/storing-references-in-a-custom-player.69823/#post-771407

 

@RcExtract
Let's not try to teach each other things in an ironic manner. I know just as well as you do what WeakReferences are and how to use them.

Did you not read my previous post? Bukkit.getPlayer() is not more expensive. Look-up in a Map (which is what Bukkit.getPlayer() is) is of constant time complexity, meaning that it takes the same amount of time no matter how many entries there are. I also showed that the time difference of Bukkit.getPlayer() is less than 10 ns (IE barely measurable under the best of circumstances, and certainly not long enough to remotely matter).

Bukkit.getPlayer(String) is somewhat slow (and used to be even slower), meaning that your argument could have made sense if we used player names to identify players - but the UUID method is just a lookup in a map, which is not slow at all.

I'm not saying that using WeakReferences is necessarily bad or that it will cause any harm to the code. All I'm saying is that there is an imminent risk of writing new HashMap<>() instead of new WeakHashMap<>() and causing disaster. This mistake will not make itself apparent at compile-time and will cause a mystery to whatever poor soul has their server gobble up loads of memory which can't be reclaimed.

What I am refuting is the fact that Bukkit.getPlayer() would be slow under any stretch of the imagination. It's not. Using Bukkit.getPlayer() together with storing UUIDs is perfectly serviceable and is the recommended method. There is nothing to warrant a change of the instructions to start using WeakReferences, especially since there are no benefits whatsoever.

The only outdated part in the 'Common Mistakes' thread is the bit about Java versions. It should be mentioned that what you wrote is inaccurate as Minecraft 1.12 and higher requires Java 8, so no servers with 1.12+ are running anything lower than Java 8. The whole rule about 'latest java version minus two' only applies to the current situation and it most likely won't stay that way (before 1.12 it was latest java version minus 4, so this is not a general rule).

I think that any eventual additions to the thread should be discussed and other people should be able to change the phrasing to make it clear what is meant.

(Also, the part I wrote about Assembly was to emphasize that just because you can do something marginally faster does not mean you should)

 

But Bukkit.getPlayer(uuid) is slower than weak reference, practically and theoretically. Also, u said it will not spend enough time to remotely matter. But what if Bukkit.getPlayer(uuid) is called multiple times? If u have a Player instance, u never need to call that method.

 

@RcExtract
Well, in my previous posts I did a rough calculation, and you would need hundreds of thousands (probably closer to millions) of calls per tick before it even creates a noticeable difference. A five nanosecond difference is small enough that the performance can be considered the same.

I'll tell you what is slow (comparatively) though. EventHandlers. Instead of running around worrying about Bukkit.getPlayer(UUID), worry about minimizing the times your EventHandlers are executed. This will save you way more performance than monkeying around with WeakHashMaps.

This also makes the whole 'clearing Player instances in EventHandlers' pointless since those EventHandlers use up way more performance than you will ever save by not using Bukkit.getPlayer().

 

I know. but cant we just change from Bukkit.getPlayer(UUID) to weak reference? I dont think its hard. Programmers may also make use of weak reference in other places, when they come to learn java in Bukkit.org, since weak reference is Java API, while Bukkit is not. I suggest to update common mistakes, so lets change from referencing UUID to weak referencing Player at the same time.

Also, u said it is disastrous to have WeakHashMap written to HashMap. But this kind of typo will never happen, also if programmers use HashMap wrongly, its their problem. When their plugins causes error or serious lag, they will come to Bukkit and ask for help, and we will say they used HashMap wrongly.

 

@RcExtract
Well, no it's not hard to start using Weak References, but there is no reason to. Changing the recommended practice just for the hell of it will only cause confusion and errors.

And how could you possibly know that such a typo will never happen? If you're using an IDE, chances are that the auto-complete functionality could cause just such a mistake.

The mentality of 'it's their problem' is not very nice towards the people you're trying to help. If you have to choose between two identical (performance-wise) approaches to a problem, but one of them is established and proven to work and makes serious mistakes less likely, why would you choose the other?

 

If it causes confusion just explain. Please give some example cuz i cant think of any, and the javadocs are very clear. There ar a ton of tutorials online.

It depends. Auto complete can also cause mistakes for Bukkit.getPlayer.

Well i just want to change Bukkit.getPlayer to weak reference while updating the common mistakes. Is there a problem? If we dont try how do we know whether the community agrees or disagrees weak reference, since its not a popular solution in bukkit.

I have read through some threads about storing player objects. Not a lot of people talked about weak references, but nearly no one disagrees with it. So i think its time to let more people know about it.

Last reminder, java has backward compatibility. You can build on any version u want and can be run on any higher version of Java, but not recommended.

 

@RcExtract
Well, what I mean would be confusing is that what you're effectively proposing is "There is this nice method which everyone is used to and works nicely. Let's change it for no reason at all!"

Auto-complete can (maybe?) cause issues with Bukkit.getPlayer(), but those issues will be easily spotted because the plugin probably won't compile, or if it does, you'll notice it right away. The issue I described with replacing 'WeakHashMap' with 'HashMap' won't be apparent and has disastrous consequences to the performance of the server.

I don't disagree with you either - WeakHashMap is a decent alternative to using Bukkit.getPlayer(), but there is no reason whatsoever to prefer it over Bukkit.getPlayer().

The bottom line is that if we have a method that works, why change it if the alternative isn't any better? I have laid out a few arguments to why you might not want to use it, but you have laid out no arguments for why one should, so there is no reason to change the standard practice.

 

Ur ignoring my arguments. i recently found posts saying that getPlayer is not only a map finding, it also consists of other things like array coping, though im not sure. Even its only map finding, its map finding. U dont do player map finding with weak reference cuz u already referenced the player. multiple Bukkit.getPlayer may cause lag, while weak reference is 100% lag safe.

 

@RcExtract
I'm assuming you are talking about the following post:

I'll have you notice two things about this post.

1. It is talking about Bukkit.getPlayer(String) not Bukkit.getPlayer(UUID), meaning that desht is not saying anything at all about Bukkit.getPlayer(UUID). I have already said that identifying the player by using their name is slow, but this has nothing to do with what we're talking about.
2. This post was written more than 5 years ago. At this point in time, there didn't exist a Bukkit.getPlayer(UUID) method, only a Bukkit.getPlayer(String) method.

This means that using this post to "prove" that Bukkit.getPlayer(UUID) is slow is totally ludicrous since desht wasn't even talking about that method.

Let us look at how Bukkit.getPlayer(UUID) actually works. Bukkit.getPlayer(UUID) calls CraftServer#getPlayer(UUID) (just a straight method call, nothing else). CraftServer#getPlayer(UUID) looks as follows:

Code:java

public Player getPlayer(UUID id) {
    EntityPlayer player = this.playerList.a(id);
    return player != null ? player.getBukkitEntity() : null;
}

This is in essence just a method call to PlayerList#a(UUID), which looks like this:

Code:java

private final Map<UUID, EntityPlayer> j = Maps.newHashMap();
 
public EntityPlayer a(UUID uuid) {
    return (EntityPlayer)this.j.get(uuid);
}

So, Bukkit.getPlayer(UUID) is essentially just a Map lookup. I'll also have you notice that this is the same method that Minecraft itself uses. Might be a compelling reason to believe me in saying that it's not inefficient?

I'm not ignoring your arguments, I'm just pointing out the (rather large) holes in them. Maybe it isn't wise to just take 5-year-old posts talking about a completely different method as gospel? Maybe you should instead look at the code for yourself to see what it actually does?

 

just list both options.

 

@RcExtract
As I have said, there is a risk of getting it wrong with quite bad consequences if you store players in WeakHashMaps.

Unless you have an actual compelling reason to store players in WeakHashMaps, I think it is unwise to instruct newcomers to do so because of the risk of getting it wrong.

 

Well u can claim any error can occur due to typo. Why stilll programming then?

Failure is the mother of success. Bukkit keeps reputation while teaching newcomers by hiding “risky” methods? Thats not the way of teaching.

 

@RcExtract
I have pointed out why that specific error is especially likely due to the auto-complete functionality in most IDEs. Additionally, this error will not make itself apparent to the developer in testing, but may still have nasty consequences for the end users.

Well, I have to disagree with you there. If there are two methods, and one decreases the likelihood of making serious mistakes, I think it's wise to teach that one.

No teacher in the world will agree with you that you should teach a worse approach to a problem just because "failure is the mother of success".

 

Worse approach? Its faster and i can think of no typo programmers can cause with weak reference. Just u scare of new things obviosuly cuz ur point does not make sense.

 

Let programmers learn weak reference (if they dont know) and possibly use in other places. While bukkit.getPlayer is only a method,and not included in Jdk.

 

U dont know if others need to use it. Weak reference exists for a reason. Not every places have methods like bukkit.getplayer to obtain the original object. Not every objects have identifiers. And weak reference is faster than bukkit.getplayer.

Edit: more people giving their opinions is appreciated.

 

We have discussed this for a week now. The difference in performance is negligible.

The usual reason for changing something is that you have a good argument. "Just u scare of new things obviosuly cuz ur point does not make sense" does not seem to me like a good argument refuting the points I've made, rather it seems to be a personal attack directed towards me.

What I do know is that people don't need WeakReferences for this purpose. If you so badly desire to teach people about WeakReferences, perhaps start by finding a problem where they are actually necessary.

 

Again. Weak reference is a good solution. Dont say “badly”.
I changed my mind not to remove Bukkit.getPlayer(UUID) back to a few posts. Cant we just add a new solution? Its a good solution.

I cant think of good arguments to respond to ur arguments which do not make sense.

 

@RcExtract
If you present a compelling reason why then, yes.

I have explained why there might be a risk in using WeakReferences. There's also the fact that serialization into configs becomes much easier if you're already storing UUIDs rather than having to convert from Player lists to UUID lists (which would require using Bukkit.getPlayer(UUID)).

By the way, the word "badly" in my previous post does not imply that WeakReferences are bad, it is an adverb to emphasize the verb "desire" (To badly desire means to want something very much).

 

Sorry for the misunderstanding.

You mean deserialization? So it depends, thats why we should provide two solutions, and possibly with examples for programmers to correctly choose one.

In Bukkit, you may use weak reference for Player objects. In other places...
I have a place in my plugin which uses weak reference but not for Player, but i choose to keep it secret.
https://stackoverflow.com/questions/7136620/weak-references-how-useful-are-they

 

@RcExtract
Well, it doesn't really depend on anything. Using Bukkit.getPlayer(UUID) has the following two advantages over using WeakHashMaps:

1. The risk of causing memory leaks is reduced dramatically.
2. If you'd like to serialize the list of players, all you have to do is save the UUIDs. With WeakHashMaps, you would have to convert a Player list into a UUID list, then save it and vice-versa for deserialization.

while WeakHashMaps don't have any advantages over Bukkit.getPlayer(UUID).

It is really not helping you convince us by saying "I have a use for WeakReferences in plugins, I just won't tell you".

 

I disagree with the first one. Both options are free of memory leaks.
Weak reference has following advantages:

1. Does not even need map findings, performance free
2. Can be used in other places, while Bukkit.getPlayer(UUID) cannot. Worth to learn.

Please stop only talking about WeakHashMap. its weak reference.

So list both options: weak reference and Bukkit.getPlayer(UUID)

Who knows if java programmers use them? But programmers not coding Bukkit plugins definitely cannot use Bukkit.getPlayer(UUID). Bukkit.getPlayer(UUID) is a method, different from 4 reference types which maybe a theory.

 

1. I've said this quite a few times now, Map look-up is not expensive.
2. This is totally irrelevant. Have you noticed that the name of this website is 'bukkit.org'? Maybe the content on it should be related to 'Bukkit' perhaps?

This forum has never claimed to teach Java, it teaches how to use the Bukkit API (which Bukkit.getPlayer(UUID) certainly is a part of).

By the way, whether it's supposed to be 'WeakReference' or 'WeakHashMap' depends on the use case, but 99 times out of a hundred you need to support multiple players (because players can do things independent of each other) so you will need to use a WeakHashMap. But either way, you understand what I am saying.

 

Bukkit is based on Java.

I just come up with an idea that a developer always want implementers to use their method instead of the Java API. Dont know if this fits who came up the idea of Bukkit.getPlayer(UUID) for resolving Player memory leak issue. Guava has its own Function functionalinterface, which is the same as the java.util.Function.