1. Guest, as per the stickied thread, this forum has not been in use since 2014. All bugs and feature requests should be posted to JIRA.

Feature (performance) Optimized Player Lookup

Discussion in 'Bugs & Feature Requests' started by Aikar, Aug 6, 2013.

  1. I have wrote another PR that provides a decent performance improvement for large servers.

    However Bukkit has not pulled it yet so just going to push to Spigot:
    https://github.com/Bukkit/CraftBukkit/pull/1220

    Vanilla Minecraft and the Bukkit API use iteration on an arraylist to do all player lookups by name.

    This is horribly ineffecient for larger servers and since these methods are used in LOTS of places for simple vanilla actions, simply having 300 players online is a big hit to many portions of code.

    This patch makes it so player lookups are done by a map instead of iteration for exact lookups, and optimizes loose lookups to try for exact first.

    This will offer much more consistent scale with larger player counts, and should provide a noticeable difference on large servers.

    Benchmarks for 300 players show that using a map instead of iteration is up to 35x faster than the default implementation.

    Many plugins will be calling these methods as part of their code, so results will really vary by server, but there should be no negative impact from this change.

    Patch is running in production on my server without issue.
     
    • Like Like x 1
    • Winner Winner x 1
    • Friendly Friendly x 1
  2. md_5

    Administrator Developer

    As I said on IRC, I don't like the use of CHM, its unnecessarily heavy for the implementation we need.
     
  3. joehot200

    Supporter

    Please explain more? Surely less lag is less lag, and is good for us!
     
  4. md_5

    Administrator Developer

    He has used concurrent hash map as opposed to a more efficient regular hash map.
     
    • Informative Informative x 1
  5. I posted benchmarks in the PR. CHM vs HM does not provide any real difference in performance. Maybe 10 nanosecond difference. The JVM optimizes this for us...

    Also, Spigot has done a good bit of work to make things "safer" on async usage, so why not this too?

    There's plenty of stuff in Spigot that spends more nanoseconds extra than this difference, and its still leagues better than the original implementation.

    If you really really would prefer non thread safe activity on plugins (which before this patch, they would of at least been partially thread safe with the CopyOnWriteArrayList), then change it to HashMap I guess, I don't have naughty plugins, but I still think it should be a CHM for ensuring that plugins are not retrieving invalid results (for example, as a normal HM, getPlayerExact() could return a player object for a player who is offline if done async...)
     
  6. I'm voting for CHM, it still faster, but also safe.
     
  7. I honestly think using the CHM is a safer and more efficient alternative to HM. Safe thread-able access to the player list would be a nice addition. And doesn't CHM allow multiple reads as oppose to HM that is one read at a time? If so, wouldn't that be knocking out 2 birds with one stone?

    And if it is a huge difference between CHM and HM, do a setup testing with the two. Start up the VPS and put one instance, test with the standard plugins, and then test with the other (C)HM, under the same circumstances.
     
  8. md_5

    Administrator Developer

    You seem to be missing the fact it's not going to be any safer.
     
  9. Regardless of which one is preferred, if it is going to increase performance regardless, CHM or HM, implement it. The community will like it.

    Edit: Random, but shouldn't this be in developer ramblings?
     
    #9 hcherndon, Aug 8, 2013
    Last edited: Aug 8, 2013
  10. I don't have access to post there.

    Anyways, its changed to plain HM.
     
    • Like Like x 1
  11. So now calling isOnline() and getOnlinePlayers() will return strange results if called async?
     
  12. This has no bearing on getOnlinePlayers.

    isOnline, I'm not 100% sure of the behavior before, its likely the same behavior as before this patch, and your talking extremely fringe case here.
     
  13. Sounds good.

    Shevchik, just do some extra checks if you are scared, shouldn't affect it too much.
     
  14. md_5

    Administrator Developer

    It's not going to be weird if you call it async
     
  15. wut?

    Anywho, when might this be implemented?
     
  16. md_5

    Administrator Developer

    The odds of a player leaving, the exact microsecond that you query, are very small.
    Its in a branch pending testing.
     
  17. Oh ok. I see what you are saying.

    I will make it happen. xD
     
  18. Absolution

    Benefactor

    Any news on this?