Cubitect / cubiomes-viewer

An efficient graphical Minecraft seed finder and map viewer.
GNU General Public License v3.0
1.04k stars 62 forks source link

Big performance loss #91

Closed PancakeTAS closed 2 years ago

PancakeTAS commented 2 years ago

Today I noticed that cubiomes-viewer searches really slowly. I did some testing and figured out that 1.10 is the version that made it so much slower.

To recreate: 1) Add Condition for Village within 500 centred square of side 2) Add Condition with Spawn relative to the Village and within 20 centred square of side 3) Launch on 1.9.1 and 1.10 and check the speeds on incremental.

In the newest version this becomes very apparent and on incremental my computer handles around 1000 seeds per second, which is nothing compared to the previous 3 million.

Cubitect commented 2 years ago

Testing for MC 1.17, I get around 100K seeds/s in both cubiomes-viewer 1.9.1 and 1.12.1, so I don't seem to be able to fully reproduce this. There was a change in 1.10 that may cause a performance penalty, since relative conditions were broken before and the fix unfortunately causes it to be a little bit slower for this search*, but certainty not that much. Maybe you had a smaller search item size in the 1.9.1 test? The search item size controls how many seeds are processed together before evaluating the results, so for searches that cover many seeds per second the search item size should ideally be larger and vise versa.

Does the Problem persist with a larger number of seeds per search item?

* Before 1.10, it was undefined behavior when more than one structure was in an area and that condition was used for a relative center. In the newer versions all the instances are found and the average position is used for the relative center. The performance should in theory not be affected that much for sufficiently restrictive searches.

PancakeTAS commented 2 years ago

Alright, lets do some testing.

Default Seeds per thread search item (64):

Seeds per thread search item (4096):

Pretty big gaps, it seems like there is some giant bottleneck or something. Tests done with conditions from the first post and 1.16.

I check for a Village within -250-250+250+250 which is a big portion of the map, maybe it trying to search for all structures except for one makes it so much slower? To test that I decreased the 500 size to just 50 and the 1.12.1 version is so much faster now.

For us minecraft TASers this is important because we often go for perfect seeds. Optimally we find the player spawn and check if a village is there (or the other way around, village first and spawn last). Then we multiply the coordinate for a wrong warp and check if a stronghold is there. If 1.10+ now uses the average position that's very bad for us. Please add an option to goggle on or off the average position searcher

Cubitect commented 2 years ago

This is certainly slower, but sounds a lot different to 3M/s vs 1K/s.

The idea was, that reference points are to be used when instances should be treated individually rather than as a group. This makes the search more thorough, but it also introduces a new performance bottleneck that can be even worse. So the best cause of action is probably to add something like a 'fuzzy' search option that restores something similar to the old behavior.

PancakeTAS commented 2 years ago

That sounds like a great idea :D.

Reference Points are also a bit too complicated for the average user and will probably not be used by that many people

Cubitect commented 2 years ago

The problem is, if the search is not well defined, people also report buggy behavior and many thorough searches become impossible.

If you only want to find some seeds that meet these requirements, I suggest you restrict the village to a smallish area away from the origin, since strongholds only generate at certain distances anyway. Here is a example search filter that I threw together based on where strongholds generate, keep in mind I didn't spend long trying to optimize this: village_to_stronghold_search.txt (Load with File->Load progress...)

PancakeTAS commented 2 years ago

The search you sent me does indeed check about 3 million seeds per second which is faster than before. The Reference Points work pretty well in this scenario and I will definitely include them in my future searches. Thanks for helping!

So the best cause of action is probably to add something like a 'fuzzy' search option that restores something similar to the old behavior.

I would still love to see this feature. Theoretically it should be able to just use a reference point condition in the background right?

Cubitect commented 2 years ago

In the release 2.0 the optimization potential is determined from the dependencies the condition has. So the fuzzy option is no longer needed as this happens automatically.