MineMaarten / Signals

Minecraft mod that adds OpenTTD style signaling and more!
GNU General Public License v3.0
17 stars 14 forks source link

Signals 1.3.0-6 doesn't play well with worldgenerated rails #63

Closed dashkal closed 6 years ago

dashkal commented 6 years ago

Mod interaction issue. I have lost cities set to sphere mode with monorails, but I have the rails broken (1 in 10 rail units is air instead). Signals appeared to be aggressively seeking out all of these "networks". Performance basically dies.

MineMaarten commented 6 years ago

Hmm yes they try to acquire the network once, and after that they shouldn't, anymore.

Just worldgen with rails (and no Signals blocks) should not trigger this network building, however, the first block update involving a rail will trigger it. Also any Signals block placed down next to a rail triggers it.

There are a few options I currently think of:

  1. Defer loading for unloaded chunks. This is tricky to do, and when you're actually using the network kinda annoying, as you need to actively visit all chunks of a large network to make it function, and it is not too clear when it doesn't function.
  2. Only load the network when Signals actually is involved and needs it. There's no use in maintaining a network if there are no Signals. However, just this alone won't prevent a lag spike when placing down a first Signal.
  3. Load the network in batches. It is possible to schedule only let's say 50 rails in a tick, and do that repeatedly until the network is formed.

I like option 3, and 2 makes sense, and probably is doable. I'll try to get a build out with option 3 today.

MineMaarten commented 6 years ago

Try this build! http://jenkins.k-4u.nl/view/MineMaarten/job/Signals/9/ This includes option 3 as described above. Option 2 and more perf improvements are to follow (I already started on a few of those before you opened this issue :))

dashkal commented 6 years ago

Much appreciated. I'll load that into my test instance this evening (PDT).

autumnfire commented 6 years ago

Running the Enigmatica 2 Expert modpack which has the build posted above and my temporary base is over top of an abandoned mineshaft. The server handles it with no issue but when I or my partner connect to the server our CPU usage jumps to roughly 100% and stays there for at least 10 minutes before dropping down to a normal 4% on an i7 8700. After running visualvm against Minecraft, the methods using the most processor time are:

net.minecraft.client.renderer.chunk.ChunkRenderDispatcher.func_178511_d() - I expect this one com.google.common.collect.RegularImmutableSet.contains() com.minemaarten.signals.rail.network.RailSection.isAdjacent() com.google.common.collect.ImmutableList.listIterator() com.minemaarten.signals.rail.network.RailNetworkClient.lambda$calculateAdjacentSections$0()

The last four move out of the top once the 10 minutes is up. I even tried copying the server's world folder to single player and it still does it. I even disabled the rail network in the config, which I thought would disable the mod and it continues to run the same functions.

dashkal16 commented 6 years ago

Sorry, have not forgotten. Other concerns took over and I haven't had time to run the test yet. Hoping to this weekend. Maybe...

MineMaarten commented 6 years ago

@autumnfire : Thanks for the feedback! I have been testing myself with some bigger networks with VisualVM to analyse bottlenecks, and as it happens, I think I resolved your case already in this commit:

https://github.com/MineMaarten/Signals/commit/bdc2a786aee10e9ae5842c09b95d2b0bf5067251#diff-0cc1a8d0e137a247cbb3353c7fff63a4

It might not be as 'minor' as the commit mentions after all ;). This wasn't included in the build you used, you can use http://jenkins.k-4u.nl/view/MineMaarten/job/Signals/11/

It does explain why it takes 10 minutes, basically what happens is that the server syncs the rail network to the client, but it does so in steps to not create too big network packets. However, client-side, it used to try to build the rail network for every received update, and immediately discard it, because a new packet would arrive to do additional updates. This creates a long queue that could've taken 10 minutes to process.

Now it should only build the network client-side when the queue has emtpied, and network is required (to prepare the renderers), which should not take 10 minutes.

The good part of all this is that it shouldn't affect tick/render time as it's all in a worker thread :).

Let me know if you find other problems!

autumnfire commented 6 years ago

Tested build 11 on both clients that had the issue and can confirm it does fix the problem. I'll let the pack devs know so they can look for the update if/when it's pushed to curseforge. Thanks again!

MineMaarten commented 6 years ago

Should be on curse now or shortly, uploaded it an hour ago :)

Because of this internal rewrite and different approach, these versions were marked beta, the fact that these problems occured were expected, fyi :). But I'm glad you guys are beta testing this and giving feedback!

dashkal commented 6 years ago

Running with Signals-1.12.2-1.3.2-11-universal.jar and I can confirm that the issue appears resolved.

gchpaco commented 6 years ago

MineMaarten: incidentally an excellent test for this is Lost Cities subways, which are vast and extensive.

gchpaco commented 6 years ago

I am running Signals-1.12.2-1.3.2-11-universal.jar and have this problem with Lost Cities worldgen (which worldgens large subway rail networks); the manifestation for me is usually that I see "Signals is updating the rail network" and then essentially the server almost freezes until it is done. This happens more than once a world, although not commonly. Generally speaking it seems to happen when I cross over from one subway network to another one. Because of the way Lost Cities subways work, the network can be very large although always extremely simple.

MineMaarten commented 6 years ago

Hmm thanks for the tip, I might try that, but I can imagine the outcome you have described. This just confirms that I should actually add option 2 as mentioned above, so that only networks get loaded when Signals actually needs it. This will introduce the same problem when someone adds a signal to a lost city metro network, but I guess that's inevitable and the one time penalty is ok to be taken.

It might also be a problem that Signals actually will chunk load all connecting rails temporarily to build the network, which, when involved with worldgen, likely causes many chunks to generate to find the whole metro network. Deferring loading this is something I wouldn't like, because it could cause confusion to users because they are not sure which parts of their rail network is loaded or not.

gchpaco commented 6 years ago

The chunk loading is almost certainly what is actually going on, and it would be acceptable to me to have Option 2, as by and large the Lost Cities nets are not suitable for automation without a lot of work (no branches, etc). And because there are usually two or three lines in any given direction that branch out and other miseries, you can end up needing to load the entire rail network which is of unknown, indeed unknowable a priori size.

MineMaarten commented 6 years ago

Option 2 has been implemented, see https://minecraft.curseforge.com/projects/signals/files/2578319