Skip to content

Make ThreadPool support Deque again. - #1958

Merged
player-03 merged 27 commits into
openfl:8.3.0-Devfrom
player-03:ThreadPool_Deque
Jul 7, 2025
Merged

Make ThreadPool support Deque again.#1958
player-03 merged 27 commits into
openfl:8.3.0-Devfrom
player-03:ThreadPool_Deque

Conversation

@player-03

Copy link
Copy Markdown
Contributor

The 8.2.0 version of ThreadPool uses the main thread to schedule threads, due to restrictions in HTML5. I figured it was easiest to make that behavior consistent across platforms, and it was worth a bit of extra overhead to get HTML5 web worker support. But users have complained about performance when running lots of small jobs (a use case I hadn't considered), so I finally got around to bringing Deque back.

This PR uses a flag (lime_threads_deque) to determine whether to use Deque. It's enabled by default on native targets, but you can comment it out for testing purposes. I'm hoping that we can do something fancy with HTML5 to make the equivalent of a Deque, and then remove the flag. That's why I made a new flag instead of just using #if !html5: it will make the transition easier in the future.

I've tested this PR in my own use cases, making sure all flag combinations work. Single-threaded, multi-threaded, with and without Deque, on native and HTML5. (Except I didn't test Deque on HTML5, because that isn't available.) But this is still a big rewrite, and it would be easy to miss something, so I'd appreciate if other users could test their use cases too.

Fixes #1895, supersedes #1837.

Since we're storing more and more in `JobData`, and the background thread only needs three bits of that data, I added those to `ThreadEvent` so we don't have to pass the full object. This may improve performance in HTML5 specifically, where passing a class instance incurs an overhead.
Now that thread pools can manage both types of job at once, we can't rely on `mode` to determine whether we're on a background thread. Honestly, I shouldn't have relied on it in the first place.
No need to set `__mainThread` based on `isWorker()` when `isWorker()` already gives the information we're after.
For single-threaded jobs, this means a pool can now handle multiple per frame. For multi-threaded jobs, this only slightly reduces the delay between jobs.
Forgot this when overriding the "send" functions.
It's a private variable, so all it really needs to do is mention the location it gets used.
Plus, alphabetize the variables.

In 8.2.0, a single-threaded pool would report `currentThreads == 1` when running a job, meaning it counted the main thread. But in retrospect, this was both redundant (with `activeJobs`) and unexpected, so I'm counting it as a bug.
All the jobs in `__multiThreadedJobs` are already known to be running in multi-threaded mode. This is left over from when pools were locked to a single mode.
We still need to throw an error when `mode` is `MULTI_THREADED`, but this can now vary per job, so the check must happen during `run()`.

Also, the old error message was out of date. You can't pass a function to the `ThreadPool` constructor.
On other targets, it will return `true`, which will be inlined and optimized out. The conditional compilation just added clutter.
Using the flag `lime_threads_deque` to distinguish between this and the old approach. Hopefully someday we can remove this flag, once we implement something akin to `Deque` in HTML5.

That aside, the hardest part is keeping track of the state of each thread. That's why there's so much more complexity: whenever the main thread sends a message to a worker, it needs to wait for confirmation, but also not send any more messages while waiting. And the code that tracks all this needs to work in both modes (with and without `Deque`).
@rainyt

rainyt commented Jun 26, 2025

Copy link
Copy Markdown
Contributor

I tested the HTML5 target of the game project and it performed well, with speed restored to before.

@skial skial mentioned this pull request Jun 26, 2025
1 task
After all jobs are done, threads will send `EXIT` events. These must be processed before removing `__update`, otherwise they'll linger in the queue until the next job starts.
Previously, while a thread was exiting, `ThreadPool` could think it was still idle. This would cause it not to spin up a new thread for the next job, leading to a pending job with no threads available to handle it.
@player-03

Copy link
Copy Markdown
Contributor Author

@barisyild Just fixed several bugs, including most of what you reported. Not sure about the lag; if it's still there you'll have to provide a sample.

@barisyild

Copy link
Copy Markdown
Contributor

@barisyild Just fixed several bugs, including most of what you reported. Not sure about the lag; if it's still there you'll have to provide a sample.

Bug still exists.

Assets.loadLibrary ("nolibrarywiththisname").onError(function (lib:AssetLibrary) {
	Sys.println("Error loading library!");
}).onComplete (function (lib:AssetLibrary) {
	Sys.println("Library loaded successfully!");
});

Old ThreadPool: Error loading library!
New ThreadPool: nothing

@player-03

Copy link
Copy Markdown
Contributor Author

That's fixed; make sure you pull the latest changes.

(I've tested on Linux, in C++, Neko, HL, and HTML5. For HTML5 I used trace() instead of println().)

@barisyild

barisyild commented Jun 27, 2025

Copy link
Copy Markdown
Contributor

That's fixed; make sure you pull the latest changes.

(I've tested on Linux, in C++, Neko, HL, and HTML5. For HTML5 I used trace() instead of println().)

It doesn't work for me either, by the way I use it with lime 8.2.2 by just changing the files.

https://github.com/openfl/lime/pull/1958/files

@barisyild

barisyild commented Jun 27, 2025

Copy link
Copy Markdown
Contributor

Bug

Code

public function new() {
  super();
  Sys.println("Started!");
  Assets.loadLibrary ("nolibrarywiththisname").onError(function (lib:AssetLibrary) {
	  Sys.println("Error loading library!");
  }).onComplete (function (lib:AssetLibrary) {
	  Sys.println("Library loaded successfully!");
  });
}

Output:

Started!

Working

Code

public function new() {
  super();
  Sys.println("Started!");
  Assets.loadLibrary ("nolibrarywiththisname").onError(function (lib:AssetLibrary) {
	  Sys.println("Error loading library!");
  }).onComplete (function (lib:AssetLibrary) {
	  Sys.println("Library loaded successfully!");
  });
  
  new Future<Void>(() -> Sys.println("1"));
}

Output:

Started!
1
Error loading library!

@player-03

player-03 commented Jun 27, 2025

Copy link
Copy Markdown
Contributor Author
public function new() {
  Sys.println("Started!");
  Assets.loadLibrary ("nolibrarywiththisname").onError(function (lib:AssetLibrary) {
     Sys.println("Error loading library!");
  }).onComplete (function (lib:AssetLibrary) {
     Sys.println("Library loaded successfully!");
  });
  
  new Future<Void>(() -> Sys.println("1"));
}

That looks exactly like the first error I fixed. The pool would shut down a bit too soon, so the error wouldn't be printed until the pool restarted.

Except that can't be what's going on here. Assets.loadLibrary() uses a different pool than new Future(), and starting a new pool does not make the old pool check its messages.

Perhaps the entire app is shutting down too soon in your case? What if you try haxe.Timer.delay(() -> Sys.println("1"), 1000); instead of new Future()?

@barisyild

Copy link
Copy Markdown
Contributor

That looks exactly like the first error I fixed. The pool would shut down a bit too soon, so the error wouldn't be printed until the pool restarted.

Except that can't be what's going on here. Assets.loadLibrary() uses a different pool than new Future(), and starting a new pool does not make the old pool check its messages.

Perhaps the entire app is shutting down too soon in your case? What if you try haxe.Timer.delay(() -> Sys.println("1"), 1000); instead of new Future()?

No, the application does not close.
Can you try reproduce?

@player-03

Copy link
Copy Markdown
Contributor Author

As stated, I've tried this plenty of times, on plenty of targets, and cannot reproduce the error.

Another thing: your new() function doesn't seem to call super(). Are you running this in a Lime or OpenFL app? Because it very much relies on Lime's event loop, and it's not going to work outside of that.

@barisyild

barisyild commented Jun 27, 2025

Copy link
Copy Markdown
Contributor

As stated, I've tried this plenty of times, on plenty of targets, and cannot reproduce the error.

Another thing: your new() function doesn't seem to call super(). Are you running this in a Lime or OpenFL app? Because it very much relies on Lime's event loop, and it's not going to work outside of that.

OpenFL, can you try this code on main class.

class Main extends Sprite
{
  public function new() {
    super();
    Sys.println("Started!");
    Assets.loadLibrary ("nolibrarywiththisname").onError(function (lib:AssetLibrary) {
	    Sys.println("Error loading library!");
    }).onComplete (function (lib:AssetLibrary) {
	    Sys.println("Library loaded successfully!");
    });
  }
}

@player-03

player-03 commented Jun 27, 2025

Copy link
Copy Markdown
Contributor Author

You know, that first lib:AssetLibrary should be error:String instead. I always fixed it on my end without bothering to mention it, but that might explain why mine works. When I copy-pasted your code, the app crashed with an error. When I changed it to this, it worked successfully, printing "Started!" and then "Error loading library!"

package;

import openfl.display.Sprite;
import openfl.Assets;
import openfl.utils.AssetLibrary;

class Main extends Sprite
{
  public function new() {
    super();
    Sys.println("Started!");
    Assets.loadLibrary ("nolibrarywiththisname").onError(function (error:String) {
	    Sys.println("Error loading library!");
    }).onComplete (function (lib:AssetLibrary) {
	    Sys.println("Library loaded successfully!");
    });
  }
}

Edit: or maybe it's specifically HL that cares about the difference. Neko and C++ seem to accept it just fine. How about you try in HL?

@barisyild

Copy link
Copy Markdown
Contributor

You know, that first lib:AssetLibrary should be error:String instead. I always fixed it on my end without bothering to mention it, but that might explain why mine works. When I copy-pasted your code, the app crashed with an error. When I changed it to this, it worked successfully, printing "Started!" and then "Error loading library!"

package;

import openfl.display.Sprite;
import openfl.Assets;
import openfl.utils.AssetLibrary;

class Main extends Sprite
{
  public function new() {
    super();
    Sys.println("Started!");
    Assets.loadLibrary ("nolibrarywiththisname").onError(function (error:String) {
	    Sys.println("Error loading library!");
    }).onComplete (function (lib:AssetLibrary) {
	    Sys.println("Library loaded successfully!");
    });
  }
}

Edit: or maybe it's specifically HL that cares about the difference. Neko and C++ seem to accept it just fine. How about you try in HL?

I'm trying on hxcpp, weird.

@player-03

Copy link
Copy Markdown
Contributor Author

HL does tend to be more picky about types. Might as well give it a try, just to see if it behaves differently in other ways.

Aside from that, try pulling from Git, to make sure you aren't missing anything.

git remote add player-03 https://github.com/player-03/lime.git
git fetch player-03
git checkout --track player-03/ThreadPool_Deque

Co-authored-by: Barış Yıldırım <25794892+barisyild@users.noreply.github.com>
@player-03

Copy link
Copy Markdown
Contributor Author

Update: we debugged this on Discord. It turned out to be a timing issue: there was a brief window where ThreadPool could incorrectly remove its update listener, and it never happened on my machine, but did on his.

Thanks to everyone who's tested so far. This is why it matters.

@47rooks

47rooks commented Jun 28, 2025

Copy link
Copy Markdown

I tried building my threadpool example with this PR and it's failing with :

Error: SDL_hidapi_steamdeck.c
c1: fatal error C1083: Cannot open source file: './lib/sdl/src/joystick/hidapi/SDL_hidapi_steamdeck.c': No such file or directory

I'm on Win 10 Haxe 4.3.6 and I pulled this branch with this command into a local haxelib in the project

haxelib git lime https://github.com/player-03/lime.git ThreadPool_Deque -debug

Anything I'm missing, doing incorrectly ?

@player-03

Copy link
Copy Markdown
Contributor Author

Looks like it's trying to rebuild the NDLLs, which is expected when installing from Git. You can often copy NDLLs from an existing Lime install (compatibility depends on how much changed between the two versions), or you can get them from the CI run for that commit (always compatible).

Or you can recompile them yourself. For that, you need to get the submodules using git submodule update --init, and you'll need the prerequisites listed here.

@47rooks

47rooks commented Jun 28, 2025

Copy link
Copy Markdown

Ok, that was it. Thanx - I think I've done this before but not for a while evidently.
Good news is my 10 thread fibonacci test program ran as expected with this change, in both cpp and hl targets.
Next is to try my flixel parallel loader. And that also worked on cpp and HL.

@player-03

Copy link
Copy Markdown
Contributor Author

Great, thanks for testing!

@joshtynjala

Copy link
Copy Markdown
Member

I confirm that this fixes issue #1895.

@player-03

Copy link
Copy Markdown
Contributor Author

Great!

Since no one else has reported any issues, and there has been a big push for this, I think it's time.

@player-03
player-03 merged commit f6393fc into openfl:8.3.0-Dev Jul 7, 2025
31 checks passed
@player-03
player-03 deleted the ThreadPool_Deque branch July 7, 2025 20:24
@barisyild

barisyild commented Jul 14, 2025

Copy link
Copy Markdown
Contributor

There is still issue in my tests, but it's incredibly rare.

Could the problem be due to lack of mutex?

It seems like other threads are accessing the __queuedExitEvents value.

@player-03

Copy link
Copy Markdown
Contributor Author

It seems like other threads are accessing the __queuedExitEvents value.

__queuedExitEvents and similar values are only accessed from the main thread. What makes you think otherwise?

@rainyt

rainyt commented Aug 14, 2025

Copy link
Copy Markdown
Contributor

Sorry, Recently, while testing Android, I encountered some new issues. I am using the 8.3.0dev branch, which includes this change. On cpp, URLLoader will perform loading without any successful or failed callbacks. How should I proceed with the next steps of testing and inspection?

@rainyt

rainyt commented Aug 14, 2025

Copy link
Copy Markdown
Contributor
2025-08-14 16:37:02.743 22553-22798/? I/trace: lime/system/ThreadPool.hx:398: run,{ uri => manifest/default.json, instance => NativeHTTPRequest },null
2025-08-14 16:37:02.750 22553-22798/? I/trace: lime/system/ThreadPool.hx:459: 0,WORK,null,idleThreads,1,activeThreads,0
2025-08-14 16:37:02.777 22553-22798/? I/trace: lime/system/ThreadPool.hx:459: 0,PROGRESS,null,idleThreads,0,activeThreads,1
2025-08-14 16:37:02.777 22553-22798/? I/trace: lime/system/ThreadPool.hx:459: 0,COMPLETE,null,idleThreads,0,activeThreads,1
2025-08-14 16:37:02.804 22553-22798/? I/trace: lime/system/ThreadPool.hx:398: run,Object,null
2025-08-14 16:37:02.805 22553-22798/? I/trace: lime/system/ThreadPool.hx:459: null,IDLE,null,idleThreads,0,activeThreads,1
2025-08-14 16:37:02.811 22553-22798/? I/trace: lime/system/ThreadPool.hx:459: 1,WORK,null,idleThreads,1,activeThreads,0
2025-08-14 16:37:02.811 22553-22798/? I/trace: lime/system/ThreadPool.hx:459: 1,COMPLETE,null,idleThreads,0,activeThreads,1
2025-08-14 16:37:02.812 22553-22798/? I/trace: lime/system/ThreadPool.hx:459: null,IDLE,null,idleThreads,0,activeThreads,1
2025-08-14 16:37:02.827 22553-22798/? I/trace: lime/system/ThreadPool.hx:398: run,CURLMulti,null
2025-08-14 16:37:02.833 22553-22798/? I/trace: lime/system/ThreadPool.hx:459: 2,WORK,null,idleThreads,1,activeThreads,0
2025-08-14 16:37:02.969 22553-22798/? I/trace: lime/system/ThreadPool.hx:459: 2,PROGRESS,null,idleThreads,0,activeThreads,1
2025-08-14 16:37:02.969 22553-22798/? I/trace: lime/system/ThreadPool.hx:459: 2,PROGRESS,null,idleThreads,0,activeThreads,1
2025-08-14 16:37:02.971 22553-22798/? I/trace: lime/system/ThreadPool.hx:459: 2,COMPLETE,null,idleThreads,0,activeThreads,1
2025-08-14 16:37:02.971 22553-22798/? I/trace: lime/system/ThreadPool.hx:459: null,IDLE,null,idleThreads,0,activeThreads,1
2025-08-14 16:37:02.986 22553-22798/? I/trace: lime/system/ThreadPool.hx:398: run,CURLMulti,null
2025-08-14 16:37:02.986 22553-22798/? I/trace: lime/system/ThreadPool.hx:459: null,EXIT,null,idleThreads,1,activeThreads,0

Is this helpful?

@rainyt

rainyt commented Aug 14, 2025

Copy link
Copy Markdown
Contributor

There must still be a problem, I fixed it after rolling back the code. I don't know how to solve it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants