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

Make Mapgen (mostly) work multithreaded again #9627

Merged
merged 5 commits into from
May 5, 2020

Conversation

sfan5
Copy link
Collaborator

@sfan5 sfan5 commented Apr 10, 2020

DO NOT SQUASH WHEN MERGING

Problem: The objects managed by BiomeManager, OreManager, DecorationManager and SchematicManager are not thread-safe at all, because:

  • ore generation code caches noise as a member variable
  • schematic may change loaded schematic data for rotation during placing
  • many more

Solution: Create a copy of the managers for each mapgen thread, the implementation of the objects can then freely modify its member variables.

-- Why not teach the implementation to be thread-safe then?
This is unfortunately not as easy and likely requires a larger refactoring*. You can't just e.g. mark the member variables thread_local. So instead I opted for creating a copy of everything, which is guaranteed to work fine with the current code.
The whole thing is misdesigned anyway, it mixes the ore/deco/... settings (which could easy be constant and shared between all mapgens) with mutable implementation details such as cached noise buffers.

* ObjDefManager is also not const correct at all, you can receive mutable references from a const ObjDefManager&. This PR does not fix that.

Consequences

fixes #8300
It is possible to safely run with num_emerge_threads > 1 again, though doing so raises the probability of the following appearing again:

The infamous y=48 bug (#1755, #5125) [mgv7, reproducible 100% with num_emerge_threads = 12 here]
pic

General decoration weirdness on chunk boundaries(?)
pic

With more sane thread values (e.g. num_emerge_threads = 2), these issues are exceedingly rare. So perhaps multiple emerge threads can be enabled by default again soon.

How to test

Test everything related to mapgen, including:

  • Custom biome mods (in particular ones that overwrite biomes)
  • Using Lua mapgens
  • Any mapgen-related Lua API functions
  • test with num_emerge_threads > 1

@sfan5 sfan5 added @ Mapgen Bugfix 🐛 PRs that fix a bug labels Apr 10, 2020
@sfan5 sfan5 requested a review from paramat April 10, 2020 13:48
@sfan5
Copy link
Collaborator Author

sfan5 commented Apr 10, 2020

The general thread unsafeness (in this case: race condition for noise creation) has been present since the very first implementation(s): dcbb953#diff-b4aa9def38b10274e96fd012895653f9R248
The particular crash in #8300 though is caused by 5c1edc5 which adds code that swaps out the noise at runtime.

@nerzhul
Copy link
Contributor

nerzhul commented Apr 10, 2020

nice to see you are working on mutithread fixes

src/mapgen/mg_decoration.cpp Show resolved Hide resolved
src/emerge.cpp Outdated Show resolved Hide resolved
src/emerge.cpp Outdated Show resolved Hide resolved
src/emerge.cpp Outdated Show resolved Hide resolved
@paramat
Copy link
Contributor

paramat commented Apr 10, 2020

The infamous y=48 bug (#1755, #5125)

#1755 is mgv6 only, and is not related to threads at all, that is simply the result of how mgv6 is coded to be incompatible with crossing a mapchunk border. For example the mudflow feature alone disallows crossing a mapchunk border.
#5125 is mgvalleys only, and is no longer valid as that was old mgvalleys code which was a horrible mess, a bug at y = 47/48 was not surprising and probably not related to threads. The bug was fixed but i also then completely rewrote the mapgen later.

However i accept that generally there may be some weirdness at mapchunk borders and overlapping areas with multiple emerge threads.

The code changes in this PR are mostly far beyond my understanding, being code architecture stuff, so i am not qualified approve the code changes, but i can test.

@paramat
Copy link
Contributor

paramat commented Apr 10, 2020

Looked through the changes, no objections to what you are doing here.

@sfan5
Copy link
Collaborator Author

sfan5 commented Apr 10, 2020

However i accept that generally there may be some weirdness at mapchunk borders and overlapping areas with multiple emerge threads.

Right. Any chance you could look into why this happens with mgv7 at some point?
As someone unfamiliar with the mapgen code I'd guess it's a problem with overgeneration (since the area the mapgen works on is one mapblock larger in each direction than the actually requested mapchunk).

@tuedel
Copy link

tuedel commented Apr 11, 2020

Probably related: #5618 #9357

@paramat
Copy link
Contributor

paramat commented Apr 11, 2020

pic

Ah i did not see the above image earlier, Github was acting strange and not loading some images.

I'd guess it's a problem with overgeneration (since the area the mapgen works on is one mapblock larger in each direction than the actually requested mapchunk).

Yes exactly that. All the other non-mgv6 mapgens overgenerate 1 up, 1 down too, do they also do this?
However, only mgv7 has a river generation loop that runs after the main generation loop, which might complicate things, i am intending to move the river generation code into the main generation loop, see #7878
The bug in the screenshot above was reported in #9357 so may not be caused by this PR?

It is possible to safely run with num_emerge_threads > 1 again, though doing so raises the probability of the following appearing again:

To clarify, are you saying that this PR increases the chances of weirdness with multiple emerge threads, relative to before? Or are you saying the old weirdnesses will just be experienced again?

@sfan5
Copy link
Collaborator Author

sfan5 commented Apr 11, 2020

The bug in the screenshot above was reported in #9357 so may not be caused by this PR?

Yes, these are existing bugs. This PR just allows you to "experience" them again since num_emerge_threads > 1 caused crashes before.

To clarify, are you saying that this PR increases the chances of weirdness with multiple emerge threads, relative to before? Or are you saying the old weirdnesses will just be experienced again?

The latter. From my testing it looks like the more emerge threads you use, the higher the likeliness of these bugs reappearing anywhere in the world.

@nerzhul
Copy link
Contributor

nerzhul commented Apr 12, 2020

At least the crash is fixed, for the emerge part i may work on it after this PR is merged to rewrite the emerge workflow which is quite fine but not so easy to understand.

@paramat paramat removed their request for review April 28, 2020 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segfault in noise.cpp
5 participants