search

owengage / fastnbt

Fast serde serializer and deserializer for Minecraft's NBT and Anvil formats
MIT License
186 stars 34 forks source link

Support for multithreading? #46

Closed Aeledfyr closed 2 years ago

Aeledfyr commented 2 years ago

I've been attempting to upgrade from fastnbt/fastanvil 0.15 to the more recent versions (0.23 and 1.3 respectively), and have run into the issue that many parts of fastanvil can no longer be easily used in multithreaded programs, mainly due to the use of RefCell for caching.

Some of these could be trivially replaced using oncecell::sync::OnceCell (which may actually be more efficient), such as in Pre18BlockStates: https://github.com/owengage/fastnbt/blob/3d0850e7cf8d1f68490c13b27d02dcc111d4df07/fastanvil/src/java/pre18.rs#L205-L212

The lazy heightmaps in the current and pre18 java chunks can't easily be fixed though, as they appear to be able to be recalculated multiple times, as recalculate_heightmap is in the public API. (What is the purpose of this?) This could either be changed use a thread safe primitive such as RwLock or Mutex, or it could ensure that the lazy heightmap never needs to be changed after being initialized (and can therefore use OnceCell for efficiency).

The more difficult issues to solve are the cases where RefCell is used for caching within regions, and the non-threadsafe trait objects that the region loader and other types use. The RefCells within RegionBuffer and Dimension could probably be replaced with threadsafe locks. The trait objects may be harder, but in theory adding Send + Sync bounds to the traits or trait objects should make it work.

I can do some of the simple changes like swapping out RefCells with other types, but some of these changes could require API changes or have performance tradeoffs and I'd prefer some input or help.

owengage commented 2 years ago

Hi, thanks for the detailed write up.

It's probably worth me mentioning that fastanvil is generally a demonstration of fastnbt. fastnbt itself I do my best to make sure is efficient, stable, and bug-free--hence being version 1. fastanvil is not version 1 because it's specifically there for owengage.com/anvil. I've not put as much care into the public API of fastanvil. I don't aim to make fastanvil support all use cases.

That said, I'm happy to expore making most of fastanvil types be Send and Sync. I wouldn't be concerned about API changes for fastanvil, and any small performance trade offs for flexiblity would probably sway me.

If you'd like to do the simpler OnceCell based changes first, and I can have a look and hopefully help with the more difficult changes. Let me know if you run into any problems!