rhai icon indicating copy to clipboard operation
rhai copied to clipboard

Unable to use AST across threads without making app state Sync + Send

Open djgaven588 opened this issue 8 months ago • 8 comments

Hello! I have a game I am working on where recently I have been working towards using more threads and improving performance.

An issue I ran into while doing this, is that my game has 2 types of data:

  • Assets which can be moved between threads (including things with Rhai scripts)
  • GameState which is always on a single thread, and is passed to scripting as an argument

When trying to make it so I can execute Rhai scripts across threads (splitting my client and server), I found that in order to use things like AST on another thread I needed to enable the sync feature. However, this forces all types to require Sync + Send, which things such as my game state can't do, as some of these references are in fact tied to a specific thread.

Is there any way to get around this to somehow have AST Send + Sync, but not require all arguments to scripts to calling Rhai functions to also be Send + Sync?

djgaven588 avatar May 09 '25 01:05 djgaven588

Well, sync was not intended for very fine-grained work. It is certainly course-grained.

The main reason why the AST is required to be Send is because it can hold a Dynamic. And if you want to put non-thread-save stuff inside, like your game state, then the AST is also not thread-save.

I'm not sure whether it is possible to make it so fine-grained. Is there any other way that you can achieve what you want?

Alternatively, the AST can be made to be local but that means removing any node variant that holds Dynamic.

schungx avatar May 09 '25 03:05 schungx

The main reason why the AST is required to be Send is because it can hold a Dynamic. And if you want to put non-thread-save stuff inside, like your game state, then the AST is also not thread-save.

From my brief glances through Rhai, I wouldn't expect any of my types to arrive within the AST itself. While a Dynamic could hold that type of value, I don't believe it is possible to get when compiling the script. I exclusively hand these off when calling the functions for my various events like so:

response = engine.call_fn_with_options::<EventResponse>(
	CallFnOptions::new().rewind_scope(false),
	&mut scope,
	&ast,
	"on_tick",
	(entity, game_state),
)

With this, each execution gets it's own (empty) scope, and the variables in question are purely passed as function arguments. Would this ever cause that to happen?

I'm not sure whether it is possible to make it so fine-grained. Is there any other way that you can achieve what you want?

I've tried a few, including trying to make my game itself Sync + Send (can't due to graphics references), and generating an AST per thread (haven't figured out a working solution as I also need to do hotreloading while also allowing calling the same function within itself), however these introduce a large amount of complexity and potential slowdowns right in a hot path, which I would like to avoid.

djgaven588 avatar May 09 '25 04:05 djgaven588

From my brief glances through Rhai, I wouldn't expect any of my types to arrive within the AST itself. While a Dynamic could hold that type of value, I don't believe it is possible to get when compiling the script. I exclusively hand these off when calling the functions for my various events like so:

Upon deeper look, it seems the culprit may be ImmutableString, which is a reference-counted shared string instance. It is widely used within the AST to reduce allocations. That means the AST cannot be Send + Sync unless ImmutableString is specifically wrapped with Arc.

schungx avatar May 09 '25 06:05 schungx

I did manage to find a way to get my game to function (still need a bit more work to fully tell).

struct VoxelExecutionContext {
    cache: RefCell<HashMap<String, Rc<(rhai::AST, VoxelScriptCapabilities)>>>,
    engine: rhai::Engine,
    last_load: RefCell<Instant>,
}

// ... VoxelPrefab
thread_local! {
	static AST_CACHE: VoxelExecutionContext = VoxelExecutionContext::default()
}

fn get_script(
	&self,
	context: &VoxelExecutionContext,
) -> Result<Rc<(rhai::AST, VoxelScriptCapabilities)>, String> {
	if context
		.last_load
		.borrow()
		.lt(&self.reloader.last_load.read().unwrap())
	{
		context.cache.borrow_mut().clear();
		*context.last_load.borrow_mut() = Instant::now();
	}

	let mut cache = context.cache.borrow_mut();
	let script = if let Some(script) = cache.get(&self.reloader.watch_path) {
		script.clone()
	} else {
		match compile_ast(&self.reloader.watch_path) {
			Ok(script) => {
				let script = Rc::new(script);

				cache.insert(self.reloader.watch_path.clone(), script.clone());

				script
			}
			Err(error) => {
				return Err(format!("Voxel Script Compilation Error: '{}'", error));
			}
		}
	};

	Ok(script)
}

Essentially does per thread script compilation, and keeps a local cache that can get wiped in the event of a reload.

The main problem though is this requires a thread_local and a hashmap for getting the cached ASTs, where as with the previous setup it just a field access. So it's a bit more expensive to get the AST, and the hot reloading needs to be done for each executing thread instead of once.

djgaven588 avatar May 09 '25 07:05 djgaven588

True, but essentially you're keeping a copy of each script on each thread. This would Nx your memory requirements (N = number = threads) considering all the scripts are run on all threads.

Not ideal at all...

What would be needed is a facility to make ImmutableString wrap with Arc instead of Rc. Let me work on that...

schungx avatar May 10 '25 01:05 schungx

I have done an experiment attempting to make AST Send + Sync without the sync feature. It appeared possible, but at the expense of turning off a bunch of language features (such as closures).

Still I got stuck at the last step where the AST needs to embed a shared module with all the function definitions. Making this module always Send + Sync is a huge chore as all function instances and metadata are shared and they also need to be Send + Sync etc.

In other words, it does not seem to be as easy as I originally anticipated.

So unfortunately at this point it is not possible to make AST : Send + Sync while Dynamic : !Send.

schungx avatar May 10 '25 02:05 schungx

Looked through a bit more based on your description, ya, that Dynamic would be a big issue.

Only idea I am able to come up with is having SyncDynamic vs Dynamic, where SyncDynamic would be used in places like within module and contain the majority of existing data, and Dynamic could contain non Send + Sync types or just a SyncDynamic, largely being used for runtime execution, scope, ect.

So sync is off, everything is Dynamic, if sync is on, everything is SyncDynamic, and "shared" could be the above split.

Not sure if that would actually be practical, but it seems like where such a division line could be drawn to separate the AST from execution.

djgaven588 avatar May 10 '25 07:05 djgaven588

Only idea I am able to come up with is having SyncDynamic vs Dynamic, where SyncDynamic would be used in places like within module and contain the majority of existing data, and Dynamic could contain non Send + Sync types or just a SyncDynamic, largely being used for runtime execution, scope, ect.

Yes, I thought of that too, and then Dynamic would embed SyncDynamic. However, the restrictions on modules would be significant.

Alternatively, you can try to make your game state Send + Sync using unsafe. However, you just have to make sure that you don't actually access the type cross threads...

schungx avatar May 10 '25 08:05 schungx

Closing this for now as there isn't a simple solution currently.

schungx avatar Aug 06 '25 04:08 schungx