Large move operations have slowed down - blame the API?

[planetguy32]

As it stands, massive structures can take quite some time to move. I suspect this may be because we allocate and post around 10 new event objects per position to move. Each handler also has to check if the block it's dealing with is an instanceof the right type. Off the top of my head I can think of these options:

• Stay the course: This would leave large moves still slow, but wouldn't break API.
• Cache event objects: This would save us the GC pressure, but there's a chance it could break API users that store our event objects after their handlers exit.
• Make an interface for blocks to implement, use (Mixin)[https://github.com/SpongePowered/Mixin] to apply our patches: This would get rid of both the allocations and the instanceof checks, but compatibility with the current API would be nigh impossible.

I'm leaning towards the second option myself since instanceof() should be fast on modern JVMs and it saves both us and API users a bunch of work, but I think the third might merit further investigation.

[da3dsoul]

I would prefer whatever is the most efficient in the long run. I don't know how many developers actually use our API. If anyone knows any, post them here and we can contact the developers to see their input.

[planetguy32]

@pixlepix and @Lordmau5 have asked about the API. The former (I'd guess) uses IMCs to blacklist things, and the latter probably uses events since IMCs weren't expressive enough for their use case. I'm not sure who else might be using this.

The second solution is probably safe in terms of breaking API and can be fairly easily implemented. The third would take some work and would break all APIs, so let's get some more comments before we start it.

[da3dsoul]

It would be nice to multi-thread some of the pre-movement stuff, and better yet the post movement stuff. Unfortunately, most of the lag in the post-movement phase is due to tile-entity re-init, so not much can be done about it. The pre-init can be done a little but only read-only phases to avoid problems.

[Lordmau5]

Dropping in. I would rewrite according to your changes - wouldn't be a biggie :) I have noticed huge lag as well when e.g. moving a 13x13x13 FFS tank, so ye... :P

//EDIT: Multi-threading doesn't sound bad. Minecraft doesn't do that at all (iirc), so you are pretty safe to use that and get stuff done faster.

[planetguy32]

I looked into implementing multithreaded world access, but accessing a block in not-yet-generated areas calls world generation to make the chunk, and tile entities live in a global list. There are probably more such things, but anyway it adds up to mean that block access often trigger global side effects that we'd have to synchronize. Almost any method we call on chunks assumes it's running in the main thread, so we'd have to use our own copies of almost all of the code in Chunk and World. That said, if someone did manage to make multithreaded world access we could probably use it to implement parallel block access in RemIM quite easily.

After dbcc83ed6fcf4dee01737cc1010f8672779a12f9, there subjectively does seem to be a speed-up. I'll keep looking for more overhead.

[da3dsoul]

I had the lighting code there for two reasons. Cosmetics is obviously one. The other is that those lighting calculations are done when the carriage stops moving. If the area is already lit, it takes some of the overhead off of the finish moving phase and moves it to the placeholder phase.

[planetguy32]

We might be better off to spread lighting updates across multiple ticks somehow, but if that isn't actually a big perf hit feel free to re-enable it.

[da3dsoul]

The main issue's making sure that the world around it doesn't change while it's thinking. It can take up to a few seconds as things are to start to move a large structure, which should be faster. Even after multithreading, not everyone has a nice 8-core cpu like myself, so if takes 0.75 seconds to start moving, there is more than enough time for a player to mess with the blocks.

[da3dsoul]

Something that would speed it up immensely would be to multithread the evaluation of the package (can it move), the adding process, and the actual block saving. We would need to implement a protection system that disallows the blocks from changing in any way, removes them from update lists, and then they are added back automatically when they are replaced.

[da3dsoul]

The protection system would need to be on main thread to ensure it works, though.

[da3dsoul]

One thing we can do is make the mods request which events it needs at init, and then we only post events if something requested it. IMC could handle that. We just have so many events that most don't need.

[planetguy32]

Protecting against block changes on the main thread could be as simple as blocking its execution, and I've hacked together thread-safe getBlock() code in the library. We probably want plugins to run on the main thread, since splitting them off would be a major API break, but other than that multithreading should be fairly easy to achieve in read-only parts of the move sequence (probably most of the first stages, in fact).

I don't think events are a huge slowdown, come to think of it. Most of the ones we have are only used by a handful of plugins, and Hotspot might be able to optimize them away since the ASM event bus goes through regular bytecode.

Though come to think of it, performance problems on my end likely have much to do with the fact that my computer is literally garbage.

[da3dsoul]

I've gotten a decent profiler (VisualVM in case you are curious https://visualvm.java.net/download.html; it works on linux, too). So I've discovered that what takes the most amount of time is the updateRendering stuff in Utils. Updating lighting takes a lot of time as well. Optimizing rather than just moving the lag around would be nice, but I'm not sure what can even be optimized there.

[planetguy32]

Yeah, I'd believe that. It looks like for each call, vanilla (and probably sneaky) setBlock has to touch up to a 32x32x32 area, so I'd imagine that would be some of the problem. Having looked through the setBlock() call tree a bunch, it looks like Mojang does some stuff to profile it, too, suggesting it's a major CPU usage.

We could skip lighting updates for really big moves, or spread them over many ticks. In the latter case, we could cancel the pending lighting updates if it moves again. I'd be a bit worried about allocating a bunch of position objects though, lest garbage collection get flooded.