How should I make huge block change without locking main thread

Discussion in 'Plugin Development' started by jamesst20, Dec 11, 2013.

Thread Status:
Not open for further replies.
  1. Offline

    jamesst20

    Hi everyone!

    I do know we must not make huge task in the main thread because blocking it would causes bad behaviours, etc..

    I am writing a command for my plugin to make the world flat for a specific radius. Making the map flat for a radius over 70 would freeze the server for few seconds which is really bad. I am aware I could divide the task and add them into few separate delayed task, but would it be the best way?

    Here would be a bad version of the code that works.

    Code:java
    1. try {
    2. Player p = (Player) cs;
    3. int radius = Integer.parseInt(args[0]);
    4.  
    5. for (int x = (int)p.getLocation().getX() - radius; x < p.getLocation().getX() + radius; x++) {
    6. for (int z = (int)p.getLocation().getZ() - radius; z < p.getLocation().getZ() + radius; z++) {
    7. for (int y = (int)p.getLocation().getY(); y < p.getLocation().getY() + radius; y++) {
    8. p.getWorld().getBlockAt(x, y, z).setType(Material.AIR);
    9. }
    10. }
    11. }
    12. } catch (NumberFormatException e) {
    13. Methods.sendPlayerMessage(cs, ChatColor.RED + "This is not a valid radius.");
    14. }


    Thanks!
     
  2. Offline

    zeeveener

    If you plan on performing a big task, look at WorldEdit's source. The probably make heavy use of Threads/BukkitRunnables.

    Make sure you understand the concept of threading though because you could cause more problems if you don't handle Concurrency properly.
     
  3. Offline

    NathanWolf

    I recently went down this same road, and for what it's worth I decided to do just what you described. I broke the work up into smallish chunks and scheduled a task to do the work until it's done, every tick.

    I think I may need to slow it down because eventually it seems to overwhelm the server with chunk updates.. I'm not sure how to get around that.

    I considered (and posted some questions about) trying to offload the work into a separate thread, but ended up deciding against it. I think the amount of work that the internal code will have to do when you actually change the blocks will far outweigh your own code. And you can't put that part (the actual block update) in another thread, so.. probably not worth it.

    I ended up making an object to track a task like this- it records its current progress through an x,y,z range, and does a fixed number of block updates at a time (1,000 seems ok-ish). I have a queue of these objects, and a repeating task that checks for objects in the queue and steps through them until they're finished, or it reaches the block-updates-per-tick-limit.

    If you want to look at my code, feel free- but it's honestly not very compartmentalized at all.. I started down that road originally, but ended up wanting to tie into my history/undo, material and messaging systems. Anyway:

    An example task, which can construct a sphere, pyramid or cubiod:

    https://github.com/elBukkit/MagicPl...kkit/plugins/magic/blocks/ConstructBatch.java

    And here is where I set up the timer:

    https://github.com/elBukkit/MagicPl...rs/mine/bukkit/plugins/magic/Spells.java#L426

    You could probably adapt this if you wanted to, or just take the idea and do it your own way. :)
     
    Jbody, jamesst20 and The_Doctor_123 like this.
  4. Offline

    The_Doctor_123

    NathanWolf
    I agree, this a good way.

    zeeveener
    World Edit is all ran on the main thread, that's why the server just stops when you modify large areas. You can't edit the world in another Thread. They were stupid, in my opinion, because there's no splitting of the tasks, resulting in the inability to modify very large areas.
     
  5. Offline

    zeeveener

    The_Doctor_123

    Ah well. I thought they used threads like what NathanWolf suggested.
     
  6. Offline

    The_Doctor_123

    zeeveener
    Not separate Threads, repeating tasks.
     
  7. Offline

    zeeveener

    The_Doctor_123

    Does each BukkitTask not run on it's own Thread? If not, then Async is where it's at. Otherwise, the task will stop the main thread either way.
     
  8. Offline

    The_Doctor_123

    zeeveener
    You can do either. It depends which method you call. If you call the sync one, the task will run on the main thread. If you call the async method, it will run on another Thread.
     
  9. Offline

    NathanWolf


    Yikes, no- I said specifically to avoid threads and asynchronous tasks. It's not going to buy you anything unless you're doing some crazy calculations to determine just a few blocks to set or something. For cleaning a whole area, you're only offloading the work of running a loop, which is pretty much nothing.

    As the Doctor mentioned, you can do either. However you really can't interact with the Bukkit API at all from an async task, so those are only for very specific work you can offload.

    The task will only "stop" the main thread (perceptively) if you ask it to do something it can't complete in a reasonable amount of time. Hence the need to break it up.

    Also... this is purely an opinion, but I think it looks really cool to see structures forming over time :) My building algorithm is currently semi-optimized to try to batch up chunk changes, but I'd almost consider re-working it just for a cooler effect...

    Anyhow, if you're curious- I haven't gone too nuts with this, but here are some examples of 128 and 64 block radius spheres I generated using this technique:

    [​IMG]
    This one is building-in-progress, it fills in from the center out


    [​IMG]
    Eco-dome!
     
    AlexMl and Jbody like this.
  10. Offline

    zeeveener

    NathanWolf

    I have a tendency of confusing terms when I speak. What I meant was Task, what I typed was Thread.
    Async methods CAN call BukkitAPI so long as you understand concurrency and how it works. Obviously there are certain things which can never be called from async, but it's not the worst thing one could do.
    However, you are correct in stating there are no real performance gains. It would stop the server from freezing though.
     
    NathanWolf likes this.
  11. Offline

    The_Doctor_123

    zeeveener
    Actually, it's probably less efficient overall to have it split up.
     
  12. Offline

    Wingzzz

    You could cause issues if you call non-thread safe code (ie: block changes). A good idea would be to create some tasks that will split up the work load over time in which it will not greatly effect the server as suggested (this will vary per-machine).

    ie: A collection or array of locations that you can change to a certain block type in a repeating task, you can increase or decrease the speed as you see fit to accommodate for lag.
     
    NathanWolf likes this.
  13. Offline

    NathanWolf

    Wingzzz I love the idea of dynamically throttling based on load!

    One other note I wanted to come back and make, since I just fixed it in my own code- make sure the chunks are loaded before you attempt to make changes to blocks.

    I added a chunk.isLoaded() check, if it's not loaded I call load() and then exit (and wait for the next iteration to try again).

    This probably spams chunk.load() since I iterate every tick, but I'm hoping that's ok.

    Testing it out, it seems to work great- I'm able to make megastructures larger than my own view distance... and I can cast the spell, log off, and come back later to see my finished masterpiece :D

    [​IMG]

    These are two complete domes- 64 and 256 radii. The outer dome clips off at y, but is otherwise complete. I can't actually see from one end to the other at max view distance :)

    Sorry for all the screenshot spam, I have something of a passion for autogenerated megastructures (even if it's just an empty plain as the OP was asking about)

    EDIT: For a sense of scale: (I love dynmap!) http://mine.elmakers.com/?worldname=world&mapname=flat&zoom=3&x=-575.5&y=64&z=-80.99999999999997
     
    AlexMl likes this.
  14. Offline

    Wingzzz

    NathanWolf
    I don't think it should be that bad, Chunk.isLoaded(); really only ends up calling .contains() on a ChunkCoordPair of int x, int z (well, after it goes through a lot of things, WorldServer etc-).
     
    NathanWolf likes this.
  15. Offline

    NathanWolf

    Thanks! That's good to know- I probably should've been less lazy and inspected the source code myself.

    One other thing I just thought of that is probably specific to my code, I wonder if I'll have issues because I require 4 different chunks to be loaded at once (each iteration writes 8 blocks in a symmetrical pattern for efficiency). So far it hasn't been an issue, but maybe with a high server load where unoccupied chunks get quickly unloaded.. hrmm.
     
  16. Offline

    Wingzzz

    I see interesting, okay well how about you create a "lock" on the chunks in use so as to ensure they do not unload whilst you're manipulating the blocks in-case no one is around to keep it loaded.

    >> ChunkUnloadEvent

    Side Note: Perhaps this discussion should be moved as I'm unsure of the benefit towards the OP considering this may have gone slightly (if not very) off-topic.
     
    NathanWolf likes this.
  17. Offline

    jamesst20

    I made this :

    Code:java
    1. try {
    2. final Player p = (Player) cs;
    3. final World world = p.getWorld();
    4. final int radius = Integer.parseInt(args[0]);
    5. final Location pLocation = p.getLocation();
    6.  
    7. taskID = Bukkit.getScheduler().runTaskTimer(JCMDEss.plugin, new Runnable() {
    8. private int x = (int) pLocation.getX() - radius;
    9. private int z = (int) pLocation.getZ() - radius;
    10. private int y = (int) pLocation.getY();
    11. @Override
    12. public void run() {
    13. int blocksChanged = 0;
    14. for (;x < pLocation.getX() + radius; x++) {
    15. for (; z < pLocation.getZ() + radius; z++) {
    16. for (; y < pLocation.getY() + radius; y++) {
    17.  
    18. if(blocksChanged >= maxBlocksPerTick){
    19. return;
    20. }
    21. Block b = new Location(world, x, y, z).getBlock();
    22. if(!b.getChunk().isLoaded()){
    23. b.getChunk().load();
    24. b = new Location(world, x, y, z).getBlock();
    25. }
    26. b.setType(Material.AIR);
    27. blocksChanged++;
    28. }
    29. y = (int) pLocation.getY();
    30. }
    31. z = (int) pLocation.getZ() - radius;
    32. }
    33. Methods.sendPlayerMessage(p, "Done.");
    34. Bukkit.getScheduler().cancelTask(taskID);
    35. }
    36. }, 0, 1).getTaskId();
    37.  
    38. } catch (NumberFormatException e) {
    39. Methods.sendPlayerMessage(cs, ChatColor.RED + "This is not a valid radius.");
    40. }


    It doesn't freeze anymore but I have tons of crash if I make it for a very large radius, why? I am checking if the chunk is loaded, can't see what else if wrong.
     
  18. Offline

    NathanWolf


    I would try sticking a "return" after "b.getChunk().load()", and get rid of the "b = new Location" after that.

    Chunk loads are asynchronous- so the block will not be immediately loaded after you call loadChunk. I handled this by just stopping and waiting for the next scheduled tick, your check will happen again and eventually the chunk will load.

    EDIT: Otherwise, this looks nice BTW! :D
     
  19. Offline

    jamesst20



    Thanks :) It still doesn't fix sadly : net.minecraft.server.v1_7_R1.ReportedException: Exception while updating neighbours
     
  20. Offline

    ZeusAllMighty11

    With NMS you can make about a million block changes per second.
     
    jamesst20 likes this.
  21. Offline

    NathanWolf


    :( Sorry to hear that! What you're doing now looks so similar to what I'm doing... I'm not sure what could be wrong, and I've never seen that particular error message before.
     
    jamesst20 likes this.
  22. Offline

    jamesst20


    Yeah, I think I should import Craftbukkit instead of Bukkit and start using Minecraft API itself :S I know Bukkit team tries as much as they can to keep us using bukkit interface. Have you got any example on how I could do that?

    Edit : oh no :( That would mean I have to change the imports every update :/
     
  23. Offline

    ZeusAllMighty11

  24. Offline

    jamesst20

  25. Offline

    Wingzzz

    jamesst20
    Instead of a for-loop you could try using a repeating task and give it a reasonable interval between block changes. Not quite sure how big of a difference it may have- but it doesn't hurt to try.
     
  26. Offline

    jamesst20


    The NMS version is so so fast that it is not even worth dividing the task. The issue is that it still require the for loop to get blocks location and this is what is slow.

    Code:java
    1. try {
    2. final Player p = (Player) cs;
    3. final World world = p.getWorld();
    4. final int radius = Integer.parseInt(args[0]);
    5. final Location pLocation = p.getLocation();
    6. final MassBlockUpdate mbu = CraftMassBlockUpdate.createMassBlockUpdater(JCMDEss.plugin, world);
    7.  
    8. mbu.setRelightingStrategy(MassBlockUpdate.RelightingStrategy.NEVER);
    9.  
    10. Bukkit.getScheduler().runTask(JCMDEss.plugin, new Runnable() {
    11. private int x = (int) pLocation.getX() - radius;
    12. private int z = (int) pLocation.getZ() - radius;
    13. private int y = (int) pLocation.getY();
    14.  
    15. @Override
    16. public void run() {
    17. for (; x < pLocation.getX() + radius; x++) {
    18. for (; z < pLocation.getZ() + radius; z++) {
    19. for (; y < pLocation.getY() + radius; y++) {
    20. mbu.setBlock(x, y, z, 0);
    21. }
    22. y = (int) pLocation.getY();
    23. }
    24. z = (int) pLocation.getZ() - radius;
    25. }
    26. Methods.sendPlayerMessage(p, "Done.");
    27. mbu.notifyClients();
    28. }
    29. });
    30.  
    31. } catch (NumberFormatException e) {
    32. Methods.sendPlayerMessage(cs, ChatColor.RED + "This is not a valid radius.");
    33. }
     
  27. Offline

    desht

    jamesst20 the for loop overhead is unavoidable (after all, looping over multiple blocks is kind of central to this whole operation), but you could eliminate a few repeated method calls & calculations there...

    PHP:
    int x2 pLocation.getX() + radius;
    int y2 pLocation.getY() + radius;
    int z2 pLocation.getZ() + radius;
    for (
    int x pLocation.getX() - radius<= x2x++) {
      for (
    int y pLocation.getY() - radius<= y2y++) {
        for (
    int z pLocation.getZ() - radius<= z2z++) {
           
    mbu.setBlock(xyz0);
        }
      }

    A few notes:
    • Note I'm also using <= rather than < to get a symmetric area around the central point.
    • Bear in mind (assuming you're using my MassBlockUpdate code) that your plugin is now CraftBukkit-version-dependent - a new version will be needed each time CB's obfuscation changes.
    • A lot of the slowness comes from relighting calculations - that's the really expensive part of it all. The default behaviour for MassBlockUpdate is to do immediate relighting, which slows it down a fair bit (though it's still faster than plain Bukkit calls). You can can change this with mbu.setRelightingStrategy(), and pass RelightingStrategy.NEVER to avoid relighting calculations entirely (in which case lighting may look a little off until clients get close and do their own relighting - especially at night), or RelightingStrategy.DEFERRED, which does relighting over the next few ticks (but is quite heavy on memory since it needs to store a queue of blocks to relight).

    As an aside, I did submit a PR to get MassBlockUpdate into Bukkit, but it wasn't accepted (although the PR is still open, so not rejected outright) since it exposes too much implementation detail.

    https://github.com/Bukkit/Bukkit/pull/867

    As Wolvereness pointed out, a better approach might be some kind of schematic API; plugins could create a schematic in memory or loaded from file, and a Bukkit API call could allow for pasting the schematic into the world. The implementation for that could be pretty fast, although of course the Bukkit interface couldn't specify anything about that. Comphenix also had some pretty interesting ideas, but the discussion did sort of peter out - I never had a chance to really follow up on it.

    EDIT by Moderator: merged posts, please use the edit button instead of double posting.
     
    Last edited by a moderator: Jun 5, 2016
  28. Offline

    NathanWolf

    This seems like a no-brainer question (assuming NO), but just in case- is it ok to populate a mass block update in a secondary thread, but apply it in the main thread? Or is it actually making changes as you modify it, and just not notifying clients until the end?

    Anyway, thanks for this- it looks helpful for the issues where the server gets overloaded sending chunk updates, but it doesn't seem like this would really be any more efficient without clients connected- am I wrong? Is it more efficient at actually performing the block updates server-side? I would assume it just batches the client block/chunk updates... does it do more?

    So just to make sure I have this straight, if I were to use this I would create one in my offloaded (synchronous) task and populate it. When the populate action is complete, I'd call notify clients.

    So I'd lose my cool "building in progress" effect, but maybe gain some efficiency in the updates?

    EDIT: I would be curious to see some real performance comparison tests- has anyone done the legwork on that? I suppose I could...

    The reason I ask is that it seems like I can do "thousands" of block updates a second without the mass update code. I currently have my batched updates cap at 1,000 block updates per tick, and that seems ok... it'd be nice to know how big of a gain this would be before going down the "refactor to use NMS" road :)
     
  29. Offline

    jamesst20

    desht

    Thanks you for all of your tips, I am unsure to understand the part of a schematic, maybe because english isn't my native language and that word means nothing to me :p

    I could still split the loop to run in more ticks instead of one, that would not make anything faster but at least prevent freezing. Then I could kind of figure out some sort of progress.

    Using your NMS library, it is very very much faster, however that means more work every update :S Just wondering if it is really worth since my plugin has never broken over any update :S


    You could use my code
    Code:java
    1. final Player p = (Player) cs;
    2. final World world = p.getWorld();
    3. final int radius = Integer.parseInt(args[0]);
    4. final Location pLocation = p.getLocation();
    5. final MassBlockUpdate mbu = CraftMassBlockUpdate.createMassBlockUpdater(JCMDEss.plugin, world);
    6.  
    7. mbu.setRelightingStrategy(MassBlockUpdate.RelightingStrategy.NEVER);
    8.  
    9. Bukkit.getScheduler().runTask(JCMDEss.plugin, new Runnable() {
    10. private int x = (int) pLocation.getX() - radius;
    11. private int z = (int) pLocation.getZ() - radius;
    12. private int y = (int) pLocation.getY();
    13.  
    14. @Override
    15. public void run() {
    16. for (; x < pLocation.getX() + radius; x++) {
    17. for (; z < pLocation.getZ() + radius; z++) {
    18. for (; y < pLocation.getY() + radius; y++) {
    19. mbu.setBlock(x, y, z, 0);
    20. }
    21. y = (int) pLocation.getY();
    22. }
    23. z = (int) pLocation.getZ() - radius;
    24. }
    25. Methods.sendPlayerMessage(p, "Done.");
    26. mbu.notifyClients();
    27. }
    28. });


    then compare when a player is offline with bukkit API vs this and by getCurrentTimeMillis() you could calculate which is more efficient :)

    My guess is that this NMS way would just be as fast as if no player would be online because even thought it is modifying blocks, the player isn't notified until the notifyClients() has been call.

    EDIT by Moderator: merged posts, please use the edit button instead of double posting.
     
    Last edited by a moderator: Jun 5, 2016
  30. Offline

    desht

    No, don't do that :) This is modifying chunk data directly - doing it from another thread is just as dangerous as doing it via Bukkit calls.

    It does two main things:
    • Batches up chunk updates and just sends one chunk update packet for each chunk affected, instead of a block update packet every time a block is changed (to each player nearby - so yes, if there are no players nearby it shouldn't make much difference).
    • Optionally bypasses lighting recalculation (see my comments in my last update) - this is done regardless of who's connected, so will make a big difference.
    Right, although there's nothing to stop you combining the two approaches.


    A performance testing plugin and some figures here, although the figures are rather outdated, the methodology is still valid: https://github.com/desht/buster (note: I haven't built buster in quite a while - not sure if it still compiles, but it should do..)
     
    Wingzzz and NathanWolf like this.
Thread Status:
Not open for further replies.

Share This Page