Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use WorkerThreadPool for Server threads #77004

Conversation

reduz
Copy link
Member

@reduz reduz commented May 12, 2023

  • Servers now use WorkerThreadPool for background computation.
  • This helps keep the number of threads used fixed at all times.
  • It also ensures everything works on HTML5 with threads.
  • And makes it easier to support disabling threads for also HTML5.

The only downside of this is that GLES3 threads likely no longer work because every time the render thread is called, the thread context is different. This needs to be researched how to fix. Maybe if GLES3 is used, threaded rendering server should be disabled for the time being?

@clayjohn
Copy link
Member

I think for GLES3 we need to take another approach as calling make_current() flushes all existing commands before returning.

https://learn.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-wglmakecurrent
https://registry.khronos.org/OpenGL-Refpages/gl2.1/xhtml/glXMakeCurrent.xml
https://registry.khronos.org/EGL/sdk/docs/man/html/eglMakeCurrent.xhtml

Note: We actual call it every time we switch between a window for multi-window rendering right now which is not ideal.

@reduz
Copy link
Member Author

reduz commented May 12, 2023

Maybe for GLES3 we can just always reserve the same thread of the threadpool so it can only be used for it and keep the others away. @RandomShaper what do you think?

}

main_thread = Thread::get_caller_id();
main_thread = Thread::MAIN_ID;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one can go away, can't it?

@RandomShaper
Copy link
Member

Maybe for GLES3 we can just always reserve the same thread of the threadpool so it can only be used for it and keep the others away. @RandomShaper what do you think?

I'm not sure... If a thread is fully reserved, it can't do other work. If it's an affinity-like semantic, if GL work is needed when it's thread pool slot happens to be busy with something else, it will have to wait when maybe there are other free slots. Now I think of it, that can work as long as the thread pool only schedules non-GL work in the GL thread when all others are busy.

@AThousandShips
Copy link
Member

You accidentally included a rebase conflict

@reduz
Copy link
Member Author

reduz commented Jan 20, 2024

@AThousandShips oops.. well, I need to finish the PR anyway so I guess I will just push when its working again.

@reduz reduz force-pushed the use-worker-thread-pool-for-server-threading branch 3 times, most recently from 149737b to 900dc33 Compare January 26, 2024 13:16
* Servers now use WorkerThreadPool for background computation.
* This helps keep the number of threads used fixed at all times.
* It also ensures everything works on HTML5 with threads.
* And makes it easier to support disabling threads for also HTML5.

The only downside of this is that GLES3 threads likely no longer work because every time the render thread is called, the thread context is different. This needs to be researched how to fix. Maybe if GLES3 is used, threaded rendering server should be disabled for the time being?
@reduz reduz force-pushed the use-worker-thread-pool-for-server-threading branch from 900dc33 to 30c92ca Compare January 27, 2024 19:28
@reduz reduz marked this pull request as ready for review January 27, 2024 19:29
@reduz reduz requested review from a team as code owners January 27, 2024 19:29
@reduz
Copy link
Member Author

reduz commented Jan 27, 2024

Okay this is done, though it needs some more discussion (And potentially a small improvement to WorkerThreadPool from @RandomShaper).

@akien-mga
Copy link
Member

Superseded by #90268.

@akien-mga akien-mga closed this Apr 9, 2024
@AThousandShips AThousandShips removed this from the 4.x milestone Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants