Paper: Disable precreation of all possible BlockStates

[ninja-]

Basically, since 1.8 there is a terrible start time regression when these dumbb BlockStates were introduced.
Minecraft precreates a BlockState for each combination of metadata values.
The idea was good, it's nice that they are deduplicated in memory.
The problem is that some blocks have a LOT of possible BlockStates.
For example, Fire has around 4096 possible combinations etc. I don't remember which ones exactly.

It takes more than a second to do all this and the server is stuck at Loading libraries...
I made a patch for 1.8, deployed and forgot about this, it's extremely tricky and volatile, makes the BlockStates lazy-create when a new one is needed, this drops the load time from more than a second to 70ms.

(very easy to break this patch in many ways and then you see weird behaviour like: can't place redstone but you can place other blocks, when you break a double flower the upper part stays etc. I've made a few testcases to ensure the patch is working)

I've never ever seen it crash on my minigames, but it can crash with some BlockState not found error when creating a new world and going to a jungle or something so I don't know what's wrong.

Have anyone tried speeding this up before?
Is anyone interested in helping fixing up the patch and bringing it to 1.12?
I'll upload a proof of concept in that case.

[electronicboy]

loading libraries is part of the process before any of that occurs.

I don't think it's possible to even consider justifying such a patch, especially when it's likely to cause further breakage with other plugins. Paper is designed for general servers, if you need special modifications, you'd likely be better off forking and maintaining your own set of patches and maintaining them

[ninja-]

loading libraries is part of the process before any of that occurs.

No, I mean BlockStates are created after libraries are loaded and before first log message is printed.
I am 100% sure this patch can be modified to be perfectly safe as long as I could get some help with it.
It doesn't even cause any NMS API breakage.

[Black-Hole]

You should add some timings first. Calculating those block states won't take that long. Fire will take about 512 distinct block states: http://minecraft.gamepedia.com/Fire#Block_state

[ninja-]

I did the exact timings long ago it's clearly more than a second in total (registration time per block class).
As for fire, it's clearly 4096(3072 maybe), it's either that the wiki lies or it was changed since 1.8.

public static final BlockStateInteger AGE = BlockStateInteger.of("age", 0, 15);
public static final BlockStateBoolean FLIP = BlockStateBoolean.of("flip");
public static final BlockStateBoolean ALT = BlockStateBoolean.of("alt");
public static final BlockStateBoolean NORTH = BlockStateBoolean.of("north");
public static final BlockStateBoolean EAST = BlockStateBoolean.of("east");
public static final BlockStateBoolean SOUTH = BlockStateBoolean.of("south");
public static final BlockStateBoolean WEST = BlockStateBoolean.of("west");
public static final BlockStateInteger UPPER = BlockStateInteger.of("upper", 0, 2);

[Brokkonaut]

It was changed in 1.9:
public static final BlockStateInteger AGE = BlockStateInteger.of("age", 0, 15);
public static final BlockStateBoolean NORTH = BlockStateBoolean.of("north");
public static final BlockStateBoolean EAST = BlockStateBoolean.of("east");
public static final BlockStateBoolean SOUTH = BlockStateBoolean.of("south");
public static final BlockStateBoolean WEST = BlockStateBoolean.of("west");
public static final BlockStateBoolean UPPER = BlockStateBoolean.of("up");

I don't really like this idea - maybe its good for minigame servers, but most other server have an uptime of days to weeks until they are restarted. So startup time is not that much important, its more about maximizing performance when running.

[Deamon5550]

It would be better to improve the code doing the creation than to make wide sweeping changes of questionable stability. At the very least it looks like there is some immutable collection copies that can be avoided and from profiling seem to be the largest single contributor to the time taken.

[ninja-]

@Deamon5550 I tried to do this really hard for few days and failed miserably.

[ninja-]

@Deamon5550 could you share a screenshot of your profiling? I can't seem to catch Block registration with it. :)

[aikar]

I'm 100% against this. It's unnecessary to make micro optimizations to server start time and risk introducing bugs.

>>> date ; java -DIReallyKnowWhatIAmDoingISwear=1 -XX:+UseG1GC -jar ../../Paper-Server/target/paper-1.12.jar
Mon Jul 10 14:51:55 EDT 2017
Loading libraries, please wait...
[14:51:58 INFO]: Starting minecraft server version 1.12
[14:51:58 INFO]: Loading properties
[14:51:58 INFO]: Default game type: SURVIVAL
[14:51:58 INFO]: This server is running Paper version git-Paper-"0efa08cb" (MC: 1.12) (Implementing API version 1.12-R0.1-SNAPSHOT)
[14:51:58 INFO]: Debug logging is disabled
[14:51:58 INFO]: Server Ping Player Sample Count: 12
[14:51:58 INFO]: Using 4 threads for Netty based IO
[14:51:58 INFO]: Enabled sleeping between chunk saves, beware of memory issues
[14:51:58 INFO]: Generating keypair
[14:51:58 INFO]: Starting Minecraft server on 0.0.0.0:25569
[14:51:58 INFO]: Using epoll channel type
[14:51:59 INFO]: Set PluginClassLoader as parallel capable

3 seconds of startup is fine. Plugins and Worlds are going to be 3-10x that.

[ninja-]

@aikar no, 3 seconds of startup is everything but fine. it's not even about the TIME itself.
your assumption of "plugins and worlds are going to be 3-10x that" is also absurd
it's about the cpu being LAGGED TO DEATH when you start server instances in close time range(i.e. 1 second of delay between starts) on pure paper 1.12

my paper 1.8 servers starts in

[16:36:36 INFO]: Done (0.402s)! For help, type "help" or "?"

(+ around ~80ms for lazy block registry init)

This is enough to fully load the server, connect to the database, load the world and load the minigame, also loading libraries like ProtocolLib, ViaVersion and anticheat.

[polkovnikov-ph]

@aikar I don't know how your server is compared to @ninja- 's server performance-wise, but 6 times higher load time without any reason is clearly a regression.

[electronicboy]

It's been 3 years since the version he's running and the current version, ofc there are going to have been changes to the server software, such as adding additional data that needs to be stored and processed, that adds additional loading time. Loading and processing data takes time, I'd hardly consider that "without any reason".

The problem is is that as soon as you start making optimizations like what is proposed is that you start modifying how the server works and introduce potential breakages which become hard to trace down and fix properly. Plugins, such as anti-cheat plugins, expect stuff like block states to be precalculated and accessible.

if CPU resources are your concern, you should probably look into limiting what is allowed for the JVM instance to take, JVM startup is always going to to take a nice amount of CPU resources, however, I'm hardly seeing much CPU usage on startup here, probably peaking at 2x of what my computer overall is using during startup.

[ninja-]

The problem is is that as soon as you start making optimizations like what is proposed is that you start modifying how the server works and introduce potential breakages which become hard to trace down and fix properly. Plugins, such as anti-cheat plugins, expect stuff like block states to be precalculated and accessible.

Could you show any example plugin supporting this claim?
From my research, the plugins should be fine as long as the lazy-creation logic is put inside the set method of BlockState.
(that's more or less of what I did)

currentBlockState.set(BlockSomething.SOMETHING,_1234) -> new mutated immutable blockstate*

• careful not to create a second instance of the same BlockState or Minecraft will silently break
• need to create a base BlockState for each block type with empty data
• need to create blockstates for blocks that are being mapped with their data from "legacy id"
  (there's a piece of code in Block.java which does that, though even without optimizing it it's a big boost already)

[ninja-]

Here's the patch so everyone can have an idea of what we're talking about.
https://gist.github.com/ninja-/4109992fd1908d88414f8d5dcdbf6fbf

Oh, and by the way, I remembered that there are indeed cases where some NMS parts fetch list of all possible BlockStates for a given block. I usually saw that coming from mobs AI. I've got that covered, in such case all missing blockstates are created before returning the list. (this doesn't happen on init though)

[ninja-]

@Deamon5550

> It would be better to improve the code doing the creation than to make wide sweeping changes of questionable stability. At the very least it looks like there is some immutable collection copies that can be avoided and from profiling seem to be the largest single contributor to the time taken.

Unfortunately that's a bad track. Guava takes care not to copy the list in the case that's in BlockStateList and only wrap it. Even if not, I tried to remove that and it didn't help the start speed. It also required changeing type definitions all over the codebase because things are usually typed with ImmutableMap instead of Map for some reason.

[aikar]

I personally don't want to see this in Paper, feels like something more applicable for TacoSpigot. @Zbob750 ?

[kashike]

I don't want this either.

[ninja-]

:v

[zachbr]

From your own patch file:

Implement highly optimized BlockStateList to speedup server
start. Don't ask me how it works and why it works...

I understand your concerns, but this is a large change that, even in principle, is going to be completely unnecessary for the majority of our user base. It has the potential to introduce problems both obvious and hidden and I am also quite concerned about how maintainable this is going to be.

This change strikes me as something better suited to either a personal fork or one more focused on start/stop performance intended for minigames or large scale deployment. Paper is not going to be that fork, at least not at such cost.

It is also my opinion, relevant to your comment about CPU time being starved at startup, that you may also be well served by imposing CPU time and/or affinity limits on individual JVM instances at an OS level, whether that be Linux or Windows, as that would allow you to be more flexible in the software you run.

With all of the above being said, you clearly have a vested interest in seeing improved startup performance and should you come back in the future with a more refined and tested version, we will be happy to re-examine it and even potentially investigate adding it to Paper.

Thank you for your time, but I agree with the other core developers.

[ninja-]

@Zbob750 my intention here was not to get it merged as-is but to get some help to fix it first and make it more maintainable, as I know there is a bug somewhere but I am out of ideas.
"Don't ask me how it works or why it works" was more of an internal joke to my coworkers as I took the patch 1:1 from the repo :P sorry about confusion here

With all of the above being said, you clearly have a vested interest in seeing improved startup performance and should you come back in the future with a more refined and tested version, we will be happy to re-examine it and even potentially investigate adding it to Paper.

Well as I understand you're saying no paper developer wants to help improve the patch but if I fix it & clean it up myself I can come back for another review :/ ok then