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

Remove GlslangInitializer mutex lock? #945

Closed
repi opened this issue Jan 3, 2020 · 5 comments · Fixed by #1059
Closed

Remove GlslangInitializer mutex lock? #945

repi opened this issue Jan 3, 2020 · 5 comments · Fixed by #1059
Assignees

Comments

@repi
Copy link

repi commented Jan 3, 2020

Currently shaderc can't be run in parallel at all, it will internally synchronize all threads in GlslangInitializer::Acquire which has a global mutex.

Found a 4 year old comment there about it:
// TODO(awoloszyn): Once glslang no longer has static global mutable state remove this class

Do you know if this is still the case? That glslang has static global mutable state that still requires using this global lock?

I did a quick local test with my shader test suite to remove the mutex in GlslangInitializer and did unlock all my 32 threads when building thousands of shaders, and seem to work. But could of course be some lingering state in glslang that can cause issues with this.

But would be most excellent to be able to get rid of this lock and enable parallel compilation of shaders!

@dneto0
Copy link
Collaborator

dneto0 commented Jan 28, 2020

I plan to revisit this.

@repi
Copy link
Author

repi commented Jan 28, 2020

Thanks!

@dneto0 dneto0 self-assigned this Jan 28, 2020
@dneto0
Copy link
Collaborator

dneto0 commented Mar 24, 2020

Following up a bit. Summary of discussion with @johnkslang:

Glslang has an internal symbol table for builtins, that takes a while to build up. It's intended to be reused across many compiles, and potentially many otherwise-uncoordinated clients. The usage model is:

  • Can have multiple clients
  • Each client can have lots of threads
  • Each client should call ShInitialize once and ShFinalize once

Shaderc can be one client. It's best to tear down when shaderc is truly done forever for the process it is within.

@johnkslang
Copy link

johnkslang commented Mar 25, 2020

From the OP:

Do you know if this is still the case? That glslang has static global mutable state that still requires using this global lock?

No, we believe this is no longer the case. Glslang was designed to not need a global lock across a compile, just briefly to check for (and build if necessary) the built-in symbol tables that are shared across compiles. There was indeed a glslang issue when shaderc added its lock, but I believe the lock should now not be required, as per the Nov. 20, 2017 set of fixes around this in glslang.

@repi
Copy link
Author

repi commented Mar 25, 2020

Thanks for the confirmation! That is good news

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants