How to get the server world from a chunk generator

[Slit_bodmod]

Hi, sorry small topic but I am writing a custom chunk generator and in order for the generation to work properly I need to access some data stored in world saved data, and thus I need a serverWorld object, at first I starting by casting the IWorld object I am handed in the function I need it for (fillFromNoise aka func_230352_b_) into a ServerWorld and using that, it didn't work, so clearly I must be getting some different world object in the chunk generator.

My question is which world object am I getting and how do I convert or access it's associated ServerWorld object safely?

[Draco18s]

Chunk generation happens on a separate thread, so the IWorld is all you're going to get. You cannot access the main server world yet, both because (a) it might not exist and (b) it wouldn't be thread safe.

[Slit_bodmod]

Ah that makes a lot of sense, thankyou, so what would you recommend in order for me to get an instance of my worldSavedData object in time for chunk loading to begin? is there some way I can make sure that the chunk generator has an instance of this object before the process begins or some way to reach across in a safe manor?

[diesieben07]

36 minutes ago, Slit_bodmod said:

so what would you recommend in order for me to get an instance of my worldSavedData object in time for chunk loading to begin?

TL;DR: You cannot. I can't see a way to any data into your ChunkGenerator safely.

[Slit_bodmod]

well what kind of alternatives are there for having level saveable data that my chunk generator can use, is there a way for me to pass other parameters into the constructor. Or a way for me to send data of any kind across the two threads. Maybe by saving a copy of the data statically somewhere and setting up appropriate checks around it?

Edited June 14, 2021 by Slit_bodmod

[diesieben07]

The thing is that your chunk generator is generated deep inside the data pack loading - before there is even a world (check where DimensionGeneratorSettings.CODEC is used - this is what ultimately will instantiate your chunk generator).

fillFromNoise will (as far as I can tell) always be called on a WorldGenRegion (don't rely on this, mods might do bad things). IWorld has a method getLevel which will return you the ServerWorld - but not all implementations will provide you something useful here (all vanilla implementations, WorldGenRegion and ServerWorld, do, but again, don't rely on this). So, you can get at the ServerWorld by calling getLevel. Now what? Well, you have achieved nothing, because you cannot use this world in a useful way. You are running on a world generation thread and ServerWorld is not threadsafe. Do not be tempted to ignore this, you will crash the game or cause strange behavior at some point. It will probably work fine for a long time. Then crash or do something stupid once - and then behave again.

You could try and create some kind of thread-safe storage that your generator can then use. How do you populate it? You cannot use WorldEvent.Load because the game actually does a whole bunch of "setting up" (e.g. setting the spawn position) before that fires - which loads chunks. What you have to do instead is use AttachCapabilitiesEvent<World> which fires right from the ServerWorld constructor. In there, get your WorldSavedData and put it in a thread safe map with weak keys (Weak keys because otherwise your map will prevent worlds from being unloaded; Guava's MapMaker does this for you: MapMaker().weakKeys().makeMap()). Then you can access this map from your chunk generator using the ServerWorld you get from the IWorld. Note that whatever data you access in the WorldSavedData must still be threadsafe (for example you can't use an ArrayList).

This is untested - but should work.

[Draco18s]

7 hours ago, diesieben07 said:

This is untested - but should work.

I read that like I read this:

import sun.java.misc.unsafe

[Slit_bodmod]

ok, thankyou that's very helpful. I have begun work implementing it and was wondering if I could ask a few questions?

- Would the values of my map also want to be weak as I presumably am also storing in the way that WorldSavedData automatically does so I don't want a copy of it to be stored

-Will the reference in my map remain a reference, there is a good chance that the main thread will change the contents of my WorldSavedData and Ideally the chunkGenerator should respond to these changes appropriately

- I don't know much about the internal structure of the jvm threading but seeing as I can guarantee that my ChunkGenerator won't be writing to the WorldSavedData, only reading, is there any chance of the multiple threads causing issues in this case

Quote

22 minutes ago, Draco18s said:

I read that like I read this:

import sun.java.misc.unsafe

 

-yes, would utilising the unsafe class be a good idea and how best can I go about it

-Also purely out of interest, would this system have been easier if I'd used capablities or mixins?

[diesieben07]

On 6/14/2021 at 6:00 PM, Slit_bodmod said:

- Would the values of my map also want to be weak as I presumably am also storing in the way that WorldSavedData automatically does so I don't want a copy of it to be stored

Which map do you mean? I am not sure what you mean here. I meant you should have a Map<ServerWorld, WorldSavedData> which holds your WorldSavedData instance. You cannot use the normal way of getting it, because that uses a non-thread-safe map.

On 6/14/2021 at 6:00 PM, Slit_bodmod said:

Will the reference in my map remain a reference, there is a good chance that the main thread will change the contents of my WorldSavedData and Ideally the chunkGenerator should respond to these changes appropriately

These are basic Java concepts. Objects in Java are passed by reference.

On 6/14/2021 at 6:00 PM, Slit_bodmod said:

I don't know much about the internal structure of the jvm threading but seeing as I can guarantee that my ChunkGenerator won't be writing to the WorldSavedData, only reading, is there any chance of the multiple threads causing issues in this case

Yes, you can have issues. Multiple threads require synchronization as long as at least one of them is writing.

On 6/14/2021 at 6:00 PM, Slit_bodmod said:

-yes, would utilising the unsafe class be a good idea and how best can I go about it

He's joking. Don't use Unsafe.

On 6/14/2021 at 6:00 PM, Slit_bodmod said:

-Also purely out of interest, would this system have been easier if I'd used capablities or mixins?

Capabilities is the same. Mixins has nothing at all to do with this.

[Draco18s]

I was absolutely joking. The unsafe package is called "unsafe" because it is not safe to use.

[Slit_bodmod]

Hi sorry for late response,

Ok so I should create a map<World,WorldSavedData> that when a World Capability is attached will thread lock, and add a mapping of that world to it's appropriate savedData

Then in my chunk generator, using IWorld.getLevel() (on an instance ofc), I can look it up in the map and modify it at will, and if I ever do need to write to it for whatever reason I need to lock it first.

If this is true what system would you recommend to keep the variables safe? Should I use a java Lock class for the functionality I stated above or make the map volatile or reading data from the map synchronised?

[diesieben07]

16 minutes ago, Slit_bodmod said:

Then in my chunk generator, using IWorld.getLevel() (on an instance ofc), I can look it up in the map and modify it at will, and if I ever do need to write to it for whatever reason I need to lock it first.

No, you must lock / synchronize anyways, because the main thread might modify the WSD / Capability. As such you have at least one thread that is writing and as such you must synchronize.

17 minutes ago, Slit_bodmod said:

If this is true what system would you recommend to keep the variables safe? Should I use a java Lock class for the functionality I stated above or make the map volatile or reading data from the map synchronised?

For the map itself use the MapMaker map like I told you further up. It is threadsafe. As for the variables in your WorldSavedData / Capability, the easiest thing would be to use synchronized.

[Slit_bodmod]

ok so would it work if I just put and get objects from my map as though everything was normal. and then once I've got my reference I need to write a synchronised method that will pull the data I need and return it to the chunk generator. (I'm only gonna need to pull one object so I think it would it be better for the synchronised function and return a deep copy?)

Also how do I get the ServerWorld from the IWorld, there isn't a function defined called getLevel, nor any function that returns a ServerWorld as far as I can see.

[diesieben07]

7 hours ago, Slit_bodmod said:

ok so would it work if I just put and get objects from my map as though everything was normal. and then once I've got my reference I need to write a synchronised method that will pull the data I need and return it to the chunk generator. (I'm only gonna need to pull one object so I think it would it be better for the synchronised function and return a deep copy?)

Yes, this sounds good. However show your code once you've implemented this.

7 hours ago, Slit_bodmod said:

Also how do I get the ServerWorld from the IWorld, there isn't a function defined called getLevel, nor any function that returns a ServerWorld as far as I can see.

You need an IServerWorld (do an instanceof check).

[Slit_bodmod]

ok so obviously I have a line of code that says this

public static final Map<World,TardisManager> INSTANCES = new MapMaker().weakKeys().makeMap();

then I have my init function that looks like this

@SubscribeEvent public static void onWorldLoading(AttachCapabilitiesEvent<World> event) {
        World world = event.getObject();
        DWMCMod.LOGGER.error("attaching capabilities"+world);
        if (world instanceof ServerWorld) {
            ServerWorld accessWorld = world.getServer().getWorld(DWMCDimensions.TARDIS_WORLD_KEY);
            TardisManager tardisManager = getTardisManagerForWorld(accessWorld);
            INSTANCES.put(accessWorld,tardisManager);
        }
    }

as you can probably guess, TardisManager is the WorldSavedData, it's basically a wrapper for a map that stores instances of Tardis which is kinda simple value struct, it's pretty much entirely primitive types (although it does contain an enum I'm going to have to pull an attribute from)

my only function that is synchronised thus, it is the one that pulls the tardis from the tardis manager

public synchronized TardisManager.Tardis getThreadedTardisForPos(IWorld world, BlockPos pos) {
        if (world instanceof IServerWorld) {
            ServerWorld server = ((IServerWorld)world).getWorld();
            TardisManager manager = INSTANCES.get(server);
            return new TardisManager.Tardis(manager.getTardisForBlock(pos));
        }
        DWMCMod.LOGGER.error("this shouldn't not be a server world I hope");
        return null;
    }

this is what that looks like, again, as you can probably guess, the TardisManager.Tardis constructor that takes in another TardisManager.Tardis is basically a copy constructor. it gets only once at the start of the fillFromNoise method in the chunk and the object it returns (that hopefully shouldn't interfere with other threads).

I think that's everything right?

[Slit_bodmod]

Oh also that enum I was talking about. So the attribute I need from that is a structure Template stored as a const variable within each instance of an enum. It thus only gets written to when the class is first inited and doesn't get edited beyond that so is that thread safe?

just for clarity, it's an enum that a reference to is stored in TardisManager.Tardis containing a constant field of type Template

[diesieben07]

15 hours ago, Slit_bodmod said:

my only function that is synchronised thus, it is the one that pulls the tardis from the tardis manager

This doesn't need to be synchronized, as the MapMaker map is threadsafe. What needs to be synchronized is that getTardisForBlock method. As for the TardisManager.Tardis class - if it only has final fields (and those fields do not contain mutable data such as lists) then it is fine to use without synchronization. However I assume the constructor of TardisManager.Tardis reads some data from whatever getTardisForBlock returns - this also must be threadsafe.

[Slit_bodmod]

ah right, so the Tardis manager contains a few different get functions for different Tardises, will they all need to be synchronised, will this impact performance as it's only this one niece case where they need to be accessed by another thread.

getTardisForBlock returns a TardisManager.Tardis. the constructor that takes for TardisManager.Tardis that takes in another TardisManager.Tardis is basically just a copy constructor that returns a new object with the same values for this thread to modify and read.

in-fact the Tardis manager stores all instances of Tardis that the program ever uses inside of a map, all constructors for the Tardis are default accessibility because of this (except for the copy constructor which is public). If I made this map using "new MapMaker().makeMap()" would that help maybe.

As for TardisManager.Tardis, not all fields are mutable, including some I need for the chunk generator can be changed in code, however I don't really want these values changing during the generation of a given chunk atleast for the Tardis object that it's working on (which is why I made the function return a new copy)

Essentially would it make sense for me to have some synchronised function(or block in a function) that takes the Tardis from the original map and copies it in a thread safe manor.

Would it help if I sent the TardisManager.Tardis file, it's quite long as it also contains some relevant maths and networking functions so I could just send the necessary excerpts?

[diesieben07]

21 minutes ago, Slit_bodmod said:

Essentially would it make sense for me to have some synchronised function(or block in a function) that takes the Tardis from the original map and copies it in a thread safe manor.

Definitely this. You should make a copy of the data you need on the main thread (i.e. in the AttachCapabilitiesEvent) and put that data into the map, if you can afford this. If it is too much data to copy you need to have thread safe data structures for all data that is accessed from both threads. Just "slapping synchronized on it" is usually not the right solution.

[Slit_bodmod]

right, well the trouble with creating a copy on the main thread is that the data is going to be modified as the world is running, particularly from the overworld (seeing as this chunk generator is designed to work for a specific dimension). so ideally I need the data to be the most up to date the moment I create a new chunk. That being said if the data changes while the chunk is being generated (or preferably when the region is being generated), the main thread still needs to keep track of that but I don't want the chunk generator to have its copy changed.

So getting a hold of the TardisManager is done through a thread safe map atm. Getting a reference to the specific tardis required is currently done from a non-thread safe map so can I just make it a thread safe map and that will be fine.

If so then real difficulty seems to be then getting all of the data from the Tardis object that map holds a reference too. As discussed the best way to do that is to try and create a copy of it in a thread safe manor for use by the chunk generator. The question obviously is how. I presume then synchronised won't help me out here as I can't make every function that writes into this data structure so I need to somehow lock the whole datastructure for writing at the start of the copy function and unlock it at the end, how do you think I might do this?

If this is the case should I remove synchronised from getThreadedTardisForPos function?

 

[diesieben07]

9 hours ago, Slit_bodmod said:

so I need to somehow lock the whole datastructure for writing at the start of the copy function and unlock it at the end, how do you think I might do this?

You can either really just use synchronized or if you want to squeeze every last bit of performance out you can use ReentrantReadWriteLock which has a separate read and write lock (write lock is exclusive, read lock is not). There are also lock free algorithms and data structures you could use, but those are usually difficult to use.

9 hours ago, Slit_bodmod said:

If this is the case should I remove synchronised from getThreadedTardisForPos function?

I don't know. Does it access a non-thread-safe data structure (such as a HashMap) which is accessed from more than on thread? If so: It needs to synchronize. You have to think in terms of your data structures, not in terms of individual methods. If you have a HashMap and use it from multiple threads all accesses to it need to do some form of synchronization.

[Slit_bodmod]

On 6/19/2021 at 8:03 AM, diesieben07 said:

Does it access a non-thread-safe data structure (such as a HashMap) which is accessed from more than on thread?

No, I think if I make the underlying data structure in my TardisManager using a MapMaker it will be fine.

As for using the ReentrantReadWriteLock appropriately. I assume I should lock it at the beginning of the copy constructor and unlock it at the end. What impact will this have. when I read/write to the other fields in my data structure, I assume it won't automatically be safe, do they have lock it first and then write to the data strucuture?

[diesieben07]

2 minutes ago, Slit_bodmod said:

No, I think if I make the underlying data structure in my TardisManager using a MapMaker it will be fine.

Only use the MapMaker map for the map with world keys so that you can have weak-reference keys. For normal concurrent maps use ConcurrentHashMap, it has better performance. Also note that "just use ConcurrentHashMap" will not magically make all your threading problems go away.

3 minutes ago, Slit_bodmod said:

As for using the ReentrantReadWriteLock appropriately. I assume I should lock it at the beginning of the copy constructor and unlock it at the end.

No. A ReentrantReadWriteLock has two locks. To guard a data structure using it you have to write-lock every time anyone writes to it and read-lock every time anyone reads from it. You cannot just lock in some places and not lock in others.

[Slit_bodmod]

8 minutes ago, diesieben07 said:

Also note that "just use ConcurrentHashMap" will not magically make all your threading problems go away.

yes but will it stop me getting thread issues when reading and writing to that hash map?

12 minutes ago, diesieben07 said:

To guard a data structure using it you have to write-lock every time anyone writes to it and read-lock every time anyone reads from it.

right, I gather the best way to go about doing this is with getters and setters for every variable (aaargh), and also locking and unlocking the read locks at the start of all functions, does this include the constructors, I assume not. I assume I do at the start of my serialisation function.

also am I right to assume that the syntax for the locks is:

my_class_lock.readLock().lock()

my_class_lock.readLock().unlock()

my_class_lock.writeLock().lock()

my_class_lock.writeLock().unlock()

and I use the read locks for reading, write locks for writing, lock at the start and unlock at the end

[diesieben07]

3 minutes ago, Slit_bodmod said:

yes but will it stop me getting thread issues when reading and writing to that hash map?

Not necessarily. Even with ConcurrentHashMap you can still have a Time-of-check to time-of-use issue:

ConcurrentMap<String, Integer> map = new ConcurrentHashMap<>();

void increment(String key) {
  Integer current = map.get(key);
  map.put(key, current == null ? 1 : current + 1);
}

This code is broken, even though it uses ConcurrentHashMap. If multiple threads call this method, two threads may both read 1, add 1 to it and put 2 in the map - essentially losing one increment. Concurrent programming is hard and not at all trivial.

6 minutes ago, Slit_bodmod said:

right, I gather the best way to go about doing this is with getters and setters for every variable (aaargh), and also locking and unlocking the read locks at the start of all functions, does this include the constructors, I assume not. I assume I do at the start of my serialisation function.

in theory, yes, but also not really. See above.

7 minutes ago, Slit_bodmod said:

also am I right to assume that the syntax for the locks is:

my_class_lock.readLock().lock()

my_class_lock.readLock().unlock()

my_class_lock.writeLock().lock()

my_class_lock.writeLock().unlock()

and I use the read locks for reading, write locks for writing, lock at the start and unlock at the end

Yes, but you have to always use try-finally, so you never forget to unlock.