How bad of an idea is this?

I'm working on adding redstone control of spawners to my creaturebox plugin, and I'm concerned about the performance impact that one section of code may have on servers.

Code:

  @Override public void onChunkLoaded(ChunkLoadEvent inEvent)
  {
    Chunk theChunk = inEvent.getChunk();

    for (int theX = 0; theX <= 15; theX++)
      for (int theY = 0; theY <= 127; theY++)
        for (int theZ = 0; theZ <= 15; theZ++)
        {
          Block theBlock = theChunk.getBlock(theX, theY, theZ);
          if (theBlock.getType() == Material.MOB_SPAWNER)
            plugin.updateDisabledSpawner(theBlock);
        }

  }

Basically, each time a chunk is loaded, I scan it for spawners, and register those that should be disabled.

My question is, should I be concerned about the time required to scan a chunk, given that I'm not modifying any blocks?

 

If you have to ask, usually pretty bad.

that's only 16*128*16 iterations, or 32,768 iterations, which isn't TOO bad. You can do millions of iterations, so long as the stuff inside each loop is not time consuming.

But, this is not of those cases. As I mentioned in this thread (was doing a similar thing), you're getBlock calls are EXTREMELY expensive. They will fill up (and probably crash your server's) cache. You're going to need to switch to getBlockTypeId to avoid filling Bukkit's automatic caching of block calls.

In a survival server where there isn't a *ton* of movement or teleporting, I'd say it wouldn't cause too much lag if you followed my suggestions. In a creative server with lots of players or lots of teleportation, it'd be a lag-fest no matter what.

I'd find a different way to do what you want.

 

I guess I'll switch to plan B then - it'll make onCreatureSpawn less efficient by scanning a 9x9x3 array of blocks around the creature each time one is spawned, but at least the spawners that are supposed to be disabled will be.

I also considered storing persistent state across server restarts, but that method seems likely to become corrupted, and cause strange bugs.

 

A loop that small is will not cause any lag at all. Sounds like the right solution.

 

Thank you Afforess, I knew the other solution didn't feel right, although it would have been elegant if I hadn't had to add that fuggly OnChunkLoaded code.

 

You are running that loop in O(1) time, however for your purposes, it would be incredibly "slow".

 

no, the loop is O(n^3), but besides the point the original example could lag the server, the modified find where a Mob spawn and find a spawner wouldn't be too bad

though if we had onChunkGenerated this could all go away because we could disable it then, or modify the original example to keep track of which chunks have been checked and only check the unchecked ones

 

O(n) isn't inherently slow - what's more important is how long 'n' is.

 

My apologies it is O(1) because it is a constant. Regardless it would still have a slow run-time.