ThreadPool Implementation

[modern gale]

func _load_resource(resource_path: String, priority: int):
    
    print("---------------------------")
    print("Adding resource to queue:")
    
    #add the resource and its priority to pool_queue, then re-sort pool_queue
    add_and_sort(resource_path, priority)
    
    print(pool_queue)

func _do_load_resource(args):
    
    var resource_path = args[0]
    var receiver = args[1]
    print("_do_load_resource called!")
    
    #load the resource
    var resource = load(resource_path)
    
    #notify the receiver that the resource has been loaded
    #receiver._on_resource_loaded(resource_path, resource)
    
    var resource_copy = resource.duplicate()
    
    resource = null
    
    emit_signal("resource_loaded", String(resource_path), resource_copy)
    
    game_instance.playerWeaponLoading = false
    game_instance.playerWeaponDoneLoading = true

    receiver.idle = true
    
    receiver.join()
    #wait for the thread to finish and clear it
    

class ThreadPlus:
    extends Thread
    var idle = true

.
.
Note: I had to break the class into two messages because it was over the character limit
.
.

Facts: The _do_load_resource function is doing everything it is supposed to do:

1. It is being called,
2. The emit_signal call is sending the copy of the resource to the target class

But :
3. The thread is not exiting (ie: the receiver.join() call in _do_load_resource is never actually completed).

It becomes idle so it gets picked up again by _process, but when the class tries to call

    idle_thread.start(self, "_do_load_resource", [pool_queue[0][0], idle_thread])

A second time, the _do_load_resource function is never called

I think this is because the thread never finishes executing the first call so it remains blocked

[dusty rover]

What does join() do?

Also note that the emitted signal is running in the thread, not on the main thread

Also I think a typical thread pool would use Semaphore and Mutex to handle dispatching jobs

[modern gale]

It waits for the function to finish executing before unblocking the thread

I can comment it out, the effect is the same

Namely that the thread is never unblocked for some reason

[modern gale]

Yes I'm not sure but I think this has to be like that

Because I want to load the resource in the ThreadPool and then make it available to the main thread asynchronously

It doesn't matter, even if I comment out the entire body of _do_load_resource the thread that .start()s it still won't exit for some reason

[dusty rover]

Most likely you don't want that, because you'll be running code in a thread that shouldn't be

However, can you clarify what is actually in join()? Are you actually joining the thread properly?

[modern gale]

The point of .join() was to try to force the thread to exit before it is called again, but that didn't work as the thread is not exiting

[dusty rover]

So what makes the thread exit?

I'm not sure this style of thread pool is even useful. You may as well just create a new thread when needed

[modern gale]

It's useful, the point of the thread pool is to defer the allocation a resource that would otherwise be heavy

[dusty rover]

That's the purpose of the thread, not the pool

[modern gale]

Yes, that is also the purpose of the thread

But the point of the pool is to limit memory allocation to this process

And make sure that the minimum user can still play the game

[dusty rover]

With a thread pool, I think you would typically want the threads already running, then you would post jobs to them with semaphores

[modern gale]

Yes that was the idea

[dusty rover]

But that's not what you're doing here

[modern gale]

To recycle the threads and have the pool execute them in order of priority

I'm not very familiar with semaphores

I have a naive implementation that creates/destroys a thread each a time a new resource is needed

But to be honest the drop in performance in this case, although a bit better than just loading the resource

Was still noticeable

[dusty rover]

Right, and my point is there's no reason to have a pool for this

I don't think creating the Thread object itself is such a heavy operation that it needs a pool

You could just create a new one whenever a new job comes in

[modern gale]

The problem starts when a user enters a busy area with a lot of load requests

Since I'm not pre-loading everything into memory

[dusty rover]

Well you could still handle the queue and limit number of requests

I'm just saying, pooling the Thread objects doesn't seem very beneficial

[modern gale]

I think that could kill the game experience

[dusty rover]

What? It would be basically the same as you have now

[modern gale]

I think it's different for 3 threads to handle, say, 54 requests in order of priority

Than it is to create 54 threads

[dusty rover]

That's not what I'm saying. I'm saying you're pooling Thread objects, not threads

I don't think there's much benefit to reusing Thread objects over creating a new one each time

Have you considered using the thread pool off of the asset lib instead of making your own?

[modern gale]

Well whatever benefit there may be

[modern gale]

I tried using their resource* queue

I managed to get it to load resources but it wasn't working properly

But I was getting lag spikes of like 200 ms with their resource queue

While naively creating/destroying a new thread each time it was needed brought it down to 60-70ms per load request

[modern gale]

I think that's basically what I'm trying to do here, a thread pool with a priority queue to handle requests

[dusty rover]

But you aren't actually creating a proper thread pool

A thread pool has the threads actually running all the time

[modern gale]

And also I am not using Godot 4.0+ and it seems like this is a Godot 4.x data structure (the thread pool) ?

[modern gale]

Yes that was noted lol

[dusty rover]

The threads are active and paused, waiting for new jobs. You don't start and end them

[modern gale]

Yes, that is what I'm trying to do

[dusty rover]

You need to use semaphores for this

[modern gale]

I see

I thought just .start()ing a function and exiting its scope would unblock the thread

[dusty rover]

The point I'm trying to make that you keep missing, is if you start() a thread for each new job, then the only thing your pool is accomplishing is avoiding creating the Thread object itself, which is not a heavy operation. So your "pool" is not really a thread pool at all

The idea of a thread pool is to have all the thread already started and waiting for the next job

Is this the thread pool you tried? https://godotengine.org/asset-library/asset/556

Even if you don't use it, you should look at it to see how it works

[modern gale]

Well that depends on whether the thread is busy or not no ?

And the # of threads you have in the pool

[modern gale]

If you have a thread "pool" with only one thread then whenever you have more than one consecutive job that condition is already not achieved

[dusty rover]

The single thread would just handle each task in order

Not sure what your argument is here. But yeah, a single thread in a pool isn't very useful

[modern gale]

I think you would need to have some sort of priority queue to handle requests in the background

[dusty rover]

Sure, that's fine

I never said anything against a priority queue

[modern gale]

Otherwise if you tried to .start() a new job while it is still executing that wouldn't work either

[dusty rover]

You start all threads when the pool is created

They all wait for new jobs to be posted

[modern gale]

I'm just trying to figure out why leaving _do_load_resources scope won't unblock the thread

[dusty rover]

Well you've never explained how you're ending the thread

[modern gale]

This is how the thread should end, when _do_load_resource is done executing

You mentioned semaphores

[dusty rover]

But how are you merging the thread?

Threads don't merge on their own

I assume that's what join() was meant to do, but you've never shared what your code for that is

[modern gale]

Wdym merging the thread ?

I thought each thread was supposed to run separately within the scope of the parameters in .start()

[dusty rover]

In order for a thread to "end" it has to be merged into the main thread

You have to call wait_to_finish() from the main thread

[modern gale]

So in this case within the scope of the function started with this call

    idle_thread.start(self, "_do_load_resource", [pool_queue[0][0], idle_thread])

[modern gale]

Yes I tried doing that as well, but the thread still wasn't finishing

[dusty rover]

Did you call it correctly though?

From the main thread?

[modern gale]

I think that's the reason I have .join() and not .wait_to_finish() at the end

[dusty rover]

I wish you would just share the code of join() so I can see

You wouldn't want to call wait_to_finish at the point you call join(). That would cause the thread to wait for itself to finish, but it could never finish because it's waiting for itself to finish

[modern gale]

Aha, I think that's a mistake right there

The Thread type doesn't have a .join() method

[dusty rover]

Right, which is why I assumed that was a method you made

If not, then yeah, your thread was probably just crashing at that point

[modern gale]

But look, even if I replace the current _do_load_resource with an empty function

The thread still won't finish

[dusty rover]

Because you won't be calling wait_to_finish

[modern gale]

Let's try an empty function with a wait_to_finish() call

[dusty rover]

But you have to make sure wait_to_finish is called from the main thread, not from the new thread

[modern gale]

The main thread = the thread that .start()ed the function call ?

[dusty rover]

yes

[modern gale]

func _do_load_resource(args):
    
    var resource_path = args[0]
    var receiver = args[1] #reference to main thread
        
    game_instance.playerWeaponLoading = false
    game_instance.playerWeaponDoneLoading = true

    receiver.idle = true

    receiver.wait_to_finish()    

This is not exiting:

F1 pressed!
---------------------------
Adding resource to queue:
[[res://Assets/Player_Sprites/1h/spritesheet_001.png, 1]]
loading resource = res://Assets/Player_Sprites/1h/spritesheet_001.png
Idle thread =[_Thread:1850]
F1 pressed!
---------------------------
Adding resource to queue:
[[res://Assets/Player_Sprites/1h/spritesheet_002.png, 1]]
loading resource = res://Assets/Player_Sprites/1h/spritesheet_002.png
Idle thread =[_Thread:1850]
F1 pressed!
F1 pressed!

[dusty rover]

Again, you cannot call it from the thread itself

You must call it from the main thread

A common method for this is to queue it: receiver.call_deferred("wait_to_finish")

[modern gale]

Oh, so do you mean like ?

    idle_thread.start(self, "_do_load_resource", [pool_queue[0][0], idle_thread])
    idle_thread.wait_to_finish()

[dusty rover]

You don't want to do that either, because that will freeze the main thread until that sub thread is done

[modern gale]

The problem is this call is being executed within a _process

    idle_thread.start(self, "_do_load_resource", [pool_queue[0][0], idle_thread])

[modern gale]

Yep I figured

[dusty rover]

this

[modern gale]

Does thread have a call_deferred method ?

[dusty rover]

All objects do

Also how are you checking if it's exiting? I don't really understand your output there

[modern gale]

A-ha, that worked

Replaced receiver.wait_to_finish() with receiver.call_deferred("wait_to_finish") seems to be

And now the thread that is set to thread.idle == true is actually finishing

And once it's picked up by the queue again it actually executes _do_load_resources (it wasn't doing that before)

Let's try actually emitting that signal and passing some data now

[dusty rover]

You should defer the emit as well, so it isn't running within the thread context

[modern gale]

Well at least it's working now

F1 pressed!
---------------------------
Adding resource to queue:
[[res://Assets/Player_Sprites/1h/spritesheet_001.png, 1]]
loading resource = res://Assets/Player_Sprites/1h/spritesheet_001.png
Idle thread =[_Thread:1772]
Resource loaded:res://Assets/Player_Sprites/1h/spritesheet_001.png
F1 pressed!
---------------------------
Adding resource to queue:
[[res://Assets/Player_Sprites/1h/spritesheet_002.png, 1]]
loading resource = res://Assets/Player_Sprites/1h/spritesheet_002.png
Idle thread =[_Thread:1772]
Resource loaded:res://Assets/Player_Sprites/1h/spritesheet_002.png
F1 pressed!
---------------------------
Adding resource to queue:
[[res://Assets/Player_Sprites/1h/spritesheet_003.png, 1]]
loading resource = res://Assets/Player_Sprites/1h/spritesheet_003.png
Idle thread =[_Thread:1772]
Resource loaded:res://Assets/Player_Sprites/1h/spritesheet_003.png

One question though

Since I have an extra field (idle) and I'm using it to signify whether or not a thread has exited

Wouldn't this cause problems ?

  receiver.idle = true
  receiver.call_deferred("wait_to_finish")

[dusty rover]

possibly

[modern gale]

Can I have anything after receiver.call_deferred("wait_to_finish")

[dusty rover]

You might want to make a function that you use call_deferred with instead, that runs wait_to_finish and sets the idle property

[modern gale]

So like

  receiver.call_deferred("wait_to_finish")
  receiver.call_deferred("set_idle")
``` ?

Would that work ?

[dusty rover]

I would say more like create a join() function in this thread object that has

    wait_to_finish()
    idle = true```

Then you would do receiver.call_deferred("join")

[modern gale]

That's also good

[dusty rover]

Of course, you really should create a proper thread pool with a semaphore to handle stuff, and not need to use wait_to_finish at all

But I suppose this will work

[modern gale]

What's wrong with emit_signal again ?

[dusty rover]

Any function calls occur within the context of the thread it's called from. That includes emitting signals

That means the code that receives the signal will be running in the thread. If you do anything that isn't thread safe, that is going to cause issues

[modern gale]

Oh I see

Is this the right syntax ?

  receiver.call_deferred("emit_signal", "resource_loaded", resource_path, resource)

[dusty rover]

That depends on where the signal is defined

That would replace receiver.emit_signal(...)

You probably don't want to call it in receiver, because that's not where the signal is

[modern gale]

?

[dusty rover]

Remove receiver.

receiver is the thread object, the signal is not defined there

[modern gale]

Why can't I just ?

  emit_signal("resource_loaded", resource_path, resource)

[dusty rover]

I already explained this

You can just call that deferred. Just don't call it on the thread object

So call_deferred("emit_signal", "resource_loaded", resource_path, resource)

[dusty rover]

Are you threading weapon loads? Why not just have the weapons loaded at the start?

[modern gale]

I would also have to load other pieces of equipment and it's too heavy on the engine

[dusty rover]

How is it too heavy?

[modern gale]

It's RAM heavy

[dusty rover]

Is it though? That tiny sprite is too RAM heavy?

[modern gale]

It's 3d rendered so there are a lot of frames

[dusty rover]

Well keep in mind threads can only do so much

[modern gale]

I figured haha

[dusty rover]

Modifying the scene tree must always be done on the main thread, and that could easily cause small lag

[modern gale]

That's still being done on the main thread

But the resource loads on the thread pool

[dusty rover]

And I'm suggesting the lag could be caused by the actual adding to the scene part

[modern gale]

I mean the player.weaponSprite.texture = ... call

[modern gale]

Do you think Godot could be re-loading the resource ?

When the signal is emitted that is

[dusty rover]

If you aren't holding on to the reference, then absolutely

But no, not immediately after loading from the thread

Unless you're calling load() on it

Are you sure you aren't over-optimizing?

[modern gale]

I am, I have the following in Game.gd:

func _on_resource_loaded(resource_path, resource):
    # handle loaded resource here
    loaded_resources[resource_path] = resource

[modern gale]

Have you seen how the enemies rubberband ?

[dusty rover]

If you just load all your weapons at the start, you won't have to worry about any of this

That's what I mean by over-optimizing

Are you sure you aren't over worried about RAM usage?

[modern gale]

There are a lot of equipment spritesheets

[dusty rover]

Well those don't change during combat I assume

[modern gale]

Why not ?

[dusty rover]

Do they? Do you not have to open some kind of inventory screen to change your armor?

[modern gale]

Nope I want players to be able to change sets with F1

[dusty rover]

Needing to load a weapon every time you switch weapons seems very inefficient

[modern gale]

Like hotkey equipment changes so they can combo

[dusty rover]

Keep any "active" items always loaded

If a player can switch these any time, then constantly loading them seems incredibly inefficient

This is why we have RAM

[modern gale]

Haha right

But I also don't want a 2D game to take up like 8 GB RAM

[dusty rover]

Then optimize your sprite sizes

Constantly loading cannot be a good solution

[modern gale]

That could significantly limit the player base

[dusty rover]

This would feel terrible anyway, wouldn't it? If you have a split second delay every time you switch a weapon due to needing to load it again?

[modern gale]

Yep that looks pretty bad

[dusty rover]

Any weapon that is easily switchable needs to be constantly loaded. That doesn't mean you need to have every weapon in the game loaded, but any weapon that is actively equipped needs to be already loaded

[modern gale]

This is a networked game, so I'd have to pre-load everything because of other players

That wouldn't even be that bad if only pre-loaded the "heavier" sprite sheets like armor pieces

But like, the fact that a 300kb sprite sheet takes like half a second to load is pretty surprising

And even if it did, I don't see why it would lag the entire game since it's being offloaded to a dedicated separate thread

[dusty rover]

It shouldn't, but debugging the exact issue is complicated

Especially when you also have to deal with networking

[modern gale]

I'm not 100% convinced that's not because I'm running the game and the server on the same machine though

[dusty rover]

That could also confuse things

[modern gale]

I mean, multithreading is definitely a necessity

Since with load the player itself was rubberbanding

And player movement is like 100% client side

So it's not like this was all for nothing

But yeah I feel like there might be something going on server side as well

I'm also seeing lag spikes in the server's profiler

[dusty rover]

You might want to try to remove the networking aspect for now so you can focus on the loading issue

It doesn't seem like a load() should cause rubberbanding, it should just cause a lag spike

[modern gale]

I mean client side player movement should be unaffected by server lag

It's like fire and forget

For now

I'll try to set up the server on my laptop so I can get the full picture

[dusty rover]

You should really try to get an entirely offline singleplayer going

You need to eliminate that variable entirely

[modern gale]

I'm not sure that'd be a smart thing to do

Like the game needs to be networked and it needs to be able to work with the network

[dusty rover]

Sure, but you also want to make sure your loading is smooth without worrying about what effect the network is having

It's hard to do that if you have other things that could be causing issues

You need to be able to separate them in order to properly test and debug

[modern gale]

Agreed

[dusty rover]

Obviously you want to be building from the ground up with networking in mind, so don't think I'm saying you shouldn't. But I think at the very least you should have some kind of non-networked test scene you can use to test your loading systems

[modern gale]

Yep it is still lagging the game

Even if I move the client to a different local machine

It's even worse b/c my laptop is significantly worse than the machine I am using to run the server

So that'd be even closer to a "production" environment

[modern gale]

@dusty rover I have around 100 weapon sprite sheets , about 1 MB each

If I tell Godot to try to pre-load everything, the game ends up taking up ~16 GB ram

If loading 100 spritesheets takes 16GB , I'm assuming 1 should take 160MB ?

Which I think may have something to do with the fact that loading the textures during runtime is lagging the game

This in Game.gd :

func _ready():
    for i in range(0, 100):
        var pad = zero_pad(i, 3)
        var weaponTexturePath = "res://Assets/Player_Sprites/1h/spritesheet_" + String(pad) + ".png"
        var texture = load(weaponTexturePath)
        loaded_resources[weaponTexturePath] = texture
    pass