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

Fix handling of pthread naming API for LLVM on Windows #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

akien-mga
Copy link
Contributor

The previous two-step method relying on std:thread:native_handle() is tricky
as it's implementation-defined, and it seems that LLVM's libc++ does not return
a pthread_t handle even when using pthread (and the MSVC code path is not an
option for LLVM on Windows).

With mingw-gcc as packaged in Linux distros with pthread support it worked
fine, but with llvm-mingw it was problematic.
It seems like it could also be problematic with clang-cl.

Setting the name in the thread directly as done for Apple platforms is simpler
and should work fine.

Co-authored-by: @hpvb

The previous two-step method relying on `std:thread:native_handle()` is tricky
as it's implementation-defined, and it seems that LLVM's libc++ does not return
a `pthread_t` handle even when using pthread (and the MSVC code path is not an
option for LLVM on Windows).

With mingw-gcc as packaged in Linux distros with pthread support it worked
fine, but with llvm-mingw it was problematic.
It seems like it could also be problematic with clang-cl.

Setting the name in the thread directly as done for Apple platforms is simpler
and should work fine.

Co-authored-by: Hein-Pieter van Braam-Stewart <[email protected]>
Comment on lines +35 to +36
# else // Linux or MinGW.
pthread_setname_np( pthread_self(), tmp );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some extra #elif would be needed to support OpenBSD, FreeBSD and NetBSD, as can be seen in Godot's own pthread name setting interface:
https://github.com/godotengine/godot/blob/a86e7c3bb7d9238e664be8a7c63d1cc19bde0d8c/drivers/unix/thread_posix.cpp#L38-L63

For this PR I kept the changes minimal, but we do try to support *BSD platforms on a best-effort basis in Godot, so if you're open to that I could add relevant changes to support those (at least theoretically, I don't have current VMs for all three flavors to test the changes). But there might be other issues for *BSD compatibility so this is properly best left as a potential follow-up once we get to it in Godot.

@wolfpld
Copy link
Owner

wolfpld commented Apr 13, 2021

This code is a bit obsolete. I now keep the most recent version in https://github.com/wolfpld/tracy/blob/master/common/TracySystem.cpp. I think it would be best to just use it instead, given that Tracy is already a dependency for etcpak. It also brings some order into how things work, i.e. setting thread name is only possible from within the thread itself.

@akien-mga
Copy link
Contributor Author

akien-mga commented Apr 14, 2021

The tracy code for this looks a lot more cross-platform indeed, I agree that it seems to be a good replacement.

I'm a bit worried about using Tracy as a dependency directly, it seems like nothing in etcpak actually relies on Tracy currently. The code we're vendoring for Godot is only the "library" bits (not Application.cpp, nor Tracy) and this seems to work well for our use case (we like to keep our library dependencies as small as possible): https://github.com/godotengine/godot/tree/master/thirdparty/etcpak

If you plan to add a tighter integration between etcpak and Tracy in the future, maybe that can be guarded behind a compiler define? For Godot's needs we mostly care about having a fast and reliable VRAM compression library, but not so much inspecting performance of the tool itself beyond "it's fast!". On the other hand for etcpak development I fully understand that Tracy can be extremely useful.

That's maybe more something to consider for the long run, but there could maybe be a clearer separation between the etcpak library, ready to use by downstream applications, the etcpak CLI application which uses it, and the development tooling that lets you and other contributors do efficient research and optimization for fast and reliable compression.

Of course that depends on how big the interest is in using the library itself. We're just getting started with it in Godot and things look good, but it hasn't passed the test of time and a stable release yet :)

@wolfpld
Copy link
Owner

wolfpld commented Apr 14, 2021

That's maybe more something to consider for the long run, but there could maybe be a clearer separation between the etcpak library, ready to use by downstream applications, the etcpak CLI application which uses it, and the development tooling that lets you and other contributors do efficient research and optimization for fast and reliable compression.

Typically I'd expect people to just extract the Compress* functions into their own code, as these functions do all the heavy magic here, i.e. they accept an image buffer with a number of blocks and output the compressed data. Proper threading can be then left to the user, if needed, as it involves just a trivial calculation of how many blocks should be processed per each job. This may as well be just what you need.

The rest of code may be a bit hairy, as it has to juggle loading images, generating mips, and at the same time it has to perform the compression. The data is also directly streamed to disk, as it's measurably faster than using intermediate buffers. So, things are a bit involved here, and I don't really see much sense in extracting these functionalities to be easier to use externally.

Let me know how this feels for you.

@akien-mga
Copy link
Contributor Author

Thanks, that's very helpful advice. Our actual code using the library was implemented by @fire and I hadn't done a deep dive into the API, but I had a look now and could confirm that we only need the Compress* functions, and thus the Process*.cpp files and their direct dependencies.

That enabled me to slim down the vendored code significantly: godotengine/godot#47890
And that solves any portability issue we may have with the rest of the code of the etcpak application.

So I think this is good as is from our point of view. 👍

My advice for potential other users of the compression code could be to split it better as a self-contained sublibrary, so that it's easier to identify (and guarantee) that it can be used standalone. E.g. a structure like this:

compress/
  Dither.*
  ProcessCommon.hpp
  ProcessDxtc.*
  ProcessRGB.*
common/
  ForceInline.hpp
  Math.hpp
  Tables.*
  Vector.hpp
app/
  Application.*
  Bitmap.*
  BlockData.*
  etc.

So it could be clearer to downstream users that to use the compression API they'd just need compress/ and common/.

@Anutrix
Copy link

Anutrix commented May 15, 2022

What's the status on this?

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.

3 participants