Mass Block Editing

[DarkArc]

This is a sampling of the current situation with WorldEdit running inside of Sponge:

I just started Warmroast and did an edit with ~110k blocks to get this sampling.

Block Workers

I've heard that we should be using BlockVolumeWorkers? However, correct me if I'm wrong, but their implementation seems to just be a wrapper around void setBlock(int x, int y, int z, BlockState block);

I've also heard some arguments for speeding the edit up by creating BlockStates off the main thread. Personally, I think that's pointless. We're constructing the BlockState inside of what is identified as com.sk89q.worldedit.sponge.SpongeWorld.setBlock() and as you can see, there is a hit of .4%, which is incredibly low -- assuming the only penalty there was the creation of the BlockState.

Additionally, please keep in mind that mod authors do not currently have to abide by any multi-threading standards, so they are free to do whatever they like when constructing a block state. We cannot make an guarantees about the retrieval of a BlockState in a multi threaded environment, and we shouldn't try to.

Unavoidable Hits

There's an unavoidable hit in the net.minecraft.world.World.markAndNotifyNeighbors() method. There's really not a way to work around having this hit, it's just what Minecraft needs to do to make blocks function properly.

Avoidable Hits

Sponge CauseTracking

As it stands, we're taking a HUGE hit from cause tracking. We need a way to completely avoid the main event bus, and CauseTracker implementation, because it's just going to murder servers. Perhaps some of this is improved in @gabizou's refactor -- I'm not sure?

Lighting (Needs fixed)

Minecraft isn't made to have large amounts of blocks change after the initial chunk creation. In fact, base chunk generation operates on top of a ChunkPrimer object so that it doesn't have to incur the penalties of lighting considerations, and physics updates.

We're currently hitting a "normal" block change method, and assuming that it will clean up our mess; long story short, it doesn't. It's costing CPU time, and it's not working, WorldEdit leaves behind many "dark patches" and light sources which did not properly update the light map around them.

As a note on the previous paragraph, Mojang uses this function for structure generation, but that's during the population phase which also includes lighting population, and even then, these structures are much smaller than what many WorldEdit users would like to create.

We need a better way to change a fairly large number of blocks, and then inform Minecraft that we've invalidated the lighting. One which is performed at the end of an edit so as to prevent lighting updates from performing duplicate and unnecessary work.

This could also allow WorldEdit's "fast mode" to actually be fast. Currently, despite not triggering physics updates, it is forced to trigger the incorrect lighting updates, because there is no way to disable this functionality via method arguments.

[Zidane]

@DarkArc

Are you using the setBlock methods on Extent that take a Cause object? I'll speak with blood and we'll accommodate a way to do lighting only after block changes are done throughout the tick. This has the potential to be a pretty crazy performance boost.

[DarkArc]

Yes this is with the setBlock method that takes a Cause.

This is where our abstraction hits Sponge: https://github.com/sk89q/WorldEdit/blob/feature/sponge-new/worldedit-sponge/src/main/java/com/sk89q/worldedit/sponge/SpongeWorld.java#L141

---- On Sat, 14 May 2016 09:08:37 -0700 notifications@github.com wrote ----

@DarkArc

Are you using the setBlock methods on Extent that take a Cause object? I'll speak with blood and we'll accommodate a way to do lighting only after block changes are done throughout the tick. This has the potential to be a pretty crazy performance boost.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub

[Zidane]

@DarkArc

So talking with @bloodmc some, basically this will require some "plumbing" to WE to get it as performant as possible but basically:

• User applies schematic/Executes some batch command
• Calculate all coordinates that will be affected
• Generate the Block Snapshots via a worker thread.
• Run restore on those snapshots back on the main thread

[Zidane]

Further remarks:

• A restore triggers no ChangeBlockEvent OR Capture. For WE's case, you will DEFINITELY want to do this.

[DarkArc]

I don't think Snapshots are the best route here.

The implementation for Snapshots is basically just setBlock, except the CauseTracker is told that blocks are being restored: https://github.com/SpongePowered/SpongeCommon/blob/master/src/main/java/org/spongepowered/common/block/SpongeBlockSnapshot.java#L166-L196

All we're going to save from doing the generation on a separate thread is... well nothing. In fact, it's going to cost more overall CPU time building up this SpongeBlockSnapshot object, which is immediately discarded. Then there's the cost of creating a new thread, and related synchronization, or buffering mechanics. Additionally, as I said previously, there are concerns with doing this async as well because there is no guarantee that creating the block state is a thread-safe operation.

It's already been shown that generating the BlockState is relatively cheap, it's the overhead of cause tracking, and lighting causing problems.

If we could get a similar method setBlockSilently() for instance, which performed the same operation as the restore method, that would be preferable. The restore method could then call world.setBlockSilenty() thus removing the incurred duplicated code. Snapshots are great for when you need to restore back to a particular point in time, but I see no gain using them, when we already have the information we need built up.

I read IRC chat a bit, and if you need all of these changes grouped, that can be arranged. We can add a flush method to WorldEdit with minimal invasion, and have that get called at the end of our operation to pass all the blocks along for that SpongeWorld instance.

However, with the numbers, I can't get behind the idea that creating BlockStates async will be worth while. Honestly, I think the best route is just to bypass events, and tracking with a setBlockSilently or betterNamedMethod method and see where that leaves us, because if for no other reason, the memory -- and corresponding GC -- overhead of grouping (or creating lots of BlockSnapshots) will be significant.

Note: Another additional method may be needed for changing the tile entity data, if that's being tracked somehow, though this is significantly less of a problem as most edits are composed of what I would call "primitive" blocks.

[bloodmc]

BlockState's are definitely thread safe as they get generated when the game starts so there is no "re-creation" of a BlockState.

As for BlockSnapshot's, this is what we use for most block events and are what most plugins expect to see in any event that changed a block. The only reason why I said to use restore is because that is the only way you are going to bypass tracking and events currently. Now lets say we introduced a new "ChangeBlockEvent.Pre" event that could be handled in one of the following ways

Option 1 - Not friendly to rollback plugins. (Note: this does not support TE data)

    public boolean setBlockBulk(Map<BlockState, Vector3i> bulkBlockList, boolean notifyNeighbors, boolean checkLight, Cause cause) {
        checkArgument(cause != null, "Cause cannot be null!");
        checkArgument(cause.root() instanceof PluginContainer, "PluginContainer must be at the ROOT of a cause!");

        final CauseTracker causeTracker = this.getCauseTracker();
        causeTracker.setPluginCause(cause);
        ChangeBlockEvent.Pre event = SpongeEventFactory.createChangeBlockEventPre(cause, bulkBlockList);
        SpongeImpl.postEvent(event);
        if (event.isCancelled()) {
            causeTracker.setPluginCause(null);
            return false;
        } else {
             // some flag to disable ChangeBlockEvents
             for (Map.Entry<BlockState, Vector3i> mapEntry : bulkBlockList.entrySet()) {
                 BlockPos pos = VecHelper.toBlockPos(mapEntry.getValue());
                 checkBlockBounds(pos.getX(), pos.getY(), pos.getZ());
                 setBlockState(pos, (IBlockState) mapEntry.getKey(), notifyNeighbors ? 3 : 2);
             }
             causeTracker.setPluginCause(null);
             return true;
        }
    }

Option 2 - supports rollback plugins and TE data

    public boolean setBlockBulk(List<Transaction<BlockSnapshot>> blockTransactions, boolean notifyNeighbors, boolean checkLight, Cause cause) {
        checkArgument(cause != null, "Cause cannot be null!");
        checkArgument(cause.root() instanceof PluginContainer, "PluginContainer must be at the ROOT of a cause!");

        final CauseTracker causeTracker = this.getCauseTracker();
        ChangeBlockEvent.Pre event = SpongeEventFactory.createChangeBlockEventPre(cause, blockTransactions);
        SpongeImpl.postEvent(event);
        if (event.isCancelled()) {
            return false;
        } else {
             // some flag to disable ChangeBlockEvents
             causeTracker.setPluginCause(cause);
             for (Transaction<BlockSnapshot> blockTransaction : blockTransactions) {
                 BlockSnapshot blockSnapshot = blockTransaction.getFinal();
                 BlockPos pos = VecHelper.toBlockPos(blockSnapshot.getPosition());
                 checkBlockBounds(pos.getX(), pos.getY(), pos.getZ());
                 setBlockState(pos, (IBlockState) blockSnapshot.getState(), notifyNeighbors ? 3 : 2);
             }
             causeTracker.setPluginCause(null);
             return true;
        }
    }

With option 1, we would only be passing the BlockState and position via the 'ChangeBlockEvent.Pre' event of what would be the final result if the event wasn't cancelled. Plugins such as Prism would be forced to generate a BlockSnapshot for every one of those positions during the event so they could rollback if required. This goes back to my original reasoning of why BlockSnapshot is extremely important as it was designed for rollbacks. As an example, a player may accidentally use WE to set 500,000 blocks not realizing that they wiped out an entire town in the process. Going with option 1, you would have to revert to a backup or have built-in restore support which would require taking a snapshot to begin with.

With option 2, you give us a list of block transactions which would contain the original state of the block position as well as what you want it to become. We would then pass this list of transactions via the bulk event and a plugin like Prism could simply take the original list and serialize it for rollback purposes(avoiding the need to generate a BlockSnapshot) and thus saving on resources.

Providing a "silent" block change method goes against our event philosophy as we believe every plugin and server administrator should have the right to know what changes are going to happen BEFORE they happen. In the end, no matter what way you look at it, I think Option 2 is the best for most cases as many server administrators will want that "rollback" capability.

[DarkArc]

Threading

I don't see what you're talking about in reference to block states.

This function is defined by the Block class, can be overridden, and is called when getting a block state via meta, which is what we currently do.

    public IBlockState getStateFromMeta(int meta)
    {
        return this.getDefaultState();
    }

That said even if you get a BlockState with the IBlockState method:

    <T extends Comparable<T>, V extends T> IBlockState withProperty(IProperty<T> property, V value);

There is no guarantee that this withProperty is going to be a thread-safe method, and no guarantee that moded blocks are going to use the two default implementations whether it be BlockState, or ExtendedBlockState.

tldr; I don't feel that there is a strong enough contract here, or a great enough need, to utilize another thread for BlockState/BlockSnapshot construction.

BlockSnapshots

As for rollback, well, we do have built in "rollback" support via the undo command. That said, I do see the merit of having the TileEntity changes -- for logging plugins -- accompany the data, so I will work towards using BlockSnapshots.

Events

I both understand and respect your reasoning here, but I would at least advise against putting this on ChangeBlockEvent.Pre. That will trigger any ChangeBlockEvent listener, and while they may be expecting large numbers of blocks, most will not be expecting what WorldEdit can/will throw at them. A separate MassChangeBlockEvent may be more appropriate, that way there's no risk of someone accidentally opting in.

I think it's fair to say, many administrators are happy with WorldEdit and similar being an admin-only tool, and thus would be okay with edits coming across it not following the standard behaviour, especially if it means their servers run faster.

[DarkArc]

I made the change to BlockSnapshots (minus TileEntity data being applied w/ the snapshot restore): https://github.com/sk89q/WorldEdit/commit/3188bc303195a3b1228604918537ef67f4489eee

Here's what our updated sampling picture looks like:

As you can see while the restore function cut the utilization in half, the Sponge builder registry has a slow spot, and brought our utilization to about what it was before.

This is actually a relatively easy fix -- from both sides. The cause of this slow down is the argument checking looking for the CanonicalName of the builderClass. This name calculation gets ran even when the request for a builder hasn't failed.

public <T extends ResettableBuilder<?, ? super T>> T createBuilder(Class<T> builderClass) {
        checkNotNull(builderClass, "Builder class was null!");
        checkArgument(this.builderSupplierMap.containsKey(builderClass), "Could not find a Supplier for the provided class: " + builderClass.getCanonicalName());
        return (T) this.builderSupplierMap.get(builderClass).get();
    }

I fixed it on the WorldEdit side (though I think there's a fair case to redesign that validation) in https://github.com/sk89q/WorldEdit/commit/8c927c7eab4925660c87bc176c57b9b0e0bc1101 by resetting an existing builder, which brought our sampling to the following:

Which removes Sponge itself from being the bottleneck (minus neighbour notify events having an effect).

[Zidane]

@DarkArc

When do you think you would have WE up and working on Sponge? We're fully ready to keep making changes on our end to help you.

[DarkArc]

@Zidane it depends on what you mean by up and working. We have builds now that support Sponge Vanilla and Sponge Forge, with WorldEdit CUI and chunk regeneration being the only missing features. http://builds.enginehub.org/job/worldedit?branch=feature%2Fsponge-new

The performance changes I've been making are going into a branch of what will likely become WorldEdit 7. That's still a ways off, but that's when we should be able to drop the need to go to NMS, and have an updated type system.

I'll probably back port some of my performance changes, like using BlockSnapshots to the sponge-new branch/builds soon.

[gravityfox]

Here's my two cents.

DarkArc mentioned that MutableBlockWorkers are currently just a wrapper, and don't offer a performance boost.

Would it be reasonable to take many of these performance optimizations and make them available to BlockWorkers? As far as I'm aware, the performance issue is only with very large numbers of block changes, so changing how setBlock works is probably not exactly needed. In addition, sponge would be able to process the change as a bulk operation, potentially reducing the overhead with the cause tracker. Optimizations to lighting and block notification could be thrown in too.

If MutableBlockWorkers are supposed to be performant for bulk operations, I say we should make it so.