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

VoxelManip reuse leads to extremely large (billions of nodes) sizes #394

Open
cheapie opened this issue Jan 8, 2025 · 4 comments
Open
Labels
Bug Something isn't working Performance Aims to improve technical performance.

Comments

@cheapie
Copy link
Contributor

cheapie commented Jan 8, 2025

run_nodes() in technic/machines/network.lua reuses the same VoxelManip object for loading each machine node. If the nodes are very far apart (for example one at 1,1,1 and one at 2000,2000,2000) and connected by an extremely long cable, an attempt is made to load both positions into the same VoxelManip, causing it to grow to an absurd size, in this case 2000x2000x2000=8 billion nodes. Luanti crashes when this happens, which it probably shouldn't, but even if it doesn't this would consume a rather excessive amount of memory.

@OgelGames
Copy link
Contributor

I think this might be the cause of #350 🤔

@OgelGames OgelGames added Bug Something isn't working Performance Aims to improve technical performance. labels Jan 8, 2025
@S-S-X
Copy link
Member

S-S-X commented Jan 8, 2025

Nice find, I guess reuse was to allow a bit better performance. Probably would be best to test performance again to see if it is significant. If it is then probably see if we can keep vm for some defined max area, I guess like mapblock or few.

@cheapie
Copy link
Contributor Author

cheapie commented Jan 12, 2025

The engine has fixed the bug leading to the segfault in this commit, but all that really means to us here is that it will now properly show up as a mod error instead of just a segfault.

I haven't done any microbenchmarking of these and I'm not sure it even really matters, but should we perhaps be using core.load_area() for this instead anyway? It's used elsewhere in the same file already.

@S-S-X
Copy link
Member

S-S-X commented Jan 13, 2025

I haven't done any microbenchmarking of these and I'm not sure it even really matters,

I've done and instantiating vm actually does matter and not just a little bit. Though been some time since I've ran those tests, probably something like 4-6 years ago, I think it was with mt 5.2 or maybe 5.3
Anyway, there's still recommendations and warnings on documentation about VM use so I'm pretty sure it is still a thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Performance Aims to improve technical performance.
Projects
None yet
Development

No branches or pull requests

3 participants