Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1.12.2 - Requesting input from maintainer for future laser relay reverse-lookup optimization PR #1397

Open
ByThePowerOfScience opened this issue Apr 6, 2024 · 1 comment

Comments

@ByThePowerOfScience
Copy link

ByThePowerOfScience commented Apr 6, 2024

I'll probably be the one to implement anything that comes from this, so don't worry about extra effort coming from replying to this. I just want to get the input of someone who knows the codebase better than I do before making my PR.

Problem Location: LaserRelayConnectionsHandler#getNetworkFor

So the issue is that getting the connections for a given blockpos happens multiple times per tick on the server side and multiple times per frame on the client-side. Each time a laser needs to know its network, it has to iterate through the entire ConcurrentSet to find its own position, which easily takes up over 13% of client time in my base and ~1.5% server time on my server.

I'm thinking up a Mixin softpatch for it in the modpack I work on, but I wanted to raise the issue here to see if you could think of any better way to handle this.

The logic currently is something like this:

  • Networks themselves exist in a ConcurrentSet at mod.data.WorldData#laserRelayNetworks, which is saved as part of the World's NBT.
  • Whenever we need to render a laser or save its state, we have to iterate through the whole network set and check if it contains that BlockPos. For servers with a large number of laser networks, this is a very expensive process to repeat several hundred times per frame.

Possible strategies:

  • I want to use a reverse-map that relates the BlockPos to their networks, but I'm not sure if that's the best idea since it would be duplicated memory.
  • We could represent each network as a GRAPH (so we don't have to store each connection pair) and do world-rendering for each laser, thus only requiring one iteration of the set to render every single laser in the entire world, but that would require a complete teardown of the TESR-based laser system that's currently implemented. Plus that would only solve the rendering problem and not the saving problem.

I feel like the reverse-lookup map is the best idea, but I wonder if we could go even further and only store the networks in the reverse-lookup map? Idk, I'll probably just save the frames at the cost of memory in my softpatch, but for the official PR I want something more coherent.

Anyway, thank you for your time!

@Ellpeck
Copy link
Owner

Ellpeck commented Jul 14, 2024

yea the code sucks and is from when i was like 15 and had no idea about anything lmao, it could definitely use with some redesigning in terms of that. please note that 1.12.2 is not supported anymore and won't have any pull requests merged, though, so if anything, this is a 1.20.4+ update :)

using a bidirectional map (i think java has a builtin type for this, or at least one of the many libs that minecraft uses does) is the easiest solution, as using a graph would mean including a third-party library (like i do in pretty pipes, it's a PAIN to maintain) or writing a good algorithm yourself which, again, is pretty error-prone and probably takes some maintenance work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants